This patch fixes endian warnings detected by sparse:
- sparse: incorrect type in argument 1 (different base types)
expected unsigned int [unsigned] val
got restricted __le32 [usertype] <noident>
- sparse: incorrect type in argument 1 (different base types)
expected unsigned int [unsigned] val
got restricted __le32 [usertype] <noident>
Signed-off-by: Leilk Liu <[email protected]>
---
drivers/spi/spi-mt65xx.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
index 4676b01..ae645fa 100644
--- a/drivers/spi/spi-mt65xx.c
+++ b/drivers/spi/spi-mt65xx.c
@@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
struct mtk_spi *mdata = spi_master_get_devdata(master);
if (mdata->tx_sgl)
- writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
+ writel((__force u32)cpu_to_le32(xfer->tx_dma),
+ mdata->base + SPI_TX_SRC_REG);
if (mdata->rx_sgl)
- writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
+ writel((__force u32)cpu_to_le32(xfer->rx_dma),
+ mdata->base + SPI_RX_DST_REG);
}
static int mtk_spi_fifo_transfer(struct spi_master *master,
--
1.8.1.1.dirty
Hi,
On Tue, Aug 11, 2015 at 12:43 PM, Leilk Liu <[email protected]> wrote:
> This patch fixes endian warnings detected by sparse:
> - sparse: incorrect type in argument 1 (different base types)
> expected unsigned int [unsigned] val
> got restricted __le32 [usertype] <noident>
> - sparse: incorrect type in argument 1 (different base types)
> expected unsigned int [unsigned] val
> got restricted __le32 [usertype] <noident>
This doesn't "fix" the warning, it only hides the warning and leaves
the actual issue unfixed.
>
> Signed-off-by: Leilk Liu <[email protected]>
> ---
> drivers/spi/spi-mt65xx.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 4676b01..ae645fa 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> struct mtk_spi *mdata = spi_master_get_devdata(master);
>
> if (mdata->tx_sgl)
> - writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> + writel((__force u32)cpu_to_le32(xfer->tx_dma),
> + mdata->base + SPI_TX_SRC_REG);
> if (mdata->rx_sgl)
> - writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> + writel((__force u32)cpu_to_le32(xfer->rx_dma),
> + mdata->base + SPI_RX_DST_REG);
The issue here is that writel already does a cpu_to_le32 conversion,
so the extra cpu_to_le32 calls are actually bogus and need to be
removed. Else it will do a double conversion on big endian systems,
resulting in the data being written in big endian.
Jonas
Hello Mark,
This patch is applied, should I append a new patch or you rollback it?
Thanks.
On Wed, 2015-08-12 at 16:45 +0200, Jonas Gorski wrote:
> Hi,
>
> On Tue, Aug 11, 2015 at 12:43 PM, Leilk Liu <[email protected]> wrote:
> > This patch fixes endian warnings detected by sparse:
> > - sparse: incorrect type in argument 1 (different base types)
> > expected unsigned int [unsigned] val
> > got restricted __le32 [usertype] <noident>
> > - sparse: incorrect type in argument 1 (different base types)
> > expected unsigned int [unsigned] val
> > got restricted __le32 [usertype] <noident>
>
> This doesn't "fix" the warning, it only hides the warning and leaves
> the actual issue unfixed.
>
> >
> > Signed-off-by: Leilk Liu <[email protected]>
> > ---
> > drivers/spi/spi-mt65xx.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> > index 4676b01..ae645fa 100644
> > --- a/drivers/spi/spi-mt65xx.c
> > +++ b/drivers/spi/spi-mt65xx.c
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> > struct mtk_spi *mdata = spi_master_get_devdata(master);
> >
> > if (mdata->tx_sgl)
> > - writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > + writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > + mdata->base + SPI_TX_SRC_REG);
> > if (mdata->rx_sgl)
> > - writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > + writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > + mdata->base + SPI_RX_DST_REG);
>
> The issue here is that writel already does a cpu_to_le32 conversion,
> so the extra cpu_to_le32 calls are actually bogus and need to be
> removed. Else it will do a double conversion on big endian systems,
> resulting in the data being written in big endian.
>
>
> Jonas
On Thu, Aug 13, 2015 at 10:11:59AM +0800, lei liu wrote:
> This patch is applied, should I append a new patch or you rollback it?
> Thanks.
Please send an incremental patch on top of what's applied and don't top
post.
On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> struct mtk_spi *mdata = spi_master_get_devdata(master);
>
> if (mdata->tx_sgl)
> - writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> + writel((__force u32)cpu_to_le32(xfer->tx_dma),
> + mdata->base + SPI_TX_SRC_REG);
> if (mdata->rx_sgl)
> - writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> + writel((__force u32)cpu_to_le32(xfer->rx_dma),
> + mdata->base + SPI_RX_DST_REG);
> }
>
This looks wrong: writel takes a CPU-endian argument, so the value returned
from cpu_to_le32() is not appropriate.
The warning is correct, and you have to remove the cpu_to_le32() conversion
in order to get the driver to behave correctly when the kernel is built
as big-endian.
Arnd
On Saturday 15 August 2015 22:16:03 Arnd Bergmann wrote:
> On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> > struct mtk_spi *mdata = spi_master_get_devdata(master);
> >
> > if (mdata->tx_sgl)
> > - writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > + writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > + mdata->base + SPI_TX_SRC_REG);
> > if (mdata->rx_sgl)
> > - writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > + writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > + mdata->base + SPI_RX_DST_REG);
> > }
> >
>
> This looks wrong: writel takes a CPU-endian argument, so the value returned
> from cpu_to_le32() is not appropriate.
>
> The warning is correct, and you have to remove the cpu_to_le32() conversion
> in order to get the driver to behave correctly when the kernel is built
> as big-endian.
Nevermind, I now saw the issue has already been raised.
Arnd
On Sat, Aug 15, 2015 at 10:16:03PM +0200, Arnd Bergmann wrote:
> On Tuesday 11 August 2015 18:43:09 Leilk Liu wrote:
> > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master *master,
> > struct mtk_spi *mdata = spi_master_get_devdata(master);
> >
> > if (mdata->tx_sgl)
> > - writel(cpu_to_le32(xfer->tx_dma), mdata->base + SPI_TX_SRC_REG);
> > + writel((__force u32)cpu_to_le32(xfer->tx_dma),
> > + mdata->base + SPI_TX_SRC_REG);
> > if (mdata->rx_sgl)
> > - writel(cpu_to_le32(xfer->rx_dma), mdata->base + SPI_RX_DST_REG);
> > + writel((__force u32)cpu_to_le32(xfer->rx_dma),
> > + mdata->base + SPI_RX_DST_REG);
> > }
> >
>
> This looks wrong: writel takes a CPU-endian argument, so the value returned
> from cpu_to_le32() is not appropriate.
>
> The warning is correct, and you have to remove the cpu_to_le32() conversion
> in order to get the driver to behave correctly when the kernel is built
> as big-endian.
Indeed, it's about time people started thinking more about the warnings
and why we have coded things in the way we have.
Look people. cpu_to_le32() takes a value in the CPU endian, and
converts it to a little endian 32-bit number. See, the clue is in
the name.
All writel() implementations take a CPU number and write it in little
endian format. Hence, writel() almost always uses cpu_to_le32()
internally.
Now think about what you're saying with "cpu_to_le32(cpu_to_le32())".
It's utter rubbish, total crap. It's wrong no matter which way you
look at it.
We have le32_to_cpu() which does what it says on the tin. Same with
be32_to_cpu() and cpu_to_be32().
Don't hack around this stuff with __force. If you're having to use
__force to get rid of a warning here, you _ARE_ doing something wrong,
no questions about that. __force in driver code is a definite sign
that you are doing something wrong. Don't do it. Ask the question
if you think you need it.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
On Sun, Aug 16, 2015 at 03:06:39PM +0100, Russell King - ARM Linux wrote:
> All writel() implementations take a CPU number and write it in little
> endian format. Hence, writel() almost always uses cpu_to_le32()
> internally.
I think the reason this keeps coming up is that people aren't expecting
writel() to be doing endianness conversion, it's easy to get caught out
by it.