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: <f68231bc-6546-4eaf-a8b2-fc31add33a1f@baylibre.com>
Date: Tue, 29 Apr 2025 10:24:57 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.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 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".
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.)