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

[htejun@gmail.com (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