linux-kernel - Re: [PATCH v8 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA engine PCI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e759d483-e303-421a-b674-72fd9121750d@deltatee.com>
Date: Tue, 12 Aug 2025 10:32:58 -0600
From: Logan Gunthorpe <logang@...tatee.com>
To: Vinod Koul <vkoul@...nel.org>
Cc: Kelvin Cao <kelvin.cao@...rochip.com>, dmaengine@...r.kernel.org,
 linux-kernel@...r.kernel.org, George.Ge@...rochip.com,
 christophe.jaillet@...adoo.fr, hch@...radead.org
Subject: Re: [PATCH v8 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA
 engine PCI driver

On 2025-08-11 11:34, Vinod Koul wrote:
> On 05-08-25, 13:15, Logan Gunthorpe wrote:
>>> This is *wrong*, you cannot process desc in status API, Please read the
>>> documentation again and if in doubt pls ask
>>
>> I don't see any other way to do this. There's no other place to cleanup
>> the completed descriptors. This is exactly what was done in the plx-dma
>> driver which copied the IOAT driver that began this pattern. There was
>> very similar feedback when I submitted the plx-dma driver[1] and I
>> pointed out there's no other way to do this.
> 
> we should try fixing these
>>
>> I'll rename the function to make this a little clearer, but I can't see
>> any other way to implement the driver without doing this.
> 
> Dont you get a notification from hardware on completion, that should
> trigger this as well as issue_pending calls... 

I dug into this a bit more this morning.

For at least PLX and the new Switchtec drivers, the hardware interrupt
is disabled when DMA_PREP_INTERRUPT is set by the caller. This makes
sense to me seeing if the caller doesn't care to be interrupted quickly
then there's not much point in paying the cost of the interrupt and
tasklet.

However if, for any reason, the caller never sets DMA_PREP_INTERURPT
then the tasklet never fires and the descriptors never have an
opportunity to get cleaned up. Presumably such a caller would be calling
dmaengine_tx_status() to poll for finished jobs and thus it's a natural
place to poll and cleanup descriptors on the ring.

I personally think this is the correct and best way to go about it.
Cleaning up descriptors is fast and if the descriptors were already
handled in the tasklet then it doesn't need to do anything anyway and
there's negligible costs.

There are two other options I can see each with significant costs:

1) Change the DMA driver so that the interrupt and tasklet gets
triggered on every completion even if DMA_PREP_INTERRUPT is not set.
This means the interrupt and tasklet will always run even if the
application says it doesn't want an interrupt. This would increase the
overhead on applications that might want to submit multiple requests and
interrupt only on the last one or simply not use interrupts and poll for
completions. (Though I don't know if any applications like that exist at
the present time).

2) Remove the cleanup in the status() call and keep everything else the
same. This would keep the performance the same but if any application
tries to use the DMA engine without periodically setting
DMA_PREP_INTERRUPT, then the driver will hang as it will never cleanup
the used up entries in the ring.

So, again, I really think the way it is now is the best option and I
personally can not see what the down side is to it.

Logan