[PATCH 2/2] chipidea: Use devm_request_irq()

[PATCH 2/2] chipidea: Use devm_request_irq()

[Uwe Kleine-König]

[Expanded Cc: a bit]

Hello,

On Wed, Jul 31, 2013 at 10:05:12AM +0100, Mark Brown wrote:
> On Wed, Jul 31, 2013 at 10:46:45AM +0200, Uwe Kleine-K?nig wrote:
We're discussing about devm_request_irq and wonder what happens at
remove time when the irq is still active.

> > OK, so the possible problem is that remove is called while the irq is
> > still active. That means you have to assert that all resources the irq
> > handler is using (e.g. ioremap, clk_prepare_enable) are only freed
> > *after* the irq is done. For ioremap that means it must be done using
> > devm_ioremap_resource. For a clock it's not that easy because the irq
> > handler has to assert that a used clk is kept prepared which can only be
> > done using clk_prepare which in turn is not allowed in an irq handler.
> 
> > Hmm. So the only possible fixes are
> > 	- devm* can be told to also care about clk_disable_unprepare
> > 	- after disabling irqs in the remove callback wait for all
> > 	  active irqs to be done. (i.e. call synchronize_irq(irq))
> > 	- don't use devm_request_irq
> 
> I'm not sure that devm_ guarantees any ordering in the cleanups it does
> so I'd not like to rely on the first option either, if there were some
> guarantee of that it'd help.  The nice thing about explicitly freeing
> the IRQ is that you can tell that all this stuff is safe by inspection.
devm_* at least uses list_for_each_entry_reverse
(drivers/base/devres.c:release_nodes()). Without this guarantee devm_
would not make much sense IMHO.

To also manage clks, we'd need something like:

	devm_clk_prepare(&dev, some_clk);

that makes devm_clk_release also call clk_unprepare the right number of
times. Maybe also the same for devm_clk_enable? Does this make sense?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

Hello,

On Wed, Jul 31, 2013 at 11:44:34AM +0200, Uwe Kleine-K?nig wrote:
> > > OK, so the possible problem is that remove is called while the irq is
> > > still active. That means you have to assert that all resources the irq

If your driver destruction path is running while your irq handler is
still running, it's a crappy / broken driver.  You need a deactivation
step whether you're using devm or not.  IRQs can be shared and the
device should be in a quiesced state before the driver detaches
itself.  Note that you can queue deactivation routine using devm.  For
an example, please take a look at
drivers/ata/libata-core.c::ata_host_start().

> > > handler is using (e.g. ioremap, clk_prepare_enable) are only freed
> > > *after* the irq is done. For ioremap that means it must be done using
> > > devm_ioremap_resource. For a clock it's not that easy because the irq
> > > handler has to assert that a used clk is kept prepared which can only be
> > > done using clk_prepare which in turn is not allowed in an irq handler.
> > 
> > > Hmm. So the only possible fixes are
> > > 	- devm* can be told to also care about clk_disable_unprepare
> > > 	- after disabling irqs in the remove callback wait for all
> > > 	  active irqs to be done. (i.e. call synchronize_irq(irq))
> > > 	- don't use devm_request_irq

Again, the right thing to do is having a proper deactivation step.
This is nothing devm can do automatically.  There's no way for it to
find out that the device is actually quiesced.  Let's say it waits for
the current instance of irq handler to finish.  How would it know that
it won't start again between the flushing of the current instance and
irq deregistration.  Add an explicit deactivation step using
devres_alloc().

> > I'm not sure that devm_ guarantees any ordering in the cleanups it does
> > so I'd not like to rely on the first option either, if there were some
> > guarantee of that it'd help.  The nice thing about explicitly freeing
> > the IRQ is that you can tell that all this stuff is safe by inspection.
> devm_* at least uses list_for_each_entry_reverse
> (drivers/base/devres.c:release_nodes()). Without this guarantee devm_
> would not make much sense IMHO.

devm guarantees that the destruction callbacks are called in the
reverse order of registration.

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Uwe Kleine-König]

On Wed, Jul 31, 2013 at 05:54:11AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 31, 2013 at 11:44:34AM +0200, Uwe Kleine-K?nig wrote:
> > > > OK, so the possible problem is that remove is called while the irq is
> > > > still active. That means you have to assert that all resources the irq
> 
> If your driver destruction path is running while your irq handler is
> still running, it's a crappy / broken driver.  You need a deactivation
Well, you cannot avoid assuming that the irq is still active when your
driver's remove callback is called. But I agree about crappyness at the
end of the destruction path. The problem is that crap is as easy as:

	probe(..)
	{
		clk = devm_get_clk(...);
		clk_prepare_enable(clk);
		writel(1, base + IRQENABLE);
		devm_request_irq(...);
	}

	remove(..)
	{
		writel(0, base + IRQENABLE);
		clk_disable_unprepare(clk);
	}

and I think there are more and more drivers doing that.

> step whether you're using devm or not.  IRQs can be shared and the
> device should be in a quiesced state before the driver detaches
> itself.  Note that you can queue deactivation routine using devm.  For
> an example, please take a look at
> drivers/ata/libata-core.c::ata_host_start().
> 
> > > > handler is using (e.g. ioremap, clk_prepare_enable) are only freed
> > > > *after* the irq is done. For ioremap that means it must be done using
> > > > devm_ioremap_resource. For a clock it's not that easy because the irq
> > > > handler has to assert that a used clk is kept prepared which can only be
> > > > done using clk_prepare which in turn is not allowed in an irq handler.
> > > 
> > > > Hmm. So the only possible fixes are
> > > > 	- devm* can be told to also care about clk_disable_unprepare
> > > > 	- after disabling irqs in the remove callback wait for all
> > > > 	  active irqs to be done. (i.e. call synchronize_irq(irq))
> > > > 	- don't use devm_request_irq
> 
> Again, the right thing to do is having a proper deactivation step.
> This is nothing devm can do automatically.  There's no way for it to
> find out that the device is actually quiesced.  Let's say it waits for
> the current instance of irq handler to finish.  How would it know that
> it won't start again between the flushing of the current instance and
> irq deregistration.  Add an explicit deactivation step using
> devres_alloc().
> 
> > > I'm not sure that devm_ guarantees any ordering in the cleanups it does
> > > so I'd not like to rely on the first option either, if there were some
> > > guarantee of that it'd help.  The nice thing about explicitly freeing
> > > the IRQ is that you can tell that all this stuff is safe by inspection.
> > devm_* at least uses list_for_each_entry_reverse
> > (drivers/base/devres.c:release_nodes()). Without this guarantee devm_
> > would not make much sense IMHO.
> 
> devm guarantees that the destruction callbacks are called in the
> reverse order of registration.
That's fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

On Wed, Jul 31, 2013 at 12:28:53PM +0200, Uwe Kleine-K?nig wrote:
> Well, you cannot avoid assuming that the irq is still active when your
> driver's remove callback is called. But I agree about crappyness at the
> end of the destruction path. The problem is that crap is as easy as:
> 
> 	probe(..)
> 	{
> 		clk = devm_get_clk(...);
> 		clk_prepare_enable(clk);
> 		writel(1, base + IRQENABLE);
> 		devm_request_irq(...);
> 	}
> 
> 	remove(..)
> 	{
> 		writel(0, base + IRQENABLE);
> 		clk_disable_unprepare(clk);
> 	}
> 
> and I think there are more and more drivers doing that.

Oh, so, the problem is that the driver is mixing devm and non-devm
resource management and ending up messing the order of shutdown?
Well, the obvious solution is using devm for clk too.  devm does
provide constructs to build custom destruction sequence so that such
calls can be mixed but it's a bit of hassle and mostly meant for
driver midlayers (libata, firewire, usb and so on) so that they can
provide nicely wrapped init/exit helpers for low level drivers.

In general, partial devm conversion can easily lead to subtle shutdown
order problems and it's best to go all the way.

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 05:54:11AM -0400, Tejun Heo wrote:
> On Wed, Jul 31, 2013 at 11:44:34AM +0200, Uwe Kleine-K?nig wrote:

> > > > OK, so the possible problem is that remove is called while the irq is
> > > > still active. That means you have to assert that all resources the irq

> If your driver destruction path is running while your irq handler is
> still running, it's a crappy / broken driver.  You need a deactivation
> step whether you're using devm or not.  IRQs can be shared and the
> device should be in a quiesced state before the driver detaches
> itself.  Note that you can queue deactivation routine using devm.  For
> an example, please take a look at
> drivers/ata/libata-core.c::ata_host_start().

I'm not sure I understand how this relates the problem.  The main issue
here is that for the shared IRQ case quiescing the device doesn't make
any difference since one of the other users of the interrupt could cause
the interrupt handler to be called regardless of what the hardware is
doing.  This means that we need to guarantee that anything the interrupt
handler relies on has not been deallocated before the interrupt handler
is unregistered.

> irq deregistration.  Add an explicit deactivation step using
> devres_alloc().

> devm guarantees that the destruction callbacks are called in the
> reverse order of registration.

OK, that's helpful.  It'd be good to document this if it's something
the API is intending to guarantee, though - devres.txt doesn't mention
this and it's not something I'd intuitively expect to be the case.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/e15fad37/attachment.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

Hello,

On Wed, Jul 31, 2013 at 12:18:53PM +0100, Mark Brown wrote:
> I'm not sure I understand how this relates the problem.  The main issue
> here is that for the shared IRQ case quiescing the device doesn't make
> any difference since one of the other users of the interrupt could cause
> the interrupt handler to be called regardless of what the hardware is
> doing.  This means that we need to guarantee that anything the interrupt
> handler relies on has not been deallocated before the interrupt handler
> is unregistered.

Yeah, if all resources are allocated using devm - note that you can
hook in non-devm resources using devres_alloc() - all resources which
would be necessary for the interrupt handler would have been allocated
before the irq was allocated, right?  And thus they'll of course
released after the IRQ is freed.  The problem arises when devm and
non-devm releases are mixed as non-devm ones would happen before all
devm ones messing up the release sequencing.

> OK, that's helpful.  It'd be good to document this if it's something
> the API is intending to guarantee, though - devres.txt doesn't mention

Oh, it's definitely guaranteed.  Nothing would work otherwise.

> this and it's not something I'd intuitively expect to be the case.

Oops... I guess I forgot to mention that.  Care to submit a patch?

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 07:32:44AM -0400, Tejun Heo wrote:

> Yeah, if all resources are allocated using devm - note that you can
> hook in non-devm resources using devres_alloc() - all resources which
> would be necessary for the interrupt handler would have been allocated
> before the irq was allocated, right?  And thus they'll of course
> released after the IRQ is freed.  The problem arises when devm and
> non-devm releases are mixed as non-devm ones would happen before all
> devm ones messing up the release sequencing.



> > OK, that's helpful.  It'd be good to document this if it's something
> > the API is intending to guarantee, though - devres.txt doesn't mention

> Oh, it's definitely guaranteed.  Nothing would work otherwise.

Most things would work just fine - most of the uses of devm_ are just
resource allocations that can safely be freed in essentially any order.
It doesn't really matter if you free the driver's private structure
before you free the clock that's pointing to it or whatever since
neither has any real connection to the other.

> > this and it's not something I'd intuitively expect to be the case.

> Oops... I guess I forgot to mention that.  Care to submit a patch?

I'll take a look.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/c5f64924/attachment.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

On Wed, Jul 31, 2013 at 12:50:27PM +0100, Mark Brown wrote:
> Most things would work just fine - most of the uses of devm_ are just
> resource allocations that can safely be freed in essentially any order.
> It doesn't really matter if you free the driver's private structure
> before you free the clock that's pointing to it or whatever since
> neither has any real connection to the other.

If you have DMA / IRQ / command engine deactivations in devm path
which often is the case with full conversions, freeing any resources
including DMA areas and host private data in the wrong order is a
horrible idea.  It's worse as it won't really be noticeable in most
cases.

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 07:55:27AM -0400, Tejun Heo wrote:

> If you have DMA / IRQ / command engine deactivations in devm path
> which often is the case with full conversions, freeing any resources
> including DMA areas and host private data in the wrong order is a
> horrible idea.  It's worse as it won't really be noticeable in most
> cases.

It's really only interrupts that affect most devices - if there's DMA or
anything going on after the remove() then as you said earlier the driver
is probably doing something wrong.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/cd025781/attachment.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

Hello,

On Wed, Jul 31, 2013 at 02:27:08PM +0100, Mark Brown wrote:
> It's really only interrupts that affect most devices - if there's DMA or
> anything going on after the remove() then as you said earlier the driver
> is probably doing something wrong.

Hmmm... it depends on the specific driver is converted but if the
deactivation sequence - shutting down of command engine - is also
handled by devm as in libata and if you have non-devres resource free
in the exit path, you have the same problem.  Again, in general,
tearing things down in the order which isn't the reverse of allocation
is a bad idea.  Adhering to the order isn't that hard and will result
in far less craziness in the long term.

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 09:42:15AM -0400, Tejun Heo wrote:
> On Wed, Jul 31, 2013 at 02:27:08PM +0100, Mark Brown wrote:

> > It's really only interrupts that affect most devices - if there's DMA or
> > anything going on after the remove() then as you said earlier the driver
> > is probably doing something wrong.

> Hmmm... it depends on the specific driver is converted but if the
> deactivation sequence - shutting down of command engine - is also
> handled by devm as in libata and if you have non-devres resource free
> in the exit path, you have the same problem.  Again, in general,

That's the only API I've ever heard of doing that.  Everything else is
just using it to do deallocation.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/052b3772/attachment.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

On Wed, Jul 31, 2013 at 02:57:51PM +0100, Mark Brown wrote:
> That's the only API I've ever heard of doing that.  Everything else is
> just using it to do deallocation.

I'm not sure why or what you're trying to argue here but take a look
at devm_pwm_release() for example.  It calls back into low level
driver free routine.  Are you arguing that it'd be a good idea to
release pci regions before this is complete?  It's just stupid to do
any differently.  There's nothing to argue about.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 10:07:58AM -0400, Tejun Heo wrote:
> On Wed, Jul 31, 2013 at 02:57:51PM +0100, Mark Brown wrote:

> > That's the only API I've ever heard of doing that.  Everything else is
> > just using it to do deallocation.

> I'm not sure why or what you're trying to argue here but take a look
> at devm_pwm_release() for example.  It calls back into low level
> driver free routine.  Are you arguing that it'd be a good idea to

That the callback is into the driver providing the PWM, not into the
driver that's using the PWM and is releasing it.

> release pci regions before this is complete?  It's just stupid to do
> any differently.  There's nothing to argue about.

What I'm saying is that in essentially all the users I've seen devm is
only being used for things like kfree() or clk_put() which aren't really
connected in any way and can happen in any order.  This (coupled with
the lack of documentation that this is supported) is why people are
nervous about anything that relies on ordering with this stuff - aside
from ATA everything is just using this for straight frees.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/5bc05f02/attachment.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

On Wed, Jul 31, 2013 at 04:25:23PM +0100, Mark Brown wrote:
> What I'm saying is that in essentially all the users I've seen devm is
> only being used for things like kfree() or clk_put() which aren't really
> connected in any way and can happen in any order.  This (coupled with
> the lack of documentation that this is supported) is why people are
> nervous about anything that relies on ordering with this stuff - aside
> from ATA everything is just using this for straight frees.

Yeah, sure, thank you very much for your input.  It is of course
strictly ordered and the documentation needs to be updated.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Mark Brown]

On Wed, Jul 31, 2013 at 11:29:32AM -0400, Tejun Heo wrote:

> Yeah, sure, thank you very much for your input.  It is of course
> strictly ordered and the documentation needs to be updated.

While I note the way you're saying this given the widespread adoption I
think there's a bit more effort needed on making sure people are aware
of this model and that it's possible to do it - most of the APIs with
devres support really ought to have things like a devm_clk_get_enable()
(an example discussed further up the thread) added to them.

devres has been quite widely adopted but not in a way that supports this
usage model.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130731/7ff5a400/attachment-0001.sig>

[PATCH 2/2] chipidea: Use devm_request_irq()

[Peter Chen]

On Wed, Jul 31, 2013 at 9:27 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jul 31, 2013 at 07:55:27AM -0400, Tejun Heo wrote:
>
>> If you have DMA / IRQ / command engine deactivations in devm path
>> which often is the case with full conversions, freeing any resources
>> including DMA areas and host private data in the wrong order is a
>> horrible idea.  It's worse as it won't really be noticeable in most
>> cases.
>
> It's really only interrupts that affect most devices - if there's DMA or
> anything going on after the remove() then as you said earlier the driver
> is probably doing something wrong.

I think the main point is we should allocate managed resource which is used
at interrupt handler before devm_request_irq, and all resources used
at interrupt
handler should be managed.

If we use non-managed resource at interrupt handler, but using managed interrupt
handler, things still will go wrong if there is an odd (unexpected)
interrupt after
we finish deactivation at removal.

-- 
BR,
Peter Chen

[PATCH 2/2] chipidea: Use devm_request_irq()

[Tejun Heo]

hello,

On Wed, Jul 31, 2013 at 09:55:26PM +0800, Peter Chen wrote:
> I think the main point is we should allocate managed resource which is used
> at interrupt handler before devm_request_irq, and all resources used
> at interrupt
> handler should be managed.
> 
> If we use non-managed resource at interrupt handler, but using managed interrupt
> handler, things still will go wrong if there is an odd (unexpected)
> interrupt after
> we finish deactivation at removal.

In general, applying devm partially isn't a good idea.  It's very easy
to get into trouble thanks to release order dependency which isn't
immediately noticeable and there have been actual bugs caused by that.
The strategies which seem to work are either

* Convert everything over to devm by wrapping deactivation in a devres
  callback too.  As long as your init sequence is sane (ie. irq
  shouldn't be request before things used by irq are ready).

* Allocate all resources using devres but shut down the execution
  engine in the remove_one().  Again, as all releases are controlled
  by devres, you won't have to worry about messing up the release
  sequencing.

Thanks.

-- 
tejun

[PATCH 2/2] chipidea: Use devm_request_irq()

[Peter Chen]

On Wed, Jul 31, 2013 at 10:15:12AM -0400, Tejun Heo wrote:
> hello,
> 
> On Wed, Jul 31, 2013 at 09:55:26PM +0800, Peter Chen wrote:
> > I think the main point is we should allocate managed resource which is used
> > at interrupt handler before devm_request_irq, and all resources used
> > at interrupt
> > handler should be managed.
> > 
> > If we use non-managed resource at interrupt handler, but using managed interrupt
> > handler, things still will go wrong if there is an odd (unexpected)
> > interrupt after
> > we finish deactivation at removal.
> 
> In general, applying devm partially isn't a good idea.  It's very easy
> to get into trouble thanks to release order dependency which isn't
> immediately noticeable and there have been actual bugs caused by that.
> The strategies which seem to work are either
> 
> * Convert everything over to devm by wrapping deactivation in a devres
>   callback too.  As long as your init sequence is sane (ie. irq
>   shouldn't be request before things used by irq are ready).
> 
> * Allocate all resources using devres but shut down the execution
>   engine in the remove_one().  Again, as all releases are controlled
>   by devres, you won't have to worry about messing up the release
>   sequencing.
> 

thanks, Tejun. So, Alex and Fabio, this patch may not be suitable currently,
since many resources at both EHCI and device side are non-managed.
-- 

Best Regards,
Peter Chen