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/ | -- 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