linux-kernel - Re: [PATCH 2/4] spi: axi-spi-engine: don't repeat mode config for offload

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <a380702d474e9b1f361f7224e20e40bdfb8a810b.camel@gmail.com>
Date: Tue, 29 Apr 2025 16:40:03 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Michael Hennerich	
 <michael.hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>,  Mark Brown <broonie@...nel.org>
Cc: linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] spi: axi-spi-engine: don't repeat mode config for
 offload

On Tue, 2025-04-29 at 10:24 -0500, David Lechner wrote:
> On 4/29/25 4:08 AM, Nuno Sá wrote:
> > Hi David,
> > 
> > The whole series LGTM but I do have a small concern (maybe an hypothetical
> > one)
> > in this one...
> > 
> > 
> > On Mon, 2025-04-28 at 15:58 -0500, David Lechner wrote:
> 
> 
> ...
> 
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_SYNC(0),
> > > +		spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_WRITE(SPI_ENGINE_CMD_REG_CONFIG,
> > > +					    priv->spi_mode_config),
> > > +		       spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > > +	writel_relaxed(SPI_ENGINE_CMD_SYNC(1),
> > > +		spi_engine->base + SPI_ENGINE_REG_CMD_FIFO);
> > > +
> > 
> > I would assume that SPI_ENGINE_CMD_SYNC(0) + SPI_ENGINE_CMD_SYNC(1) should
> > be
> > executed in order by the core? I think all this relaxed API's don't give any
> > guarantee about store completion so, in theory, we could have SYNC(0) after
> > SYNC(1). Even the full barrier variants don't guarantee this I believe [1].
> > There's ioremap_np() variant but likely not supported in many platforms.
> > Doing a
> > read() before SYNC(1) should be all we need to guarantee proper ordering
> > here.
> > 
> > Or maybe this is all theoretical and not an issue on the platforms this
> > driver
> > is supported but meh, just raising the possibility.
> > 
> > 
> > [1]:
> > https://elixir.bootlin.com/linux/v6.12-rc6/source/Documentation/driver-api/device-io.rst#L299
> > 
> 
> The way I am reading this, relaxed isn't "no barriers", just "weaker
> barriers".

Yes, sometimes just compiler ones. Bad phrasing from me...

> Quoting from the linked doc:
> 
> 	On architectures that require an expensive barrier for serializing
> 	against DMA, these “relaxed” versions of the MMIO accessors only
> 	serialize against each other, but contain a less expensive barrier
> 	operation.
> 
> So that sounds like we should not have a problem to me. (And if there is a
> problem, then it would affect other parts of the driver, like when we load
> the fifos with tx data in a loop.)

Well that just ensures that they are issued by the order you wrote them but it
does not guarantee that they end up in the same order on the peripheral itself.

Anyways, likely not an issue on the arches we care and as you mentioned it,
there are other places of the driver where this could matter. So, I guess not
material for this series.

- Nuno Sá