linux-kernel - Re: [PATCH v3 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: <0e26494d-7b85-4874-a2e4-a498ce1864c8@linaro.org>
Date: Wed, 25 Jun 2025 11:09:57 +0100
From: James Clark <james.clark@...aro.org>
To: Frank Li <Frank.li@....com>
Cc: Vladimir Oltean <olteanv@...il.com>, Mark Brown <broonie@...nel.org>,
 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 v3 6/6] spi: spi-fsl-dspi: Report FIFO overflows as errors



On 24/06/2025 5:50 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:36AM +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 because we need to add a
>> register read.
> 
> "We need to add a register read" is not reason.
> 

Maybe: "It's not likely to catch any errors in host mode so optimize by 
avoiding an extra register read"?

> Add checking FIFO error status at target mode in a DMA transfer since PIO
> mode already do it. It help catch some host mode ...
> 

Are you suggesting that we check for FIFO errors in host mode too? It 
requires an extra read and check in dspi_tx_dma_callback(), but I'm not 
sure what it could catch. Realistically as long as everything is setup 
correctly then neither of those flags will be set. It will either always 
work or never work.

It might be better to add it later if a use becomes apparent otherwise 
it's extra noise in the code.

>> In IRQ and polling modes always do it because SPI_SR was
>> already read and it might catch some host mode programming/buffer
>> management errors too.
>>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>>   drivers/spi/spi-fsl-dspi.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 58881911e74a..16a9769f518d 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -560,12 +560,24 @@ static void dspi_rx_dma_callback(void *arg)
>>   	complete(&dma->cmd_rx_complete);
>>   }
>>
>> +static int dspi_fifo_error(struct fsl_dspi *dspi, u32 spi_sr)
>> +{
>> +	if (spi_sr & (SPI_SR_TFUF | SPI_SR_RFOF)) {
>> +		dev_err_ratelimited(&dspi->pdev->dev, "FIFO errors:%s%s\n",
>> +				    spi_sr & SPI_SR_TFUF ? " TX underflow," : "",
>> +				    spi_sr & SPI_SR_RFOF ? " RX overflow," : "");
>> +		return -EIO;
>> +	}
>> +	return 0;
>> +}
>> +
>>   static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>   {
>>   	size_t size = dspi_dma_transfer_size(dspi);
>>   	struct device *dev = &dspi->pdev->dev;
>>   	struct fsl_dspi_dma *dma = dspi->dma;
>>   	int time_left;
>> +	u32 spi_sr;
>>   	int i;
>>
>>   	for (i = 0; i < dspi->words_in_flight; i++)
>> @@ -614,7 +626,8 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>
>>   	if (spi_controller_is_target(dspi->ctlr)) {
>>   		wait_for_completion_interruptible(&dspi->dma->cmd_rx_complete);
>> -		return 0;
>> +		regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> +		return dspi_fifo_error(dspi, spi_sr);
>>   	}
>>
>>   	time_left = wait_for_completion_timeout(&dspi->dma->cmd_tx_complete,
>> @@ -1069,6 +1082,10 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>
>>   			if (spi_sr & SPI_SR_CMDTCF)
>>   				break;
>> +
>> +			dspi->cur_msg->status = dspi_fifo_error(dspi, spi_sr);
>> +			if (dspi->cur_msg->status)
>> +				return;
> 
> 
> Although fifo error may happen after you check, it may reduce some possilbity
> and catch some problems.
> 
> Frak
> 

Not sure what you mean by this one. But I've seen a few small errors now 
that I look again. The error check should be before the transfer 
complete break. And tries should be reset for each part of the message.

>>   		} while (--tries);
>>
>>   		if (!tries) {
>> @@ -1085,6 +1102,7 @@ static void dspi_poll(struct fsl_dspi *dspi)
>>   static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   {
>>   	struct fsl_dspi *dspi = (struct fsl_dspi *)dev_id;
>> +	int status;
>>   	u32 spi_sr;
>>
>>   	regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> @@ -1093,6 +1111,14 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>>   	if (!(spi_sr & SPI_SR_CMDTCF))
>>   		return IRQ_NONE;
>>
>> +	status = dspi_fifo_error(dspi, spi_sr);
>> +	if (status) {
>> +		if (dspi->cur_msg)
>> +			WRITE_ONCE(dspi->cur_msg->status, status);
>> +		complete(&dspi->xfer_done);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>>   	dspi_rxtx(dspi);
>>
>>   	if (!dspi->len) {
>>
>> --
>> 2.34.1
>>