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

Logan Gunthorpe
2025-12-19T17:20:17Z

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

Logan