| 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
| ||
|
Message-ID: <19302e67-7df1-4100-912d-c9b426a4b943@linaro.org>
Date: Fri, 27 Jun 2025 09:52:16 +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 25/06/2025 3:55 pm, Frank Li wrote:
> On Wed, Jun 25, 2025 at 11:09:57AM +0100, James Clark wrote:
>>
>>
>> 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.
>
> I think your origial last phrase is not good enough. You may rephrase it
> to make it clear.
>
> for example: according to your patch
>
> "Only do this for target mode in a DMA transfer because we need to add a register read."
>
> "add a register read" is result, not a reason. the reason should be "you want
> host side capture such error."
>
> Frank
>
>
Got it, thanks
>>
>>>> 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
>>>>
>>
Powered by blists - more mailing lists