linux-kernel - Re: [PATCH v4 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors

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: <f4616741-9763-4221-b385-74c6d332b092@linaro.org>
Date: Mon, 30 Jun 2025 11:46:40 +0100
From: James Clark <james.clark@...aro.org>
To: Mark Brown <broonie@...nel.org>, Frank Li <Frank.li@....com>
Cc: Vladimir Oltean <olteanv@...il.com>,
 Vladimir Oltean <vladimir.oltean@....com>, Arnd Bergmann <arnd@...db.de>,
 Larisa Grigore <larisa.grigore@....com>, Christoph Hellwig <hch@....de>,
 linux-spi@...r.kernel.org, imx@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors



On 27/06/2025 10:41 pm, Mark Brown wrote:
> On Fri, Jun 27, 2025 at 03:56:43PM -0400, Frank Li wrote:
>> On Fri, Jun 27, 2025 at 11:21:42AM +0100, James Clark wrote:
> 
>>> In target mode, the host sending more data than can be consumed would be
>>> a common problem for any message exceeding the FIFO or DMA buffer size.
>>> Cancel the whole message as soon as this condition is hit as the message
>>> will be corrupted.
> 
>>> Only do this for target mode in a DMA transfer, it's not likely these
>>> flags will be set in host mode
> 
>> "not likely", I think SPI controller should stop CLK if FIFO empty and full.
>> It should be "never" happen.
> 
>> Only check FIFO error flags at target mode because it never happen at host mode.
> 
>> You can remove below whole paragram.
> 
> I agree it *should* never happen in host mode but it can be good
> practice to look in case something gets messed up.  That said extra
> device accesses in the hot path are probably going to be noticable so
> likely not worth it, but in the dspi_poll() case:
> 

Yeah the point was to be defensive. Even though it shouldn't happen I 
don't see an issue with checking error flags.

We could add a condition to only do it in target mode, but it doesn't 
make the code more readable, and it wouldn't be any faster than just 
checking the flags. So I'm not sure what problem we're trying to solve 
by removing it.

>> @@ -1067,6 +1080,9 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>                          regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>>                          regmap_write(dspi->regmap, SPI_SR, spi_sr);
>>
>> +                       dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
>> +                       if (dspi->cur_msg->status)
>> +                               return;
> 
> we're reading the register anyway so the overhead is much lower.

In both poll and interrupt mode we're already reading it. Only DMA mode 
didn't have a read and I didn't add a check for error flags there anyway.