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: <3baed5bf-8fa1-4ef9-8cb1-58145a6dd37c@deltatee.com>
Date: Tue, 5 Aug 2025 13:15:34 -0600
From: Logan Gunthorpe <logang@...tatee.com>
To: Vinod Koul <vkoul@...nel.org>, Kelvin Cao <kelvin.cao@...rochip.com>
Cc: 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

Hi Vinod,

Kelvin has asked me to take over the upstreaming of the switchtec_dma
driver. It's been over a year since the last posting but we intend on
sending a new version shortly after the merge window. I've fixed a
number of issues and gone through the bulk of the changes requested but
there are two points that I'm going to have to push back on.

This driver shares a lot of similarities with the plx_dma driver I wrote
a few years ago and had similar issues.

On 2024-04-07 06:21, Vinod Koul wrote:
> On 18-03-24, 09:33, Kelvin Cao 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 =
>> +		to_switchtec_dma_chan(desc->chan);
>> +	dma_cookie_t cookie;
>> +
>> +	cookie = dma_cookie_assign(desc);
>> +
>> +	spin_unlock_bh(&swdma_chan->submit_lock);
> 
> I was expecting desc to be pushing to pending list?? where is that done

The driver maintains a pending list in DMA coherent memory. The pending
list is written directly in switchtec_dma_prep_desc(). So there's
nothing the hardware needs done when a new operation is submitted to the
queue. When switchtec_dma_issue_pending() is called, the sq_tail pointer
is updated which will signal the hardware to start pulling all the
queued requests. I don't see any other way this could be done in the
dmaengine framework.

> Also consider using virt-dma for desc management, you dont need to
> handle that on your own

I looked into this and a virtual queue does not make sense to me for
this device. The driver maintains it's own queue that hardware reads
directly so there would be no benefit in having another queue that then
needs to be copied to the queue the hardware reads from.

>> +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 = to_switchtec_dma_chan(chan);
>> +	enum dma_status ret;
>> +
>> +	ret = dma_cookie_status(chan, cookie, txstate);
>> +	if (ret == DMA_COMPLETE)
>> +		return ret;
>> +
>> +	switchtec_dma_process_desc(swdma_chan);
> 
> 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.

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.

Thanks,

Logan

[1]
https://lore.kernel.org/all/746371aa-b3f6-aaca-35f2-0f815294dc71@deltatee.com/




Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ