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

[Vinod Koul <vkoul@kernel.org>]

On 19-12-25, 10:42, Logan Gunthorpe wrote:
> 
> 
> 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.

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. 

This is also the expectations from clients, so we would like to see this
behaviour in the driver as well.

> 
> >> +
> >> +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.

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!

> 
> >> +
> >> +	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.

That is interesting, can you add this in comments here, not very obvious
hardware behaviour

-- 
~Vinod