On Wed, Jul 31, 2013 at 04:20:55PM +0800, Peter Chen wrote: > On Wed, Jul 31, 2013 at 09:33:06AM +0200, Uwe Kleine-König wrote: > > On Tue, Jul 30, 2013 at 10:04:29PM -0300, Fabio Estevam wrote: > > > From: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx> > > > > > > By using devm_request_irq() we don't need to call free_irq(), which simplifies > > > the code a bit. > > > > > > Signed-off-by: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx> > > > --- > > > drivers/usb/chipidea/core.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > index 5cc1b02..d185c41 100644 > > > --- a/drivers/usb/chipidea/core.c > > > +++ b/drivers/usb/chipidea/core.c > > > @@ -502,8 +502,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > } > > > > > > platform_set_drvdata(pdev, ci); > > > - ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name, > > > - ci); > > > + ret = devm_request_irq(dev, ci->irq, ci_irq, IRQF_SHARED, > > > + ci->platdata->name, ci); > > Mark Brown (now on Cc:) replied to one of my patches using > > devm_request_irq: > > > > I'm always deeply suspicous of devm_request_irq() since you need > > to be *very* sure that the interrupt can't fire during cleanup > > and cause the handlers to try to use data structures that are > > already being freed. > > > > and: > > > > devm_request_threaded_irq() is just generally a bit of a warning > > sign since it needs careful checking to tell if it's safe. > > > > The probably problem I find is the free_irq will be called after > driver's removal is called, then the problem Mark described will occur. > See __device_release_driver(struct device *dev) at drivers/base/dd.c. So you mean the remove callback (dev->bus->remove or drv->remove) is called before devres_release_all, right? 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 Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
