Hi Dave, Vinod,
On Wed, Apr 13, 2022 at 12:58 AM Dave Jiang <[email protected]> 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 <[email protected]>
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 -- [email protected]
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 <[email protected]> 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 <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]>
>> 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.