| 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: <a27378b4-45f7-4f5f-b01f-72eb9b7d2d8f@linaro.org>
Date: Wed, 25 Jun 2025 10:56:25 +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 2/6] spi: spi-fsl-dspi: Store status directly in
cur_msg->status
On 24/06/2025 5:18 pm, Frank Li wrote:
> On Tue, Jun 24, 2025 at 11:35:32AM +0100, James Clark wrote:
>> This will allow us to return a status from the interrupt handler in a
>> later commit and avoids copying it at the end of
>> dspi_transfer_one_message(). For consistency make polling and DMA modes
>> use the same mechanism.
>>
>> Refactor dspi_rxtx() and dspi_poll() to not return -EINPROGRESS because
>> this isn't actually a status that was ever returned to the core layer
>> but some internal state. Wherever that was used we can look at dspi->len
>> instead.
>>
>> No functional changes intended.
>>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>> drivers/spi/spi-fsl-dspi.c | 68 ++++++++++++++++++++++++----------------------
>> 1 file changed, 35 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
>> index 744dfc561db2..feb29bb92a77 100644
>> --- a/drivers/spi/spi-fsl-dspi.c
>> +++ b/drivers/spi/spi-fsl-dspi.c
>> @@ -591,11 +591,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>>
>> static void dspi_setup_accel(struct fsl_dspi *dspi);
>>
>> -static int dspi_dma_xfer(struct fsl_dspi *dspi)
>> +static void dspi_dma_xfer(struct fsl_dspi *dspi)
>> {
>> struct spi_message *message = dspi->cur_msg;
>> struct device *dev = &dspi->pdev->dev;
>> - int ret = 0;
>>
>> /*
>> * dspi->len gets decremented by dspi_pop_tx_pushr in
>> @@ -612,14 +611,12 @@ static int dspi_dma_xfer(struct fsl_dspi *dspi)
>> message->actual_length += dspi->words_in_flight *
>> dspi->oper_word_size;
>>
>> - ret = dspi_next_xfer_dma_submit(dspi);
>> - if (ret) {
>> + message->status = dspi_next_xfer_dma_submit(dspi);
>> + if (message->status) {
>> dev_err(dev, "DMA transfer failed\n");
>> break;
>> }
>> }
>> -
>> - return ret;
>> }
>>
>> static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>> @@ -986,36 +983,40 @@ static void dspi_fifo_write(struct fsl_dspi *dspi)
>> dspi->progress, !dspi->irq);
>> }
>>
>> -static int dspi_rxtx(struct fsl_dspi *dspi)
>> +static void dspi_rxtx(struct fsl_dspi *dspi)
>> {
>> dspi_fifo_read(dspi);
>>
>> if (!dspi->len)
>> /* Success! */
>> - return 0;
>> + return;
>>
>> dspi_fifo_write(dspi);
>> -
>> - return -EINPROGRESS;
>> }
>>
>> -static int dspi_poll(struct fsl_dspi *dspi)
>> +static void dspi_poll(struct fsl_dspi *dspi)
>> {
>> int tries = 1000;
>> u32 spi_sr;
>>
>> - do {
>> - regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> - regmap_write(dspi->regmap, SPI_SR, spi_sr);
>> + while (dspi->len) {
>
> Preivous have not checked dspi->len.
>
> Not sure if it is logical equivalence
>
It used to be this:
status = dspi_poll(dspi);
} while (status == -EINPROGRESS);
Where checking status for -EINPROGRESS is equivalent to checking dspi->len.
>> + do {
>> + regmap_read(dspi->regmap, SPI_SR, &spi_sr);
>> + regmap_write(dspi->regmap, SPI_SR, spi_sr);
>>
>> - if (spi_sr & SPI_SR_CMDTCF)
>> - break;
>> - } while (--tries);
>> + if (spi_sr & SPI_SR_CMDTCF)
>> + break;
>> + } while (--tries);
>>
>> - if (!tries)
>> - return -ETIMEDOUT;
>> + if (!tries) {
>> + dspi->cur_msg->status = -ETIMEDOUT;
>> + return;
>> + }
>>
>> - return dspi_rxtx(dspi);
>> + dspi_rxtx(dspi);
>> + }
>> +
>> + dspi->cur_msg->status = 0;
>> }
>>
>> static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>> @@ -1029,8 +1030,13 @@ static irqreturn_t dspi_interrupt(int irq, void *dev_id)
>> if (!(spi_sr & SPI_SR_CMDTCF))
>> return IRQ_NONE;
>>
>> - if (dspi_rxtx(dspi) == 0)
>> + dspi_rxtx(dspi);
>> +
>> + if (!dspi->len) {
>> + if (dspi->cur_msg)
>> + WRITE_ONCE(dspi->cur_msg->status, 0);
>> complete(&dspi->xfer_done);
>> + }
>>
>> return IRQ_HANDLED;
>> }
>> @@ -1060,7 +1066,6 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>> struct spi_device *spi = message->spi;
>> struct spi_transfer *transfer;
>> bool cs = false;
>> - int status = 0;
>> u32 val = 0;
>> bool cs_change = false;
>>
>> @@ -1120,7 +1125,7 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>> dspi->progress, !dspi->irq);
>>
>> if (dspi->devtype_data->trans_mode == DSPI_DMA_MODE) {
>> - status = dspi_dma_xfer(dspi);
>> + dspi_dma_xfer(dspi);
>> } else {
>> /*
>> * Reset completion counter to clear any extra
>> @@ -1133,15 +1138,12 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>>
>> dspi_fifo_write(dspi);
>>
>> - if (dspi->irq) {
>> + if (dspi->irq)
>> wait_for_completion(&dspi->xfer_done);
>> - } else {
>> - do {
>> - status = dspi_poll(dspi);
>> - } while (status == -EINPROGRESS);
>> - }
>> + else
>> + dspi_poll(dspi);
>> }
>> - if (status)
>> + if (READ_ONCE(message->status))
>
> Why need READ_ONCE()? Does any hardware (DMA) set status?
>
> where update message->status at pio mode.
>
> Frank
Because message->status is set from dspi_interrupt(), so the compiler
wouldn't be aware that this variable can be changed from elsewhere.
Otherwise it could assume that it's never written to in
dspi_transfer_one_message() and not bother to read it again.
>> break;
>>
>> spi_transfer_delay_exec(transfer);
>> @@ -1150,7 +1152,8 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>> dspi_deassert_cs(spi, &cs);
>> }
>>
>> - if (status || !cs_change) {
>> + dspi->cur_msg = NULL;
>> + if (message->status || !cs_change) {
>> /* Put DSPI in stop mode */
>> regmap_update_bits(dspi->regmap, SPI_MCR,
>> SPI_MCR_HALT, SPI_MCR_HALT);
>> @@ -1159,10 +1162,9 @@ static int dspi_transfer_one_message(struct spi_controller *ctlr,
>> ;
>> }
>>
>> - message->status = status;
>> spi_finalize_current_message(ctlr);
>>
>> - return status;
>> + return message->status;
>> }
>>
>> static int dspi_set_mtf(struct fsl_dspi *dspi)
>>
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists