On 07/06/2016 06:31 PM, Vinod Koul wrote:
> On Tue, Jul 05, 2016 at 11:10:22PM +0200, Linus Walleij wrote:
>> On Tue, Jul 5, 2016 at 4:54 PM, Vinod Koul <vinod.koul@xxxxxxxxx> wrote:
>>
>>> dmaengine device should explicitly call devm_free_irq() when using
>>> devm_reqister_irq().
>>>
>>> The irq is still ON when devices remove is executed and irq should be
>>> quiesced before remove is completed.
>>>
>>> Signed-off-by: Vinod Koul <vinod.koul@xxxxxxxxx>
>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>
>> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>
>> This patch makes me very uneasy. A spurious IRQ can always happen, so
>> Isn't this problem existing on more or less the .remove() path of 95%
>> if all drivers
>> using devm_request_irq() in the entire kernel?
>
> Yes it is indeed a problem. IMHO we should never have done devm variant
> for irq APIs. I have been pushing back on drivers not freeing up irqs in
> .remove.
>
> Maybe we should have devm variants made deprecated and discourage the
> use of these..
There are safe use-cases for devm_request_irq(), but I agree by default it
should be avoided since it is really easy to get it wrong.
In general any resource that is accessed in the interrupt handler needs to
be available until after the IRQ has been freed. This is regardless of
whether devm_ is used or not.
The rule of thumb for the devm case is that it is only safe to use it if
free_irq() is the last line in your remove function.
E.g.
static int probe(...)
{
...
dev_state_struct = kzalloc();
...
request_irq(...);
...
return 0;
}
static int remove(...)
{
...
free_irq(...);
kfree(dev_state_struct);
return 0;
}
is not save to convert (assuming dev_state_struct is used in the IRQ
handler) to devm_request_irq since that would change the order in which
resources are released. Switching to devm would free the memory before the
IRQ which can trigger a use after free.
On the other hand
static int probe(...)
{
...
dev_state_struct = devm_kzalloc();
...
request_irq(...);
...
return 0;
}
static int remove(...)
{
...
free_irq(...);
return 0;
}
is safe to switch to devm_request_irq() since it will not change the
ordering since free_irq() is already the last instruction executed in the
remove function. devm will make sure that deallocation happens in the
opposite order of allocation.
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
