| Message ID | 164978679251.2361020.5856734256126725993.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a8facc7b988599f83a680d2d61f4607cda495175 |
| Headers | show |
| Series | dmaengine: add verification of DMA_INTERRUPT capability for dmatest | expand |
Hi Dave, Vinod, On Wed, Apr 13, 2022 at 12:58 AM Dave Jiang <dave.jiang@intel.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@intel.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. Do you agree? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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@intel.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@intel.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?
On 5/30/2022 10:24 PM, Vinod Koul wrote: > 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@intel.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@intel.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? Yes we can revert it.
diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c index f696246f57fd..0a2168a4ccb0 100644 --- 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) runtime = ktime_to_us(ktime); ret = 0; +err_pq_array: kfree(dma_pq); err_srcs_array: kfree(srcs);
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@intel.com> --- drivers/dma/dmatest.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)