Re: [PATCH v11 2/3] dmaengine: switchtec-dma: Implement hardware initialization and cleanup

[Vinod Koul <vkoul@kernel.org>]

On 19-12-25, 10:19, Logan Gunthorpe wrote:
> Hi Vinod,
> 
> Thanks for reviewing this. I've queued up a bunch of changes based on
> your feedback, but I have some questions and notes.
> 
> On 2025-12-16 04:00, Vinod Koul wrote:
> >> +	struct switchtec_dma_desc **desc_ring;
> > 
> > This is bit interesting. Any reason why you choose double pointer here.
> > If you take a look at other driver, they follow similar approach but
> > dont use double pointer for this.
> > 
> 
> The ring size has had to be quite large. It is currently set to 32K in
> order to be able to get maximum performance in some work loads. While
> each element is only 104 bytes, the total array is more than 3MB which
> is too much for a single allocation. The PLX dma driver this is based on
> does the same thing even though it's queue size is only 2048.

Ring size is fine, the question is about use of a double pointer instead
of a single pointer

> >> +		if (swdma_chan->cq_tail == 0)
> >> +			swdma_chan->phase_tag = !swdma_chan->phase_tag;
> >> +
> >> +		/*  Out of order CE */
> >> +		if (se_idx != tail) {
> >> +			spin_unlock_bh(&swdma_chan->complete_lock);
> >> +			continue;
> >> +		}
> >> +
> >> +		do {
> >> +			dma_cookie_complete(&desc->txd);
> >> +			dma_descriptor_unmap(&desc->txd);
> >> +			dmaengine_desc_get_callback_invoke(&desc->txd, &res);
> >> +			desc->txd.callback = NULL;
> >> +			desc->txd.callback_result = NULL;
> > 
> > Not filling results?
> 
> I'm not sure the question here. This code calls the callback with the
> result and then clears the callback for the next use of the descriptor.
> 
> >> +			desc->completed = false;
> > 
> > ?
> > 
> 
> The completed flag is used to track which requests in the ring have been
> completed. It is set to true when it's completed and then cleared here
> when they have been cleaned up again for reuse in a subsequent
> transaction. The completions can happen out of order but they are
> cleaned up in order before they can be reused.
> >> @@ -127,9 +1140,13 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
> >>  {
> >>  	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> >>  
> >> +	switchtec_dma_chans_release(pdev, swdma_dev);
> >> +
> >>  	rcu_assign_pointer(swdma_dev->pdev, NULL);
> >>  	synchronize_rcu();
> >>  
> >> +	pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
> >> +
> > 
> > This is good. But what about the tasklet, that needs to be quiesced too
> > 
> 
> I'm not sure what you are looking for, I couldn't find any explicit
> quiesce function that is commonly used on teardown in other drivers.
> Clearing the pdev pointer above and synchronizing the RCU should ensure
> there are no other jobs in progress or tasklets running.
> 
> Note: I have tested teardown while running dmatest and have not found
> any issues.

The tasklet can be schedule and then you free irq which creates a racy
situation, so I always recommend to kill the tasklet after freeing the
irq!

-- 
~Vinod