| 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: <aFrP1ZNvjHCPmbWG@lizhi-Precision-Tower-5810>
Date: Tue, 24 Jun 2025 12:18:29 -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 2/6] spi: spi-fsl-dspi: Store status directly in
cur_msg->status
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
> + 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
> 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