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

Vinod Koul
2025-12-23T10:26:43Z

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