Dangerous devm_request_irq() conversions

Dangerous devm_request_irq() conversions

[Dmitry Torokhov]

Hi,

It looks like a whole slew of devm_request_irq() conversions just got
applied to mainline and many of them are quite broken.

Consider fd5231ce336e038037b4f0190a6838bdd6e17c6d or
c1879fe80c61f3be6f2ddb82509c2e7f92a484fe: the drivers udsed first to
free IRQ and then unregister the corresponding device ensuring that IRQ
handler, while it runs, has the device available. The mechanic
conversion to devm_request_irq() reverses the order of these operations
opening the race window where IRQ can reference device (or other
resource) that is already gone.

It would be nice if these could be reverted and revioewed again for
correctness.

In general any conversion to devm_request_irq() needs double and triple
checking.

Thanks.

-- 
Dmitry

Re: Dangerous devm_request_irq() conversions

[Jingoo Han]

On Friday, February 22, 2013 3:54 PM, Dmitry Torokhov wrote:
> 
> Hi,
> 
> It looks like a whole slew of devm_request_irq() conversions just got
> applied to mainline and many of them are quite broken.
> 
> Consider fd5231ce336e038037b4f0190a6838bdd6e17c6d or
> c1879fe80c61f3be6f2ddb82509c2e7f92a484fe: the drivers udsed first to
> free IRQ and then unregister the corresponding device ensuring that IRQ
> handler, while it runs, has the device available. The mechanic
> conversion to devm_request_irq() reverses the order of these operations
> opening the race window where IRQ can reference device (or other
> resource) that is already gone.
> 
> It would be nice if these could be reverted and revioewed again for
> correctness.

Um, other RTC drivers already have been using devm_request_threaded_irq() or
devm_request_irq() like this, before I added these patches.

For example, 
./drivers/rtc/rtc-tegra.c
./drivers/rtc/rtc-spear.c
./drivers/rtc/rtc-s3c.c
./drivers/rtc/rtc-mxc.c
./drivers/rtc/rtc-ds1553.c
./drivers/rtc/rtc-ds1511.c
./drivers/rtc/rtc-snvs.c
./drivers/rtc/rtc-imxdi.c
./drivers/rtc/rtc-tx4939.c
./drivers/rtc/rtc-mv.c
./drivers/rtc/rtc-coh901331.c
./drivers/rtc/rtc-stk17ta8.c
./drivers/rtc/rtc-lpc32xx.c
./drivers/rtc/rtc-tps65910.c
./drivers/rtc/rtc-rc5t583.c


Also, even more, some RTC drivers calls rtc_device_unregister() first,
then calls free_irq() later.

For example,
./drivers/rtc/rtc-vr41xx.c
./drivers/rtc/rtc-da9052.c
./drivers/rtc/rtc-isl1208.c
./drivers/rtc/rtc-88pm860x.c
./drivers/rtc/rtc-tps6586x.c
./drivers/rtc/rtc-mpc5121.c
./drivers/rtc/rtc-m48t59.c


Please, don't argue revert without concrete reasons.

If these devm_request_threaded_irq() or devm_request_irq() make the problem,
devm_free_irq() will be added later.


> 
> In general any conversion to devm_request_irq() needs double and triple
> checking.
> 
> Thanks.
> 
> --
> Dmitry

Re: Dangerous devm_request_irq() conversions

[Dmitry Torokhov]

On Fri, Feb 22, 2013 at 04:12:36PM +0900, Jingoo Han wrote:
> On Friday, February 22, 2013 3:54 PM, Dmitry Torokhov wrote:
> > 
> > Hi,
> > 
> > It looks like a whole slew of devm_request_irq() conversions just got
> > applied to mainline and many of them are quite broken.
> > 
> > Consider fd5231ce336e038037b4f0190a6838bdd6e17c6d or
> > c1879fe80c61f3be6f2ddb82509c2e7f92a484fe: the drivers udsed first to
> > free IRQ and then unregister the corresponding device ensuring that IRQ
> > handler, while it runs, has the device available. The mechanic
> > conversion to devm_request_irq() reverses the order of these operations
> > opening the race window where IRQ can reference device (or other
> > resource) that is already gone.
> > 
> > It would be nice if these could be reverted and revioewed again for
> > correctness.
> 
> Um, other RTC drivers already have been using devm_request_threaded_irq() or
> devm_request_irq() like this, before I added these patches.
> 
> For example, 
> ./drivers/rtc/rtc-tegra.c
> ./drivers/rtc/rtc-spear.c
> ./drivers/rtc/rtc-s3c.c
> ./drivers/rtc/rtc-mxc.c
> ./drivers/rtc/rtc-ds1553.c
> ./drivers/rtc/rtc-ds1511.c
> ./drivers/rtc/rtc-snvs.c
> ./drivers/rtc/rtc-imxdi.c
> ./drivers/rtc/rtc-tx4939.c
> ./drivers/rtc/rtc-mv.c
> ./drivers/rtc/rtc-coh901331.c
> ./drivers/rtc/rtc-stk17ta8.c
> ./drivers/rtc/rtc-lpc32xx.c
> ./drivers/rtc/rtc-tps65910.c
> ./drivers/rtc/rtc-rc5t583.c
> 
> 
> Also, even more, some RTC drivers calls rtc_device_unregister() first,
> then calls free_irq() later.
> 
> For example,
> ./drivers/rtc/rtc-vr41xx.c
> ./drivers/rtc/rtc-da9052.c
> ./drivers/rtc/rtc-isl1208.c
> ./drivers/rtc/rtc-88pm860x.c
> ./drivers/rtc/rtc-tps6586x.c
> ./drivers/rtc/rtc-mpc5121.c
> ./drivers/rtc/rtc-m48t59.c
> 
> 
> Please, don't argue revert without concrete reasons.

What more concrete reason do you need? I explained to you the exact
reason on the patches I noticed before and also on the 2 commits
referenced above: blind conversion to devm_* changes order of operation
which may be deadly with IRQs (but others, like clocks and regulators,
are important too).

The fact that crap slipped in the kernel before is not the valid reason
for adding more of the same crap.

Please *understand* APIs you are using before making changes.

> 
> If these devm_request_threaded_irq() or devm_request_irq() make the problem,
> devm_free_irq() will be added later.

And the point? If you use devm_request_irq() and then call
devm_free_irq() manually in all paths what you achieved is waste of
memory required for devm_* tracking.

-- 
Dmitry

RE: Dangerous devm_request_irq() conversions

[Jingoo Han]

On Friday, February 22, 2013 4:27 PM, Dmitry Torokhov wrote:
> On Fri, Feb 22, 2013 at 04:12:36PM +0900, Jingoo Han wrote:
> > On Friday, February 22, 2013 3:54 PM, Dmitry Torokhov wrote:
> > >
> > > Hi,
> > >
> > > It looks like a whole slew of devm_request_irq() conversions just got
> > > applied to mainline and many of them are quite broken.
> > >
> > > Consider fd5231ce336e038037b4f0190a6838bdd6e17c6d or
> > > c1879fe80c61f3be6f2ddb82509c2e7f92a484fe: the drivers udsed first to
> > > free IRQ and then unregister the corresponding device ensuring that IRQ
> > > handler, while it runs, has the device available. The mechanic
> > > conversion to devm_request_irq() reverses the order of these operations
> > > opening the race window where IRQ can reference device (or other
> > > resource) that is already gone.
> > >
> > > It would be nice if these could be reverted and revioewed again for
> > > correctness.
> >
> > Um, other RTC drivers already have been using devm_request_threaded_irq() or
> > devm_request_irq() like this, before I added these patches.
> >
> > For example,
> > ./drivers/rtc/rtc-tegra.c
> > ./drivers/rtc/rtc-spear.c
> > ./drivers/rtc/rtc-s3c.c
> > ./drivers/rtc/rtc-mxc.c
> > ./drivers/rtc/rtc-ds1553.c
> > ./drivers/rtc/rtc-ds1511.c
> > ./drivers/rtc/rtc-snvs.c
> > ./drivers/rtc/rtc-imxdi.c
> > ./drivers/rtc/rtc-tx4939.c
> > ./drivers/rtc/rtc-mv.c
> > ./drivers/rtc/rtc-coh901331.c
> > ./drivers/rtc/rtc-stk17ta8.c
> > ./drivers/rtc/rtc-lpc32xx.c
> > ./drivers/rtc/rtc-tps65910.c
> > ./drivers/rtc/rtc-rc5t583.c
> >
> >
> > Also, even more, some RTC drivers calls rtc_device_unregister() first,
> > then calls free_irq() later.
> >
> > For example,
> > ./drivers/rtc/rtc-vr41xx.c
> > ./drivers/rtc/rtc-da9052.c
> > ./drivers/rtc/rtc-isl1208.c
> > ./drivers/rtc/rtc-88pm860x.c
> > ./drivers/rtc/rtc-tps6586x.c
> > ./drivers/rtc/rtc-mpc5121.c
> > ./drivers/rtc/rtc-m48t59.c
> >
> >
> > Please, don't argue revert without concrete reasons.
> 
> What more concrete reason do you need? I explained to you the exact
> reason on the patches I noticed before and also on the 2 commits
> referenced above: blind conversion to devm_* changes order of operation
> which may be deadly with IRQs (but others, like clocks and regulators,
> are important too).
> 
> The fact that crap slipped in the kernel before is not the valid reason
> for adding more of the same crap.
> 
> Please *understand* APIs you are using before making changes.
> 
> >
> > If these devm_request_threaded_irq() or devm_request_irq() make the problem,
> > devm_free_irq() will be added later.
> 
> And the point? If you use devm_request_irq() and then call
> devm_free_irq() manually in all paths what you achieved is waste of
> memory required for devm_* tracking.

CC'ed Al Viro, Tejun Heo


So, is there any report that the devm_request_threaded_irq() makes
the deadly problem related IRQ in such cases?

According to your comment, it seems that there is no reason to use
devm_request_irq() or devm_request_threaded_irq().

Please, argue that it would be better to deprecate devm_request_irq()
or devm_request_threaded_irq().


> 
> --
> Dmitry

Re: Dangerous devm_request_irq() conversions

[Dmitry Torokhov]

On Fri, Feb 22, 2013 at 04:57:29PM +0900, Jingoo Han wrote:
> On Friday, February 22, 2013 4:27 PM, Dmitry Torokhov wrote:
> > On Fri, Feb 22, 2013 at 04:12:36PM +0900, Jingoo Han wrote:
> > > On Friday, February 22, 2013 3:54 PM, Dmitry Torokhov wrote:
> > > >
> > > > Hi,
> > > >
> > > > It looks like a whole slew of devm_request_irq() conversions just got
> > > > applied to mainline and many of them are quite broken.
> > > >
> > > > Consider fd5231ce336e038037b4f0190a6838bdd6e17c6d or
> > > > c1879fe80c61f3be6f2ddb82509c2e7f92a484fe: the drivers udsed first to
> > > > free IRQ and then unregister the corresponding device ensuring that IRQ
> > > > handler, while it runs, has the device available. The mechanic
> > > > conversion to devm_request_irq() reverses the order of these operations
> > > > opening the race window where IRQ can reference device (or other
> > > > resource) that is already gone.
> > > >
> > > > It would be nice if these could be reverted and revioewed again for
> > > > correctness.
> > >
> > > Um, other RTC drivers already have been using devm_request_threaded_irq() or
> > > devm_request_irq() like this, before I added these patches.
> > >
> > > For example,
> > > ./drivers/rtc/rtc-tegra.c
> > > ./drivers/rtc/rtc-spear.c
> > > ./drivers/rtc/rtc-s3c.c
> > > ./drivers/rtc/rtc-mxc.c
> > > ./drivers/rtc/rtc-ds1553.c
> > > ./drivers/rtc/rtc-ds1511.c
> > > ./drivers/rtc/rtc-snvs.c
> > > ./drivers/rtc/rtc-imxdi.c
> > > ./drivers/rtc/rtc-tx4939.c
> > > ./drivers/rtc/rtc-mv.c
> > > ./drivers/rtc/rtc-coh901331.c
> > > ./drivers/rtc/rtc-stk17ta8.c
> > > ./drivers/rtc/rtc-lpc32xx.c
> > > ./drivers/rtc/rtc-tps65910.c
> > > ./drivers/rtc/rtc-rc5t583.c
> > >
> > >
> > > Also, even more, some RTC drivers calls rtc_device_unregister() first,
> > > then calls free_irq() later.
> > >
> > > For example,
> > > ./drivers/rtc/rtc-vr41xx.c
> > > ./drivers/rtc/rtc-da9052.c
> > > ./drivers/rtc/rtc-isl1208.c
> > > ./drivers/rtc/rtc-88pm860x.c
> > > ./drivers/rtc/rtc-tps6586x.c
> > > ./drivers/rtc/rtc-mpc5121.c
> > > ./drivers/rtc/rtc-m48t59.c
> > >
> > >
> > > Please, don't argue revert without concrete reasons.
> > 
> > What more concrete reason do you need? I explained to you the exact
> > reason on the patches I noticed before and also on the 2 commits
> > referenced above: blind conversion to devm_* changes order of operation
> > which may be deadly with IRQs (but others, like clocks and regulators,
> > are important too).
> > 
> > The fact that crap slipped in the kernel before is not the valid reason
> > for adding more of the same crap.
> > 
> > Please *understand* APIs you are using before making changes.
> > 
> > >
> > > If these devm_request_threaded_irq() or devm_request_irq() make the problem,
> > > devm_free_irq() will be added later.
> > 
> > And the point? If you use devm_request_irq() and then call
> > devm_free_irq() manually in all paths what you achieved is waste of
> > memory required for devm_* tracking.
> 
> CC'ed Al Viro, Tejun Heo
> 
> 
> So, is there any report that the devm_request_threaded_irq() makes
> the deadly problem related IRQ in such cases?
> 
> According to your comment, it seems that there is no reason to use
> devm_request_irq() or devm_request_threaded_irq().

devm_request_irq() or devm_request_threaded_irq() are OK if:

1. _ALL_ resources in the driver are controlled by devm_* so that order
of freeing is not disturbed, or

2. You can shut off interrupts in the chip so that interrupts will not
be generated even though IRQ handler is installed, or

3. Some combination of above.

Note that it is not only interrupts, other resources like clocks and
regulators and pins and other objects are important too, IRQ is just the
most dangerous.

> 
> Please, argue that it would be better to deprecate devm_request_irq()
> or devm_request_threaded_irq().
> 

No, they do have their use. But probably people doing the conversion
should be required to put $100 in escrow ;)

-- 
Dmitry

Re: Dangerous devm_request_irq() conversions

['Tejun Heo']

Hey, guys.

On Fri, Feb 22, 2013 at 12:05:48AM -0800, Dmitry Torokhov wrote:
> > Please, argue that it would be better to deprecate devm_request_irq()
> > or devm_request_threaded_irq().
> 
> No, they do have their use. But probably people doing the conversion
> should be required to put $100 in escrow ;)

So, what devm does is simple - it records resources being allocated
using devm and releases them in the reverse order when the driver
detaches from the device.  If all or most resources are covered by
devm, this is the right thing to do most of the time and there's
nothing much to worry about.

However, singling out one resource and converting only that one to
devm is likely to break something if the resource in question has
release ordering dependency and irqs do definitely have ordering
requirements while tearing down, so, no, one can't single out
request_irq() and change only that one to devm_request_irq().  It's
like mixing managed objects with manual new/delete objects.  You can't
do that willy-nilly and it generaly isn't a good idea as evidenced by
only c++ doing it.

The recommended way to do devm conversion is doing it all the way.
Look at the whole subsystem, identify resources used by various
drivers (they usually aren't that diverse in a single subsystem),
identify the pattern and implement helpers in the subsystem as
appropriate and convert the whole thing, preferably to the point where
->remove() almost doesn't have to worry about freeing resources other
than shutting down devices or doing one-off cleanups not covered by
devm.

If there are resources which are frequently used by a group of drivers
but devm interface doesn't currently exist, add a devm interface
either as a generic or subsystem-specific helper.  devm is
specifically designed to allow such extensions.

So, in summary, no, please don't single out irq requests and convert
only those to devm.  That's bound to break stuff.

Thanks.

-- 
tejun