linux-kernel - Re: [PATCH 1/4] spi: spi-fsl-dspi: Clear completion counter before initiating transfer

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: <20250610213120.ib5tkbh4xu7af4jr@skbuf>
Date: Wed, 11 Jun 2025 00:31:20 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: James Clark <james.clark@...aro.org>
Cc: Vladimir Oltean <olteanv@...il.com>, Mark Brown <broonie@...nel.org>,
	linux-spi@...r.kernel.org, imx@...ts.linux.dev,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] spi: spi-fsl-dspi: Clear completion counter before
 initiating transfer

On Wed, Jun 11, 2025 at 12:01:47AM +0300, Vladimir Oltean wrote:
> On Tue, Jun 10, 2025 at 04:41:04PM +0100, James Clark wrote:
> > On 10/06/2025 12:34 pm, Vladimir Oltean wrote:
> > > On Mon, Jun 09, 2025 at 04:32:38PM +0100, James Clark wrote:
> > > > In target mode, extra interrupts can be received between the end of a
> > > > transfer and halting the module if the host continues sending more data.
> > > 
> > > Presumably you mean not just any extra interrupts can be received, but
> > > specifically CMDTCF, since that triggers the complete(&dspi->xfer_done)
> > > call. Other interrupt sources are masked in XSPI mode and should be
> > > irrelevant.
> > 
> > Yes complete(&dspi->xfer_done) is called so CMDTCF is set. For example in
> > one case of underflow I get SPI_SR = 0xca8b0450, which is these flags:
> > 
> >   TCF, TXRXS, TFUF, TFFF, CMDTCF, RFOF, RFDF, CMDFFF
> > 
> > Compared to a successful transfer I get 0xc2830330:
> > 
> >   TCF, TXRXS,       TFFF, CMDTCF,       RFDF, CMDFFF
> 
> Ok, so my new question would be: if CMDTCF is set, presumably it means a
> command was transferred. What command was transferred, and who put data
> in the FIFO for it?
> 
> Because the answer to the above is AFAIU "no one", I guess the driver
> should ignore CMDTCF when TFUF (TX FIFO underflow) is set; I consider
> that to be the logic bug. You are also doing that in patch 4/4, except
> you still call complete() for some reason. If you don't call complete(),
> there is no reason to fend against spurious completions.
> 
> I think I would prefer seeing more deliberate decisions in the driver,
> it helps if things don't just work by coincidence.

After thinking some more, I think I agree with your decision.

If there's a TX FIFO underflow in target mode, presumably there are 2
cases to handle.

1. The underflow occurred in the middle of a large-ish SPI message
   prepared by the driver, where the driver couldn't refill the TX FIFO
   fast enough in dspi_interrupt().

2. The underflow occurred because the driver had absolutely no SPI
   message prepared, and yet the host wanted something.

What changed my mind is that if you don't call complete() on SPI_SR_TFUF
(like I suggested), then case #1 above will hang. Your proposal is to
call complete() anyway, but to discard any previous completions,
associated with case #2, when there's a new message to prepare.

But I would like you to introduce a comment above the earlier
reinit_completion() explaining why it is there.