When building with 'make C=1', sparse reports an endianess bug:
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:60:30: warning: cast removes address space of expression
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
The current code is clearly wrong, as it passes an endian-swapped word
into a register function where it gets swapped again. I assume that this
was simply ported from a non-Linux OS, and the swap was done incorrectly.
Replace it with a cast to uintptr_t.
Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/dma/dw-edma/dw-edma-v0-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
index 97e3fd41c8a8..d670ebcc37b3 100644
--- a/drivers/dma/dw-edma/dw-edma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
@@ -195,7 +195,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
struct dw_edma_v0_lli __iomem *lli;
struct dw_edma_v0_llp __iomem *llp;
u32 control = 0, i = 0;
- u64 sar, dar, addr;
+ uintptr_t sar, dar, addr;
int j;
lli = chunk->ll_region.vaddr;
@@ -214,11 +214,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
/* Transfer size */
SET_LL(&lli[i].transfer_size, child->sz);
/* SAR - low, high */
- sar = cpu_to_le64(child->sar);
+ sar = (uintptr_t)child->sar;
SET_LL(&lli[i].sar_low, lower_32_bits(sar));
SET_LL(&lli[i].sar_high, upper_32_bits(sar));
/* DAR - low, high */
- dar = cpu_to_le64(child->dar);
+ dar = (uintptr_t)child->dar;
SET_LL(&lli[i].dar_low, lower_32_bits(dar));
SET_LL(&lli[i].dar_high, upper_32_bits(dar));
i++;
@@ -232,7 +232,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
/* Channel control */
SET_LL(&llp->control, control);
/* Linked list - low, high */
- addr = cpu_to_le64(chunk->ll_region.paddr);
+ addr = (uintptr_t)chunk->ll_region.paddr;
SET_LL(&llp->llp_low, lower_32_bits(addr));
SET_LL(&llp->llp_high, upper_32_bits(addr));
}
@@ -262,7 +262,7 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
SET_CH(dw, chan->dir, chan->id, ch_control1,
(DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
/* Linked list - low, high */
- llp = cpu_to_le64(chunk->ll_region.paddr);
+ llp = (uintptr_t)chunk->ll_region.paddr;
SET_CH(dw, chan->dir, chan->id, llp_low, lower_32_bits(llp));
SET_CH(dw, chan->dir, chan->id, llp_high, upper_32_bits(llp));
}
--
2.20.0
Hi,
On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <[email protected]> wrote:
> When building with 'make C=1', sparse reports an endianess bug:
I didn't know that option.
>
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:60:30: warning: cast removes address space of expression
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
>
> The current code is clearly wrong, as it passes an endian-swapped word
> into a register function where it gets swapped again. I assume that this
Sorry I didn't catch this, endianness-swapped word into a register
function where it gets swapped again?
Where?
> was simply ported from a non-Linux OS, and the swap was done incorrectly.
> Replace it with a cast to uintptr_t.
>
> Fixes: 7e4b8a4fbe2c ("dmaengine: Add Synopsys eDMA IP version 0 support")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> drivers/dma/dw-edma/dw-edma-v0-core.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index 97e3fd41c8a8..d670ebcc37b3 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> @@ -195,7 +195,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> struct dw_edma_v0_lli __iomem *lli;
> struct dw_edma_v0_llp __iomem *llp;
> u32 control = 0, i = 0;
> - u64 sar, dar, addr;
> + uintptr_t sar, dar, addr;
Will this type assure variables sar, dar and addr are 64 bits?
> int j;
>
> lli = chunk->ll_region.vaddr;
> @@ -214,11 +214,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> /* Transfer size */
> SET_LL(&lli[i].transfer_size, child->sz);
> /* SAR - low, high */
> - sar = cpu_to_le64(child->sar);
> + sar = (uintptr_t)child->sar;
Assuming the host is a big-endian machine and the eDMA on the endpoint
strictly requires the address to be little endian.
By not using cpu_to_le64(), the address to be written on the eDMA will be
in big-endian format, right? If so, that will break the driver.
> SET_LL(&lli[i].sar_low, lower_32_bits(sar));
> SET_LL(&lli[i].sar_high, upper_32_bits(sar));
> /* DAR - low, high */
> - dar = cpu_to_le64(child->dar);
> + dar = (uintptr_t)child->dar;
Ditto.
> SET_LL(&lli[i].dar_low, lower_32_bits(dar));
> SET_LL(&lli[i].dar_high, upper_32_bits(dar));
> i++;
> @@ -232,7 +232,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> /* Channel control */
> SET_LL(&llp->control, control);
> /* Linked list - low, high */
> - addr = cpu_to_le64(chunk->ll_region.paddr);
> + addr = (uintptr_t)chunk->ll_region.paddr;
Ditto.
> SET_LL(&llp->llp_low, lower_32_bits(addr));
> SET_LL(&llp->llp_high, upper_32_bits(addr));
> }
> @@ -262,7 +262,7 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> SET_CH(dw, chan->dir, chan->id, ch_control1,
> (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
> /* Linked list - low, high */
> - llp = cpu_to_le64(chunk->ll_region.paddr);
> + llp = (uintptr_t)chunk->ll_region.paddr;
Ditto.
> SET_CH(dw, chan->dir, chan->id, llp_low, lower_32_bits(llp));
> SET_CH(dw, chan->dir, chan->id, llp_high, upper_32_bits(llp));
> }
> --
> 2.20.0
On Fri, Jun 21, 2019 at 11:43 AM Gustavo Pimentel
<[email protected]> wrote:
>
> Hi,
>
> On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <[email protected]> wrote:
>
> > When building with 'make C=1', sparse reports an endianess bug:
>
> I didn't know that option.
And CF="-D__CHECK_ENDIAN__" is useful.
--
With Best Regards,
Andy Shevchenko
On Fri, Jun 21, 2019 at 10:42 AM Gustavo Pimentel
<[email protected]> wrote:
> On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <[email protected]> wrote:
>
> > When building with 'make C=1', sparse reports an endianess bug:
>
> I didn't know that option.
>
> >
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:60:30: warning: cast removes address space of expression
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> >
> > The current code is clearly wrong, as it passes an endian-swapped word
> > into a register function where it gets swapped again. I assume that this
>
> Sorry I didn't catch this, endianness-swapped word into a register
> function where it gets swapped again?
> Where?
See dw_edma_v0_core_write_chunk()
sar = cpu_to_le64(child->sar);
SET_LL(&lli[i].sar_low, lower_32_bits(sar));
SET_LL(&lli[i].sar_high, upper_32_bits(sar));
SET_LL() expands to writel(), which does a cpu_to_le32() swap.
(the swap gets left out on architectures that are little-endian only)
> > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > index 97e3fd41c8a8..d670ebcc37b3 100644
> > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > @@ -195,7 +195,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > struct dw_edma_v0_lli __iomem *lli;
> > struct dw_edma_v0_llp __iomem *llp;
> > u32 control = 0, i = 0;
> > - u64 sar, dar, addr;
> > + uintptr_t sar, dar, addr;
>
> Will this type assure variables sar, dar and addr are 64 bits?
No, just as long as a pointer. I somehow misread these as
referring to a kernel pointer, but got that part wrong. The
local variables can just be dropped then, just use
lower_32_bits(child->sar)) etc.
> > int j;
> >
> > lli = chunk->ll_region.vaddr;
> > @@ -214,11 +214,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > /* Transfer size */
> > SET_LL(&lli[i].transfer_size, child->sz);
> > /* SAR - low, high */
> > - sar = cpu_to_le64(child->sar);
> > + sar = (uintptr_t)child->sar;
>
> Assuming the host is a big-endian machine and the eDMA on the endpoint
> strictly requires the address to be little endian.
> By not using cpu_to_le64(), the address to be written on the eDMA will be
> in big-endian format, right? If so, that will break the driver.
No, because of the double-swap you are writing a big-endian address
here, which is the bug I am referring to.
Arnd
On Fri, Jun 21, 2019 at 9:51:13, Andy Shevchenko
<[email protected]> wrote:
> On Fri, Jun 21, 2019 at 11:43 AM Gustavo Pimentel
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <[email protected]> wrote:
> >
> > > When building with 'make C=1', sparse reports an endianess bug:
> >
> > I didn't know that option.
>
> And CF="-D__CHECK_ENDIAN__" is useful.
Cool, I will add it to my personal notes.
Thanks.
>
>
> --
> With Best Regards,
> Andy Shevchenko
On Fri, Jun 21, 2019 at 9:55:11, Arnd Bergmann <[email protected]> wrote:
> On Fri, Jun 21, 2019 at 10:42 AM Gustavo Pimentel
> <[email protected]> wrote:
> > On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <[email protected]> wrote:
> >
> > > When building with 'make C=1', sparse reports an endianess bug:
> >
> > I didn't know that option.
> >
> > >
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:60:30: warning: cast removes address space of expression
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: warning: incorrect type in argument 1 (different address spaces)
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: expected void const volatile [noderef] <asn:2>*addr
> > > drivers/dma/dw-edma/dw-edma-v0-debugfs.c:86:24: got void *[assigned] ptr
> > >
> > > The current code is clearly wrong, as it passes an endian-swapped word
> > > into a register function where it gets swapped again. I assume that this
> >
> > Sorry I didn't catch this, endianness-swapped word into a register
> > function where it gets swapped again?
> > Where?
>
> See dw_edma_v0_core_write_chunk()
>
> sar = cpu_to_le64(child->sar);
> SET_LL(&lli[i].sar_low, lower_32_bits(sar));
> SET_LL(&lli[i].sar_high, upper_32_bits(sar));
>
> SET_LL() expands to writel(), which does a cpu_to_le32() swap.
> (the swap gets left out on architectures that are little-endian only)
That's right! I didn't look inside of writel().
>
> > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > index 97e3fd41c8a8..d670ebcc37b3 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > @@ -195,7 +195,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > > struct dw_edma_v0_lli __iomem *lli;
> > > struct dw_edma_v0_llp __iomem *llp;
> > > u32 control = 0, i = 0;
> > > - u64 sar, dar, addr;
> > > + uintptr_t sar, dar, addr;
> >
> > Will this type assure variables sar, dar and addr are 64 bits?
>
> No, just as long as a pointer. I somehow misread these as
> referring to a kernel pointer, but got that part wrong. The
> local variables can just be dropped then, just use
> lower_32_bits(child->sar)) etc.
>
> > > int j;
> > >
> > > lli = chunk->ll_region.vaddr;
> > > @@ -214,11 +214,11 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> > > /* Transfer size */
> > > SET_LL(&lli[i].transfer_size, child->sz);
> > > /* SAR - low, high */
> > > - sar = cpu_to_le64(child->sar);
> > > + sar = (uintptr_t)child->sar;
> >
> > Assuming the host is a big-endian machine and the eDMA on the endpoint
> > strictly requires the address to be little endian.
> > By not using cpu_to_le64(), the address to be written on the eDMA will be
> > in big-endian format, right? If so, that will break the driver.
>
> No, because of the double-swap you are writing a big-endian address
> here, which is the bug I am referring to.
After doing a quick exercise I realize that cpu_to_le64() shouldn't be
there at all like you said. Since I only tested this driver on
little-endian architecture system this issue wasn't visible at all.
>
> Arnd