| Message ID | 20230526-spi-imx-endian-v1-1-98d4d4ef4afc@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: imx: correct handling of MXC_CSPIRXDATA value endianness | expand |
Hello Simon, On 26.05.23 16:03, Simon Horman wrote: > The existing code seems be intended to handle MXC_CSPIRXDATA values > which are in big endian. However, it seems that this is only > handled correctly in the case where the host is little endian. > > First, consider the read case. > > u32 val = be32_to_cpu(readl(...)) > > readl() will read a 32bit value and return it after applying le32_to_cpu(). > On a little endian host le32_to_cpu() is a noop. So the raw value is > returned. This is then converted from big endian to host byte-order - > the value is byte-swapped - using be32_to_cpu(). Assuming the raw value > is big endian a host byte-order value is obtained. This seems correct. > > However, on a big endian system, le32_to_cpu() will perform a byte-swap, > while be32_to_cpu() is a noop. Assuming the underlying value is big > endian this is incorrect, because it should not be byte-swapped to > obtain the value in host byte-order - big endian. > > Surveying other kernel code it seems that a correct approach is: > > be32_to_cpu((__force __be32)__raw_readl(...)) How about using ioread32be? > > Here __raw_readl() does returns the raw value, without any calls > that can alter the byte-order. And be32_to_cpu() is called to correctly > either a) swaps the byte-order on a little endian host or b) does not > swap the byte-order on a big endian host. > > Second, let us consider the write case: > > val = cpu_to_be32(val); > ... > writel(val, ...); > > writel() will write the 32bit value, passed as big endian, after applying > cpu_to_le32(). On a little endian system cpu_to_le32() is a noop and > thus the big endian value is stored. This seems correct. > > However, on a big endian system cpu_to_le32() will byte-swap the value. > That is, converting it from big endian to little endian. The little > endian value is then stored. This seems incorrect. > > Surveying other kernel code it seems that a correct approach is: > > __raw_writel((__force u32)cpu_to_be32(val), ...); > > __raw_writel() will write the value with out applying any endian > conversion functions. Thus the big endian value is written. > This seems correct for the case at hand. > > This patch adopts the __raw_readl() and __raw_writel() approaches described > above. It also avoids the following, which stores a big endian value in > a host byte-order variable. > > u32 val; > ... > val = cpu_to_be32(val); > > Reported by Sparse as: > > .../spi-imx.c:410:19: warning: cast to restricted __be32 > .../spi-imx.c:439:21: warning: incorrect type in assignment (different base types) > .../spi-imx.c:439:21: expected unsigned int [addressable] [usertype] val > .../spi-imx.c:439:21: got restricted __be32 [usertype] > > Compile tested only. > > Fixes: 71abd29057cb ("spi: imx: Add support for SPI Slave mode") > Signed-off-by: Simon Horman <horms@kernel.org> > --- > drivers/spi/spi-imx.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index bd6ddb142b13..99c1f76e073d 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -406,7 +406,10 @@ static void spi_imx_buf_tx_swap(struct spi_imx_data *spi_imx) > > static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx) > { > - u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA)); > + u32 val; > + > + val = be32_to_cpu((__force __be32)__raw_readl(spi_imx->base + > + MXC_CSPIRXDATA)); > > if (spi_imx->rx_buf) { > int n_bytes = spi_imx->slave_burst % sizeof(val); > @@ -435,13 +438,13 @@ static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx) > if (spi_imx->tx_buf) { > memcpy(((u8 *)&val) + sizeof(val) - n_bytes, > spi_imx->tx_buf, n_bytes); > - val = cpu_to_be32(val); > spi_imx->tx_buf += n_bytes; > } > > spi_imx->count -= n_bytes; > > - writel(val, spi_imx->base + MXC_CSPITXDATA); > + __raw_writel((__force u32)cpu_to_be32(val), > + spi_imx->base + MXC_CSPITXDATA); > } > > /* MX51 eCSPI */ > > >
On 26.05.23 16:09, Ahmad Fatoum wrote: >> - writel(val, spi_imx->base + MXC_CSPITXDATA); >> + __raw_writel((__force u32)cpu_to_be32(val), >> + spi_imx->base + MXC_CSPITXDATA); >> } On more thing: __raw_writel doesn't involve a write barrier (at least on ARM). That means above code introduces a bug as the CPU may now reorder writes that were sequential before. Both iowrite32be() and readl() have a __iowmb(); on ARM before doing the write itself. >> >> /* MX51 eCSPI */ >> >> >> >
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index bd6ddb142b13..99c1f76e073d 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -406,7 +406,10 @@ static void spi_imx_buf_tx_swap(struct spi_imx_data *spi_imx) static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx) { - u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA)); + u32 val; + + val = be32_to_cpu((__force __be32)__raw_readl(spi_imx->base + + MXC_CSPIRXDATA)); if (spi_imx->rx_buf) { int n_bytes = spi_imx->slave_burst % sizeof(val); @@ -435,13 +438,13 @@ static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx) if (spi_imx->tx_buf) { memcpy(((u8 *)&val) + sizeof(val) - n_bytes, spi_imx->tx_buf, n_bytes); - val = cpu_to_be32(val); spi_imx->tx_buf += n_bytes; } spi_imx->count -= n_bytes; - writel(val, spi_imx->base + MXC_CSPITXDATA); + __raw_writel((__force u32)cpu_to_be32(val), + spi_imx->base + MXC_CSPITXDATA); } /* MX51 eCSPI */
The existing code seems be intended to handle MXC_CSPIRXDATA values which are in big endian. However, it seems that this is only handled correctly in the case where the host is little endian. First, consider the read case. u32 val = be32_to_cpu(readl(...)) readl() will read a 32bit value and return it after applying le32_to_cpu(). On a little endian host le32_to_cpu() is a noop. So the raw value is returned. This is then converted from big endian to host byte-order - the value is byte-swapped - using be32_to_cpu(). Assuming the raw value is big endian a host byte-order value is obtained. This seems correct. However, on a big endian system, le32_to_cpu() will perform a byte-swap, while be32_to_cpu() is a noop. Assuming the underlying value is big endian this is incorrect, because it should not be byte-swapped to obtain the value in host byte-order - big endian. Surveying other kernel code it seems that a correct approach is: be32_to_cpu((__force __be32)__raw_readl(...)) Here __raw_readl() does returns the raw value, without any calls that can alter the byte-order. And be32_to_cpu() is called to correctly either a) swaps the byte-order on a little endian host or b) does not swap the byte-order on a big endian host. Second, let us consider the write case: val = cpu_to_be32(val); ... writel(val, ...); writel() will write the 32bit value, passed as big endian, after applying cpu_to_le32(). On a little endian system cpu_to_le32() is a noop and thus the big endian value is stored. This seems correct. However, on a big endian system cpu_to_le32() will byte-swap the value. That is, converting it from big endian to little endian. The little endian value is then stored. This seems incorrect. Surveying other kernel code it seems that a correct approach is: __raw_writel((__force u32)cpu_to_be32(val), ...); __raw_writel() will write the value with out applying any endian conversion functions. Thus the big endian value is written. This seems correct for the case at hand. This patch adopts the __raw_readl() and __raw_writel() approaches described above. It also avoids the following, which stores a big endian value in a host byte-order variable. u32 val; ... val = cpu_to_be32(val); Reported by Sparse as: .../spi-imx.c:410:19: warning: cast to restricted __be32 .../spi-imx.c:439:21: warning: incorrect type in assignment (different base types) .../spi-imx.c:439:21: expected unsigned int [addressable] [usertype] val .../spi-imx.c:439:21: got restricted __be32 [usertype] Compile tested only. Fixes: 71abd29057cb ("spi: imx: Add support for SPI Slave mode") Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/spi/spi-imx.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)