linux-kernel - Re: [PATCH] dmaengine: add verification of DMA

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 31 May 2022 10:54:00 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Dave Jiang <dave.jiang@...el.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        dmaengine <dmaengine@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] dmaengine: add verification of DMA_INTERRUPT capability
 for dmatest

On 30-05-22, 10:06, Geert Uytterhoeven wrote:
> Hi Dave, Vinod,

Hi Geert,

> 
> On Wed, Apr 13, 2022 at 12:58 AM Dave Jiang <dave.jiang@...el.com> wrote:
> > Looks like I forgot to add DMA_INTERRUPT cap setting to the idxd driver and
> > dmatest is still working regardless of this mistake. Add an explicit check
> > of DMA_INTERRUPT capability for dmatest to make sure the DMA device being used
> > actually supports interrupt before the test is launched and also that the
> > driver is programmed correctly.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@...el.com>
> 
> Thanks for your patch, which is now commit a8facc7b988599f8
> ("dmaengine: add verification of DMA_INTERRUPT capability for
> dmatest") upstream.
> 
> > --- a/drivers/dma/dmatest.c
> > +++ b/drivers/dma/dmatest.c
> > @@ -675,10 +675,16 @@ static int dmatest_func(void *data)
> >         /*
> >          * src and dst buffers are freed by ourselves below
> >          */
> > -       if (params->polled)
> > +       if (params->polled) {
> >                 flags = DMA_CTRL_ACK;
> > -       else
> > -               flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
> > +       } else {
> > +               if (dma_has_cap(DMA_INTERRUPT, dev->cap_mask)) {
> > +                       flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
> > +               } else {
> > +                       pr_err("Channel does not support interrupt!\n");
> > +                       goto err_pq_array;
> > +               }
> > +       }
> >
> >         ktime = ktime_get();
> >         while (!(kthread_should_stop() ||
> > @@ -906,6 +912,7 @@ static int dmatest_func(void *data)
> 
> Shimoda-san reports that this commit breaks dmatest on rcar-dmac.
> Like most DMA engine drivers, rcar-dmac does not set the DMA_INTERRUPT
> capability flag, hence dmatest now fails to start:
> 
>     dmatest: Channel does not support interrupt!
> 
> To me, it looks like the new check is bogus, as I believe it confuses
> two different concepts:
> 
>   1. Documentation/driver-api/dmaengine/provider.rst says:
> 
>        - DMA_INTERRUPT
> 
>          - The device is able to trigger a dummy transfer that will
>            generate periodic interrupts
> 
>   2. In non-polled mode, dmatest sets DMA_PREP_INTERRUPT.
>      include/linux/dmaengine.h says:
> 
>        * @DMA_PREP_INTERRUPT - trigger an interrupt (callback) upon
> completion of
>        *  this transaction
> 
> As dmatest uses real transfers, I think it does not depend on
> the ability to use interrupts from dummy transfers.

Yes this does not look right to me. DMA_INTERRUPT is for a specific
capability which is linked to dma_prep_interrupt() which dmatest does
not use so i think it is not correct for dmatest to use this...

I can revert this patch... Dave?

-- 
~Vinod