linux-kernel - Re: [PATCH] spi: fsi: Implement a timeout for polling status

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: <CACPK8Xd42+NgTfS8ERagv-1GkAb8XiY8U71Q8Hz0wQ9dEUJekQ@mail.gmail.com>
Date:   Fri, 18 Mar 2022 04:19:15 +0000
From:   Joel Stanley <joel@....id.au>
To:     Eddie James <eajames@...ux.ibm.com>
Cc:     linux-spi@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH] spi: fsi: Implement a timeout for polling status

On Thu, 17 Mar 2022 at 21:14, Eddie James <eajames@...ux.ibm.com> wrote:
>
> The data transfer routines must poll the status register to
> determine when more data can be shifted in or out. If the hardware
> gets into a bad state, these polling loops may never exit. Prevent
> this by returning an error if a timeout is exceeded.

This makes sense. We may even want to put this code in regardless.

However, I'm wondering why the code in fsi_spi_status didn't catch this.

> static int fsi_spi_status(struct fsi_spi *ctx, u64 *status, const char *dir)
> {
>        int rc = fsi_spi_read_reg(ctx, SPI_FSI_STATUS, status);

You mentioned the error condition is we get back 0xff. That means that
status will be 0xffff_ffff_ffff_ffff ?

Did you observe status being this value?

>        if (rc)
>                return rc;
>
>        if (*status & SPI_FSI_STATUS_ANY_ERROR) {

I think that we're checking against 0xffe0f000.

>                dev_err(ctx->dev, "%s error: %016llx\n", dir, *status);
>
>                rc = fsi_spi_reset(ctx);
>                if (rc)
>                        return rc;

Is the problem here? fsi_spi_reset writes to the clock config
registers, but doesn't read the status.

Obviously doing the writes causes a call to fsi_spi_check_status, but
that checks the FSI2SPI bridge, not the SPI master.

...but it doesn't matter, because we're either going to return an
error from the reset, or return EREMOTEIO, so there's no masking of
the error.

>
>                return -EREMOTEIO;
>        }
>
>        return 0;
> }


>
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
> ---
>  drivers/spi/spi-fsi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/spi/spi-fsi.c b/drivers/spi/spi-fsi.c
> index b6c7467f0b59..d403a7a3021d 100644
> --- a/drivers/spi/spi-fsi.c
> +++ b/drivers/spi/spi-fsi.c
> @@ -25,6 +25,7 @@
>
>  #define SPI_FSI_BASE                   0x70000
>  #define SPI_FSI_INIT_TIMEOUT_MS                1000
> +#define SPI_FSI_STATUS_TIMEOUT_MS      100

Can you add a comment (or put something in the commit message) about
why you chose 100ms.

>  #define SPI_FSI_MAX_RX_SIZE            8
>  #define SPI_FSI_MAX_TX_SIZE            40
>
> @@ -299,6 +300,7 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx,
>                                  struct spi_transfer *transfer)
>  {
>         int rc = 0;
> +       unsigned long end;
>         u64 status = 0ULL;
>
>         if (transfer->tx_buf) {
> @@ -315,10 +317,14 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx,
>                         if (rc)
>                                 return rc;
>
> +                       end = jiffies + msecs_to_jiffies(SPI_FSI_STATUS_TIMEOUT_MS);
>                         do {
>                                 rc = fsi_spi_status(ctx, &status, "TX");
>                                 if (rc)
>                                         return rc;
> +
> +                               if (time_after(jiffies, end))
> +                                       return -ETIMEDOUT;
>                         } while (status & SPI_FSI_STATUS_TDR_FULL);
>
>                         sent += nb;
> @@ -329,10 +335,14 @@ static int fsi_spi_transfer_data(struct fsi_spi *ctx,
>                 u8 *rx = transfer->rx_buf;
>
>                 while (transfer->len > recv) {
> +                       end = jiffies + msecs_to_jiffies(SPI_FSI_STATUS_TIMEOUT_MS);
>                         do {
>                                 rc = fsi_spi_status(ctx, &status, "RX");
>                                 if (rc)
>                                         return rc;
> +
> +                               if (time_after(jiffies, end))
> +                                       return -ETIMEDOUT;
>                         } while (!(status & SPI_FSI_STATUS_RDR_FULL));
>
>                         rc = fsi_spi_read_reg(ctx, SPI_FSI_DATA_RX, &in);
> --
> 2.27.0
>