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

Logan Gunthorpe
2025-12-19T17:42:40Z

On 2025-12-16 04:50, Vinod Koul wrote:
>> +static dma_cookie_t
>> +switchtec_dma_tx_submit(struct dma_async_tx_descriptor *desc)
>> + __releases(swdma_chan->submit_lock)
>> +{
>> + struct switchtec_dma_chan *swdma_chan =
>> + container_of(desc->chan, struct switchtec_dma_chan, dma_chan);
>> + dma_cookie_t cookie;
>> +
>> + cookie = dma_cookie_assign(desc);
>> +
>> + spin_unlock_bh(&swdma_chan->submit_lock);
>> +
>> + return cookie;
>> +}
>
> 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.

>> +
>> +static enum dma_status switchtec_dma_tx_status(struct dma_chan *chan,
>> + dma_cookie_t cookie, struct dma_tx_state *txstate)
>> +{
>> + struct switchtec_dma_chan *swdma_chan =
>> + container_of(chan, struct switchtec_dma_chan, dma_chan);
>> + enum dma_status ret;
>> +
>> + ret = dma_cookie_status(chan, cookie, txstate);
>> + if (ret == DMA_COMPLETE)
>> + return ret;
>> +
>> + switchtec_dma_cleanup_completed(swdma_chan);
>
> Why are we doing this on a status query??

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.

>> +
>> + return dma_cookie_status(chan, cookie, txstate);
>
> No setting residue?

residue is set in switchtec_dma_cleanup_completed() which is called a
couple lines up.

>> +
>> + spin_lock_bh(&swdma_chan->submit_lock);
>> + writew(swdma_chan->head, &swdma_chan->mmio_chan_hw->sq_tail);
>> + spin_unlock_bh(&swdma_chan->submit_lock);
>> +
>> + rcu_read_unlock();
>> +}
>
> This seems to assume that the channel is idle when issue_pending is
> invoked. That is not the right assumption, we can be running a txn so you
> need to check if channel is not running and start if it is idle

This is just updating the tail pointer for the hardware's submission
queue. If the channel is idle, it will start processing of a job. But if
the channel is not idle it informs the hardware there are more elements
in the queue. Not doing this if the hardware is not idle would mean the
hardware never gets informed there are more jobs in the queue. That
would not be correct.

Thanks,

Logan