linux-kernel - Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register settings

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: <20240920-jujitsu-botanical-31a58a1bc1da@thorsis.com>
Date: Fri, 20 Sep 2024 10:53:23 +0200
From: Alexander Dahl <ada@...rsis.com>
To: Mark Brown <broonie@...nel.org>
Cc: Alexander Dahl <ada@...rsis.com>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	Claudiu Beznea <claudiu.beznea@...on.dev>,
	Tudor Ambarus <tudor.ambarus@...aro.org>,
	"open list:SPI SUBSYSTEM" <linux-spi@...r.kernel.org>,
	"moderated list:ARM/Microchip (AT91) SoC support" <linux-arm-kernel@...ts.infradead.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] spi: atmel-quadspi: Avoid overwriting delay register
 settings

Hello Mark,

Am Fri, Sep 20, 2024 at 10:22:19AM +0200 schrieb Mark Brown:
> On Wed, Sep 18, 2024 at 10:27:43AM +0200, Alexander Dahl wrote:
> > Previously the MR and SCR registers were just set with the supposedly
> > required values, from cached register values (cached reg content
> > initialized to zero).
> > 
> > All parts fixed here did not consider the current register (cache)
> > content, which would make future support of cs_setup, cs_hold, and
> > cs_inactive impossible.
> > 
> > Setting SCBR in atmel_qspi_setup() erases a possible DLYBS setting from
> > atmel_qspi_set_cs_timing().  The DLYBS setting is applied by ORing over
> > the current setting, without resetting the bits first.  All writes to MR
> > did not consider possible settings of DLYCS and DLYBCT.
> > 
> > Signed-off-by: Alexander Dahl <ada@...rsis.com>
> > Fixes: f732646d0ccd ("spi: atmel-quadspi: Add support for configuring CS timing")
> 
> This isn't actually a fix AFAICT since nothing yet sets any of these
> fields?

You're right if we just consider board dts files in mainline.  None of
those using the atmel-quadspi driver have a spi-cs-*-delay property
set in a SPI slave device node in current master.

The changes in this patch to MR writes do not change behaviour.

For changes to SCR however I see two possible bugs:

1. if atmel_qspi_set_cs_timing() is called before atmel_qspi_setup(),
   the second call just overwrites what the first call set.

2. if atmel_qspi_set_cs_timing() is called multiple times with
   different values, the values written to the register from the second
   call onwards are just wrong.

Maybe both are scenarios not happening in practice?

Long story short, I could just remove the Fixes line, and the rest is
fine?  Or should I split up with changes for MR and SCR going to
separate patches?

Greets
Alex