| 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: <CAPLW+4nOGjfniu+shzO5irmH5bC1E_yD0EZcuDwQJKdfMiDswA@mail.gmail.com>
Date: Thu, 25 Jan 2024 14:03:15 -0600
From: Sam Protsenko <semen.protsenko@...aro.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: broonie@...nel.org, andi.shyti@...nel.org, arnd@...db.de,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
alim.akhtar@...sung.com, linux-spi@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-arch@...r.kernel.org, andre.draszik@...aro.org,
peter.griffin@...aro.org, kernel-team@...roid.com, willmcvicker@...gle.com
Subject: Re: [PATCH v2 10/28] spi: s3c64xx: use full mask for {RX, TX}_FIFO_LVL
On Thu, Jan 25, 2024 at 8:50 AM Tudor Ambarus <tudor.ambarus@...aroorg> wrote:
>
> SPI_STATUSn.{RX, TX}_FIFO_LVL fields show the data level in the RX and
> TX FIFOs. The IP supports FIFOs from 8 to 256 bytes, but apart from the
> MODE_CFG.{RX, TX}_RDY_LVL fields that configure the {RX, TX} FIFO
> trigger level in the interrupt mode, there's nothing in the registers
> that configure the FIFOs depth. Is the responsibility of the SoC that
> integrates the IP to dictate the FIFO depth and of the SPI driver to
> make sure it doesn't bypass the FIFO length.
>
> {RX, TX}_FIFO_LVL was used to pass the FIFO length information based on
> the IP configuration in the SoC. Its value was defined so that it
> includes the entire FIFO length. For example, if one wanted to specify a
> 64 FIFO length (0x40), it wold configure the FIFO level to 127 (0x7f).
s/wodl/would/
> This is not only wrong, because it doesn't respect the IP's register
> fields, it's also misleading. Use the full mask for the
> SPI_STATUSn.{RX, TX}_FIFO_LVL fields. No change in functionality is
> expected.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@...aro.org>
> ---
> drivers/spi/spi-s3c64xx.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index d046810da51f..b048e81e6207 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -78,6 +78,8 @@
> #define S3C64XX_SPI_INT_RX_FIFORDY_EN BIT(1)
> #define S3C64XX_SPI_INT_TX_FIFORDY_EN BIT(0)
>
> +#define S3C64XX_SPI_ST_RX_FIFO_LVL GENMASK(23, 15)
What about s3c* architectures, where RX_LVL starts with bit #13, as
can be seen from .rx_lvl_offset values in corresponding port_configs?
Wouldn't this change break those?
More generally, I don't understand why this patch is needed. Looks
like it just changes the naming of the FIFO level accessing macros,
making the code more bloated too.
> +#define S3C64XX_SPI_ST_TX_FIFO_LVL GENMASK(14, 6)
> #define S3C64XX_SPI_ST_RX_OVERRUN_ERR BIT(5)
> #define S3C64XX_SPI_ST_RX_UNDERRUN_ERR BIT(4)
> #define S3C64XX_SPI_ST_TX_OVERRUN_ERR BIT(3)
> @@ -108,9 +110,6 @@
> #define FIFO_LVL_MASK(i) ((i)->port_conf->fifo_lvl_mask[i->port_id])
> #define S3C64XX_SPI_ST_TX_DONE(v, i) (((v) & \
> (1 << (i)->port_conf->tx_st_done)) ? 1 : 0)
> -#define TX_FIFO_LVL(v, i) (((v) >> 6) & FIFO_LVL_MASK(i))
> -#define RX_FIFO_LVL(v, i) (((v) >> (i)->port_conf->rx_lvl_offset) & \
> - FIFO_LVL_MASK(i))
> #define FIFO_DEPTH(i) ((FIFO_LVL_MASK(i) >> 1) + 1)
>
> #define S3C64XX_SPI_POLLING_SIZE 32
> @@ -219,7 +218,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> loops = msecs_to_loops(1);
> do {
> val = readl(regs + S3C64XX_SPI_STATUS);
> - } while (TX_FIFO_LVL(val, sdd) && loops--);
> + } while (FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, val) && loops--);
>
> if (loops == 0)
> dev_warn(&sdd->pdev->dev, "Timed out flushing TX FIFO\n");
> @@ -228,7 +227,7 @@ static void s3c64xx_flush_fifo(struct s3c64xx_spi_driver_data *sdd)
> loops = msecs_to_loops(1);
> do {
> val = readl(regs + S3C64XX_SPI_STATUS);
> - if (RX_FIFO_LVL(val, sdd))
> + if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, val))
> readl(regs + S3C64XX_SPI_RX_DATA);
> else
> break;
> @@ -499,10 +498,11 @@ static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd,
>
> do {
> status = readl(regs + S3C64XX_SPI_STATUS);
> - } while (RX_FIFO_LVL(status, sdd) < max_fifo && --val);
> + } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < max_fifo &&
> + --val);
>
> /* return the actual received data length */
> - return RX_FIFO_LVL(status, sdd);
> + return FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status);
> }
>
> static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
> @@ -533,7 +533,7 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
> if (val && !xfer->rx_buf) {
> val = msecs_to_loops(10);
> status = readl(regs + S3C64XX_SPI_STATUS);
> - while ((TX_FIFO_LVL(status, sdd)
> + while ((FIELD_GET(S3C64XX_SPI_ST_TX_FIFO_LVL, status)
> || !S3C64XX_SPI_ST_TX_DONE(status, sdd))
> && --val) {
> cpu_relax();
> @@ -568,7 +568,7 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
>
> /* sleep during signal transfer time */
> status = readl(regs + S3C64XX_SPI_STATUS);
> - if (RX_FIFO_LVL(status, sdd) < xfer->len)
> + if (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len)
> usleep_range(time_us / 2, time_us);
>
> if (use_irq) {
> @@ -580,7 +580,8 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd,
> val = msecs_to_loops(ms);
> do {
> status = readl(regs + S3C64XX_SPI_STATUS);
> - } while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
> + } while (FIELD_GET(S3C64XX_SPI_ST_RX_FIFO_LVL, status) < xfer->len &&
> + --val);
>
> if (!val)
> return -EIO;
> --
> 2.43.0.429.g432eaa2c6b-goog
>
Powered by blists - more mailing lists