Re: [PATCH 2/2] chipidea: Use devm_request_irq()
On Tue, Jul 30, 2013 at 10:04:29PM -0300, Fabio Estevam wrote: From: Fabio Estevam fabio.este...@freescale.com 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.este...@freescale.com --- 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. I don't know the details and didn't find any problems with it that a plain request_irq doesn't have. But I wonder about the details and if others are aware of the possible problems. Mark, maybe you can describe in more detail the downsides you see? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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.este...@freescale.com 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.este...@freescale.com --- 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. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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.este...@freescale.com 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.este...@freescale.com --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
On Wed, Jul 31, 2013 at 10:46:45AM +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 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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
[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/ | -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
On Wed, Jul 31, 2013 at 9:27 PM, Mark Brown broo...@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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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. signature.asc Description: Digital signature
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] chipidea: Use devm_request_irq()
From: Fabio Estevam fabio.este...@freescale.com 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.este...@freescale.com --- 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); if (ret) goto stop; @@ -514,7 +514,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (!ret) return 0; - free_irq(ci-irq, ci); stop: ci_role_stop(ci); rm_wq: @@ -531,7 +530,6 @@ static int ci_hdrc_remove(struct platform_device *pdev) dbg_remove_files(ci); flush_workqueue(ci-wq); destroy_workqueue(ci-wq); - free_irq(ci-irq, ci); ci_role_stop(ci); return 0; -- 1.8.1.2 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] chipidea: Use devm_request_irq()
On Tue, Jul 30, 2013 at 10:04:29PM -0300, Fabio Estevam wrote: From: Fabio Estevam fabio.este...@freescale.com 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.este...@freescale.com --- 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); if (ret) goto stop; @@ -514,7 +514,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (!ret) return 0; - free_irq(ci-irq, ci); stop: ci_role_stop(ci); rm_wq: @@ -531,7 +530,6 @@ static int ci_hdrc_remove(struct platform_device *pdev) dbg_remove_files(ci); flush_workqueue(ci-wq); destroy_workqueue(ci-wq); - free_irq(ci-irq, ci); ci_role_stop(ci); return 0; -- 1.8.1.2 Reviewed-by: Peter Chen peter.c...@freescale.com -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
