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: <aFrXRDJmMgt0qTlL@lizhi-Precision-Tower-5810>
Date: Tue, 24 Jun 2025 12:50:12 -0400
From: Frank Li <Frank.li@....com>
To: James Clark <james.clark@...aro.org>
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 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.

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

> 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

>  		} 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
>