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

2013-07-31 Thread Uwe Kleine-König
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 ---

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

2013-07-31 Thread Peter Chen
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.

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

2013-07-31 Thread Uwe Kleine-König
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

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

2013-07-31 Thread Mark Brown
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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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:

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread Peter Chen
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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-31 Thread 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

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

2013-07-30 Thread Fabio Estevam
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(-)

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

2013-07-30 Thread Peter Chen
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 ---