Re: [PATCH v11 3/3] dmaengine: switchtec-dma: Implement descriptor submission

Logan Gunthorpe
2025-12-23T18:41:02Z

On 2025-12-23 03:33, Vinod Koul wrote:
> On 19-12-25, 10:42, Logan Gunthorpe wrote:
>> On 2025-12-16 04:50, Vinod Koul wrote:
>>> What about the descriptor, you should push into a pending queue or
>>> something here?
>>
>> There's only the one queue in this driver and it is added to directly by
>> switchtec_dma_prep_memcpy(). So there's nothing more to do on the
>> hardware's side to submit the job. The jobs added to the queue with the
>> prep() function will start to be processed when the sq_tail pointer is
>> set in switchtec_dma_issue_pending() below.
>
> That is not really correct dmaengine semantics, please fix that.
> We expect the descriptor to be prepared in the prep_xxx call. Then we
> expect it to be submitted in a queue and finally pushed to hardware on
> issue_pending.

Ok, I was digging into this. The PLX driver this was based on is a bit
different, the switchtec one is actually closer to the IOAT driver. Both
have a ring queue with a head index that gets incremented for each entry.

As best as I can tell the IOAT driver increments a produce value after a
prep_xxx call and then in tx_submit function it copies the the produce
value to the actual head value. The switchtec driver, unlike IOAT, only
ever "produces" one reqeust at a time, but I think if we move the head
increment from the prep command into the tx_submit() call it will be
correct.
>> We've answered this question a couple times now. See my detailed
>> response here:
>>
>> https://lore.kernel.org/all/e759d483-e303-421a-b674-72fd9121750d@deltatee.com/
>>
>> Essentially this is the only place we have to do the cleanup when
>> running jobs with interrupts disabled (a valid use case). Both PLX and
>> IOAT do similar things for good reasons and we'd really rather not have
>> the downsides noted in the link above without a more compelling
>> technical reason for this being incorrect.
>
> This feels wrong :-) Relying on query to cleanup should not be done. A
> client may not issue this. It is not a mandatory thing to do!
> I would not suggest enabling interrupts, as you suggested in the link
> above. I am sure you are getting at least some completion notices? The
> tasklet can then do the cleanup as well. that would be a better reliable
> way to do this
>
> We should fix the other two drivers!

Yes, it is not mandatory to call tx_status() nor is it mandatory to be
called with the IOAT, PLX and Swithctec drivers as they are currently
written. The only "completion notice" the hardware can send is the
hardware interrupt and if we want to disable that interrupt for a polled
mode of operation then the completions need to be processed somewhere
and the only place to do that is in tx_status().

The way I read things is there are two ways to operate: with or without
interrupts.

1) If interrupts are used (DMA_PREP_INTERRUPT is set), then the tasklet
will be triggered by the hardware interrupt and the tasklet will handle
the cleanup. The tasklet calls the callback which signals the user. In
this situation *if* tx_status() is called, the dma cookie will likely
already be set and the function will return early without calling
cleanup(). If tx_status() doesn't get called, the tasklet would process
the completion and call the callback and everything works fine. If
tx_status() happens to be called before the interrupt occurs, then
cleanup() would be called but chances are nothing will be done; or it
could cleanup a descriptor before the tasklet if the timing is just
right (but that would not be a problem either).

2) If interrupts are not used (DMA_PREP_INTERRUPT is not set), then no
interrupt will occur and thus the tasklet will never run. The only way
for the user to know if a job is done is by calling tx_status(). In
which case, the driver must query the hardware for pending completions
during the tx_status() call. There is no other place to process the
completions.

The only option we have to remove the cleanup() call from tx_status()
would be to essentially ignore DMA_PREP_INTERRUPT and thus the hardware
interrupt will always occur for every request regardless of whether the
user needs to be interrupted or not. This means applications that don't
need the interrupt will incur extra hardware interrupts when they aren't
really needed. I personally think that is a bad choice: it is much
better to disable the hardware interrupt if nothing is waiting for it
and process the completions in tx_status() when it is polled.

Logan