linux-kernel - Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write handlers

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: <aSbhzoSKjVAIuY-m@smile.fi.intel.com>
Date: Wed, 26 Nov 2025 13:17:34 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: david laight <david.laight@...box.com>
Cc: Prajna Rajendra Kumar <prajna.rajendrakumar@...rochip.com>,
	linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
	Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v2 2/6] spi: microchip-core: Refactor FIFO read and write
 handlers

On Wed, Nov 26, 2025 at 09:21:45AM +0000, david laight wrote:
> On Wed, 26 Nov 2025 08:54:40 +0100
> Andy Shevchenko <andriy.shevchenko@...ux.intel.com> wrote:
> 
> > Make both handlers to be shorter and easier to understand.
> > While at it, unify their style.

...

> >  	for (int i = 0; i < fifo_max; i++) {

^^^	(1)

...

> >  		while (readb(spi->regs + MCHP_CORESPI_REG_STAT) &
> >  		       MCHP_CORESPI_STATUS_RXFIFO_EMPTY)
> >  			;
> 
> This is a hard spin until data is available.
> I think 'spi' is a bit like 'i2c' (etc) so is quite slow.
> Even if the code thinks there are 'fifo_max' bytes in the fifo it seems
> wrong to spin indefinitely.
> If the code is trying to read a response that is still arriving from the
> physical hardware is is positively wrong.
> If some hardware has a glitch that FIFO_EMPTY is temporarily incorrectly set
> after a read - then maybe you need some recovery code.
> Otherwise I suspect FIFO_EMPTY should generate a short read.
> 
> The write code (below) does an 'early terminate'' on fifo full.
> Presumably it is woken by an interrupt to continue the write?
> 
> I actually doubt that transferring messages that are larger than the
> device fifo is ever going to be completely reliable.
> You'd need to guarantee the interrupt latency to update the fifo be short
> enough to guarantee the fifo won't underflow/overflow.
> (Unless the spi hardware 'clock stops' the physical interface when the fifo
> if full/empty - which is effectively what happens when software 'bit-bangs'
> these serial interfaces.)

I also saw that code and it needs to be amended, but it's out of the scope of
this mini-series.

...

> > +	for (int i = 0; i < fifo_max; i++) {
> 
> unsigned int or u32 ??

int works as well and I won't to touch (1) above, less churn.

...

> > +			writeb(*spi->tx_buf++, spi->regs + MCHP_CORESPI_REG_TXDATA);
> > +		elsespi->regs + MCHP_CORESPI_REG_TXDATA);
> > +			writeb(0xaa, spi->regs + MCHP_CORESPI_REG_TXDATA);
> 
> I'm not sure I don't prefer the version with one writeb() call.
> How about:
> 		writeb(spi->tx_buf ? *spi->tx_buf++ : 0xaa,
> 			spi->regs + MCHP_CORESPI_REG_TXDATA);

I find ternary here is unreadable as regular if. With regular if we also see
the exact difference at a glance. I definitely prefer my variant.

...

Thanks for the review.

-- 
With Best Regards,
Andy Shevchenko