linux-kernel - Re: [PATCH 28/33] dmaengine: at_hdmac: Use devm_request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 20 Oct 2022 07:24:03 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <vkoul@...nel.org>
CC:     <peda@...ntia.se>, <du@...ntia.se>, <regressions@...mhuis.info>,
        <Ludovic.Desroches@...rochip.com>, <maciej.sosnowski@...el.com>,
        <dan.j.williams@...el.com>, <Nicolas.Ferre@...rochip.com>,
        <mripard@...nel.org>, <torfl6749@...il.com>,
        <linux-kernel@...r.kernel.org>, <dmaengine@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 28/33] dmaengine: at_hdmac: Use devm_request_irq()

On 10/19/22 19:49, Vinod Koul wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 20-08-22, 15:57, Tudor Ambarus wrote:
>> IRQs requested with this function will be automatically freed on driver
>> detach. Use devm_request_irq() and make the code cleaner.
> 
> That is not really good idea!
> 
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@...rochip.com>
>> ---
>>  drivers/dma/at_hdmac.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
>> index 9c414f167b62..96b885f83374 100644
>> --- a/drivers/dma/at_hdmac.c
>> +++ b/drivers/dma/at_hdmac.c
>> @@ -2241,6 +2241,10 @@ static int __init at_dma_probe(struct platform_device *pdev)
>>       irq = platform_get_irq(pdev, 0);
>>       if (irq < 0)
>>               return irq;
>> +     err = devm_request_irq(&pdev->dev, irq, at_dma_interrupt, 0,
>> +                            dev_name(&pdev->dev), atdma);
>> +     if (err)
>> +             return err;
>>
>>       /* discover transaction capabilities */
>>       atdma->dma_device.cap_mask = plat_dat->cap_mask;
>> @@ -2257,10 +2261,6 @@ static int __init at_dma_probe(struct platform_device *pdev)
>>       /* force dma off, just in case */
>>       at_dma_off(atdma);
>>
>> -     err = request_irq(irq, at_dma_interrupt, 0, "at_hdmac", atdma);
>> -     if (err)
>> -             goto err_irq;
> 
> here you are ready to receive the irq, but now you moved it early on..
> it may cause issues..

Oh, yes.

> 
>> -
>>       platform_set_drvdata(pdev, atdma);
>>
>>       /* create a pool of consistent memory blocks for hardware descriptors */
>> @@ -2377,8 +2377,6 @@ static int __init at_dma_probe(struct platform_device *pdev)
>>  err_memset_pool_create:
>>       dma_pool_destroy(atdma->dma_desc_pool);
>>  err_desc_pool_create:
>> -     free_irq(platform_get_irq(pdev, 0), atdma);
>> -err_irq:
>>       clk_disable_unprepare(atdma->clk);
>>  err_clk_prepare:
>>       clk_put(atdma->clk);
>> @@ -2397,7 +2395,6 @@ static int at_dma_remove(struct platform_device *pdev)
>>
>>       dma_pool_destroy(atdma->memset_pool);
>>       dma_pool_destroy(atdma->dma_desc_pool);
>> -     free_irq(platform_get_irq(pdev, 0), atdma);
> 
> Now we have an active irq while we are unrolling device, which can be
> fired and cause tasklet to also get scheduled...
> 
> You need to make sure now irq cant fire here and tasklet is killed..

right, I should have disabled the irq here and I missed it. Thanks, will
respin.

> 
>>
>>       list_for_each_entry_safe(chan, _chan, &atdma->dma_device.channels,
>>                       device_node) {
>> --
>> 2.25.1
> 
> --
> ~Vinod

-- 
Cheers,
ta