[PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

When the size of a transfer exceeds the size of the FIFO (32 bytes), RX
overflows will be generated and receive data will be corrupted and
warnings will be produced. For example, here's an error generated by a
transfer of 36 bytes:

  spi_master spi0: mchp_corespi_interrupt: RX OVERFLOW: rxlen: 4, txlen: 0

I am not entirely sure how this happens, as rxlen being 4 means that 32
of 36 bytes have been read from the RX FIFO so there should be
sufficient room for 4 more bytes but timing is likely a factor as simply
adding a delay in the transmit path is enough to avoid the overflows.

CC: stable@vger.kernel.org
Fixes: 9ac8d17694b6 ("spi: add support for microchip fpga spi controllers")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Been sitting on this one for a bit, original reporter claims the problem
isn't fixed, but it fixed the issue on my setup so I am sending the patch
as it's an improvement on the status quo at the very least.

CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Mark Brown <broonie@kernel.org>
CC: linux-spi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/spi/spi-microchip-core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c
index 5b6af55855ef..3582fe8d3fc4 100644
--- a/drivers/spi/spi-microchip-core.c
+++ b/drivers/spi/spi-microchip-core.c
@@ -221,6 +221,13 @@ static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi)
 	while ((i < fifo_max) && !(mchp_corespi_read(spi, REG_STATUS) & STATUS_TXFIFO_FULL)) {
 		u32 word;
 
+		/*
+		 * If the transfer is larger than FIFO_DEPTH, spin until space
+		 * is made in the RX FIFO to avoid losing data to RX overflows
+		 */
+		while (mchp_corespi_read(spi, REG_STATUS) & STATUS_RXFIFO_FULL)
+			;
+
 		if (spi->n_bytes == 4)
 			word = spi->tx_buf ? *((u32 *)spi->tx_buf) : 0xaa;
 		else if (spi->n_bytes == 2)
-- 
2.45.2

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

On Tue, Jan 14, 2025 at 05:13:49PM +0000, Conor Dooley wrote:

> When the size of a transfer exceeds the size of the FIFO (32 bytes), RX
> overflows will be generated and receive data will be corrupted and
> warnings will be produced. For example, here's an error generated by a
> transfer of 36 bytes:

>   spi_master spi0: mchp_corespi_interrupt: RX OVERFLOW: rxlen: 4, txlen: 0

> I am not entirely sure how this happens, as rxlen being 4 means that 32
> of 36 bytes have been read from the RX FIFO so there should be
> sufficient room for 4 more bytes but timing is likely a factor as simply
> adding a delay in the transmit path is enough to avoid the overflows.

The reads from the FIFO happen prior to the check for overflow in the
interrupt handler so if we overflow then take the interrupt we will read
the full 32 byte FIFO before it sees that there was an overflow.

> @@ -221,6 +221,13 @@ static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi)
>  	while ((i < fifo_max) && !(mchp_corespi_read(spi, REG_STATUS) & STATUS_TXFIFO_FULL)) {
>  		u32 word;
>  
> +		/*
> +		 * If the transfer is larger than FIFO_DEPTH, spin until space
> +		 * is made in the RX FIFO to avoid losing data to RX overflows
> +		 */
> +		while (mchp_corespi_read(spi, REG_STATUS) & STATUS_RXFIFO_FULL)
> +			;
> +

So, this is the transmit side but we're polling the RX FIFO and not
doing anything to clear it?  I see that the FIFO reads are driven from
interrupt context.  If I had to guess I'd say that there's latency in
the interrupt being delivered (possibly to a different CPU) and when the
transfer is being driven by the TX side it's stuffing data into the TX
FIFO faster than interrupts are being delivered (the TX side just seems
to busy wait on there being space in the FIFO which can't be good for
slower speeds...) so the TX and RX sides of the transfer get out of sync.

Given that AFAICT the controller has to RX all the time I suspect you
want to move the RX processing out of interrupt context into the main
_transfer_one() function and busy wait for that too, or push the TX side
into the interrupt handler (which at first glance looks simpler).
Either way the two directions will be driven from the same place and so
not get out of sync.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]

On Tue, Jan 14, 2025 at 07:44:26PM +0000, Mark Brown wrote:
> On Tue, Jan 14, 2025 at 05:13:49PM +0000, Conor Dooley wrote:
> 
> > When the size of a transfer exceeds the size of the FIFO (32 bytes), RX
> > overflows will be generated and receive data will be corrupted and
> > warnings will be produced. For example, here's an error generated by a
> > transfer of 36 bytes:
> 
> >   spi_master spi0: mchp_corespi_interrupt: RX OVERFLOW: rxlen: 4, txlen: 0
> 
> > I am not entirely sure how this happens, as rxlen being 4 means that 32
> > of 36 bytes have been read from the RX FIFO so there should be
> > sufficient room for 4 more bytes but timing is likely a factor as simply
> > adding a delay in the transmit path is enough to avoid the overflows.
> 
> The reads from the FIFO happen prior to the check for overflow in the
> interrupt handler so if we overflow then take the interrupt we will read
> the full 32 byte FIFO before it sees that there was an overflow.
> 
> > @@ -221,6 +221,13 @@ static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi)
> >  	while ((i < fifo_max) && !(mchp_corespi_read(spi, REG_STATUS) & STATUS_TXFIFO_FULL)) {
> >  		u32 word;
> >  
> > +		/*
> > +		 * If the transfer is larger than FIFO_DEPTH, spin until space
> > +		 * is made in the RX FIFO to avoid losing data to RX overflows
> > +		 */
> > +		while (mchp_corespi_read(spi, REG_STATUS) & STATUS_RXFIFO_FULL)
> > +			;
> > +
> 
> So, this is the transmit side but we're polling the RX FIFO and not
> doing anything to clear it?  I see that the FIFO reads are driven from
> interrupt context.  If I had to guess I'd say that there's latency in
> the interrupt being delivered (possibly to a different CPU) and when the
> transfer is being driven by the TX side it's stuffing data into the TX
> FIFO faster than interrupts are being delivered (the TX side just seems
> to busy wait on there being space in the FIFO which can't be good for
> slower speeds...) so the TX and RX sides of the transfer get out of sync.
> 
> Given that AFAICT the controller has to RX all the time I suspect you
> want to move the RX processing out of interrupt context into the main
> _transfer_one() function and busy wait for that too, or push the TX side
> into the interrupt handler (which at first glance looks simpler).
> Either way the two directions will be driven from the same place and so
> not get out of sync.

Hmm, at first glance it is indeed simpler, but it may be a bit of a
poor compromise. There used to be calls to
mchp_corespi_write_fifo() in the interrupt handler, but I removed them
in the ?summer? because messages were being sent out of order. IIRC what
happens is that the tx fifo can be emptied by the controller hardware
before the whole message has been written into it, thereby generating
the interrupts and triggering a parallel call to mchp_corespi_write_fifo().

To avoid that, the first byte could be sent by transfer_one(), and then
everything else handled by the current implementation of
mchp_corespi_write_fifo(), but I think that would cause "actual" RX
overflows, without cutting down to sending (FIFO_MAX - 1) bytes per call
or ensuring that the rx fifo is read before the tx one. Given the timing
issues there seem to be around the rx ready/tx done interrupts and the
rx/tx fifo status bits being set/cleared, I don't want to play games
there. That means cutting down to something well under FIFO_MAX (which
is 32 bytes), probably to something like 1/2/4 bytes depending on
transfer size since that can be done in a single write on the bus and
will allow the same code to be used in transfer_one() and in the
interrupt handler.

Moving it out of the interrupt handler entirely probably makes the code
a lot easier to reason about, so I think I'd actually rather do that.
That said, I don't think I would want that change going into fixes as it's
a pretty intrusive change, although given where we are in the cycle it
ain't as much of a problem since I'd not be sending it until after 6.13
is released anyway.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Wed, Jan 15, 2025 at 06:22:55PM +0000, Conor Dooley wrote:

> Moving it out of the interrupt handler entirely probably makes the code
> a lot easier to reason about, so I think I'd actually rather do that.
> That said, I don't think I would want that change going into fixes as it's
> a pretty intrusive change, although given where we are in the cycle it
> ain't as much of a problem since I'd not be sending it until after 6.13
> is released anyway.

OTOH the patch you sent will lock up hard if the RX FIFO gets stuck for
some other reason which is always a bit concerning...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Mukesh Kumar Savaliya]

On 1/14/2025 10:43 PM, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
Not sure why this "From:" is coming here, should be not coming.
> 
> When the size of a transfer exceeds the size of the FIFO (32 bytes), RX
> overflows will be generated and receive data will be corrupted and
> warnings will be produced. For example, here's an error generated by a
> transfer of 36 bytes:
> 
why warning on RX generated when TZ size is > 32 bytes ?
Also you should be taking 32 bytes in a one go and then send remaining 4 
bytes. I am thinking if your TX is faster than the RX receiving ?
>    spi_master spi0: mchp_corespi_interrupt: RX OVERFLOW: rxlen: 4, txlen: 0
> 
> I am not entirely sure how this happens, as rxlen being 4 means that 32
> of 36 bytes have been read from the RX FIFO so there should be
> sufficient room for 4 more bytes but timing is likely a factor as simply
> adding a delay in the transmit path is enough to avoid the overflows.
I suggest to try to chunk in 32 bytes size.
> 
> CC: stable@vger.kernel.org
> Fixes: 9ac8d17694b6 ("spi: add support for microchip fpga spi controllers")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> Been sitting on this one for a bit, original reporter claims the problem
> isn't fixed, but it fixed the issue on my setup so I am sending the patch
> as it's an improvement on the status quo at the very least.
> 
> CC: Conor Dooley <conor.dooley@microchip.com>
> CC: Daire McNamara <daire.mcnamara@microchip.com>
> CC: Mark Brown <broonie@kernel.org>
> CC: linux-spi@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> ---
>   drivers/spi/spi-microchip-core.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/spi/spi-microchip-core.c b/drivers/spi/spi-microchip-core.c
> index 5b6af55855ef..3582fe8d3fc4 100644
> --- a/drivers/spi/spi-microchip-core.c
> +++ b/drivers/spi/spi-microchip-core.c
> @@ -221,6 +221,13 @@ static inline void mchp_corespi_write_fifo(struct mchp_corespi *spi)
>   	while ((i < fifo_max) && !(mchp_corespi_read(spi, REG_STATUS) & STATUS_TXFIFO_FULL)) {
>   		u32 word;
>   
> +		/*
> +		 * If the transfer is larger than FIFO_DEPTH, spin until space
> +		 * is made in the RX FIFO to avoid losing data to RX overflows
> +		 */
what if you transfer first 32 bytes and then remaining ?
> +		while (mchp_corespi_read(spi, REG_STATUS) & STATUS_RXFIFO_FULL)
> +			;
> +
>   		if (spi->n_bytes == 4)
>   			word = spi->tx_buf ? *((u32 *)spi->tx_buf) : 0xaa;
>   		else if (spi->n_bytes == 2)

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On Wed, Jan 29, 2025 at 06:57:34PM +0530, Mukesh Kumar Savaliya wrote:
> On 1/14/2025 10:43 PM, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> Not sure why this "From:" is coming here, should be not coming.

It is there because the sending address was not the author address. It's
fairly normal and required to set the correct address in git were Mark
to apply the patch.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] spi: microchip-core: prevent RX overflows when transmit size > FIFO size

[Mark Brown]

On Tue, 14 Jan 2025 17:13:49 +0000, Conor Dooley wrote:
> When the size of a transfer exceeds the size of the FIFO (32 bytes), RX
> overflows will be generated and receive data will be corrupted and
> warnings will be produced. For example, here's an error generated by a
> transfer of 36 bytes:
> 
>   spi_master spi0: mchp_corespi_interrupt: RX OVERFLOW: rxlen: 4, txlen: 0
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: microchip-core: prevent RX overflows when transmit size > FIFO size
      commit: 91cf42c63f2d8a9c1bcdfe923218e079b32e1a69

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark