dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] dmaengine: dw-edma: fix endianess confusion
@ 2019-06-17 13:17 Arnd Bergmann
  2019-06-21  8:42 ` Gustavo Pimentel
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-06-17 13:17 UTC (permalink / raw)
  To: Gustavo Pimentel, Vinod Koul
  Cc: Arnd Bergmann, Dan Williams, Andy Shevchenko, Russell King,
	Joao Pinto, dmaengine, linux-kernel

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 <arnd@arndb.de>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* RE: [PATCH] dmaengine: dw-edma: fix endianess confusion
  2019-06-17 13:17 [PATCH] dmaengine: dw-edma: fix endianess confusion Arnd Bergmann
@ 2019-06-21  8:42 ` Gustavo Pimentel
  2019-06-21  8:51   ` Andy Shevchenko
  2019-06-21  8:55   ` Arnd Bergmann
  0 siblings, 2 replies; 6+ messages in thread
From: Gustavo Pimentel @ 2019-06-21  8:42 UTC (permalink / raw)
  To: Arnd Bergmann, Gustavo Pimentel, Vinod Koul
  Cc: Dan Williams, Andy Shevchenko, Russell King, Joao Pinto,
	dmaengine, linux-kernel

Hi,

On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>
> ---
>  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



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: dw-edma: fix endianess confusion
  2019-06-21  8:42 ` Gustavo Pimentel
@ 2019-06-21  8:51   ` Andy Shevchenko
  2019-06-21  8:56     ` Gustavo Pimentel
  2019-06-21  8:55   ` Arnd Bergmann
  1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2019-06-21  8:51 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: Arnd Bergmann, Vinod Koul, Dan Williams, Andy Shevchenko,
	Russell King, Joao Pinto, dmaengine, linux-kernel

On Fri, Jun 21, 2019 at 11:43 AM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> Hi,
>
> On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@arndb.de> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] dmaengine: dw-edma: fix endianess confusion
  2019-06-21  8:42 ` Gustavo Pimentel
  2019-06-21  8:51   ` Andy Shevchenko
@ 2019-06-21  8:55   ` Arnd Bergmann
  2019-06-21 10:44     ` Gustavo Pimentel
  1 sibling, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2019-06-21  8:55 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: Vinod Koul, Dan Williams, Andy Shevchenko, Russell King,
	Joao Pinto, dmaengine, linux-kernel

On Fri, Jun 21, 2019 at 10:42 AM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
> On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@arndb.de> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH] dmaengine: dw-edma: fix endianess confusion
  2019-06-21  8:51   ` Andy Shevchenko
@ 2019-06-21  8:56     ` Gustavo Pimentel
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo Pimentel @ 2019-06-21  8:56 UTC (permalink / raw)
  To: Andy Shevchenko, Gustavo Pimentel
  Cc: Arnd Bergmann, Vinod Koul, Dan Williams, Andy Shevchenko,
	Russell King, Joao Pinto, dmaengine, linux-kernel

On Fri, Jun 21, 2019 at 9:51:13, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 21, 2019 at 11:43 AM Gustavo Pimentel
> <Gustavo.Pimentel@synopsys.com> wrote:
> >
> > Hi,
> >
> > On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@arndb.de> 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



^ permalink raw reply	[flat|nested] 6+ messages in thread
* RE: [PATCH] dmaengine: dw-edma: fix endianess confusion
  2019-06-21  8:55   ` Arnd Bergmann
@ 2019-06-21 10:44     ` Gustavo Pimentel
  0 siblings, 0 replies; 6+ messages in thread
From: Gustavo Pimentel @ 2019-06-21 10:44 UTC (permalink / raw)
  To: Arnd Bergmann, Gustavo Pimentel
  Cc: Vinod Koul, Dan Williams, Andy Shevchenko, Russell King,
	Joao Pinto, dmaengine, linux-kernel

On Fri, Jun 21, 2019 at 9:55:11, Arnd Bergmann <arnd@arndb.de> wrote:

> On Fri, Jun 21, 2019 at 10:42 AM Gustavo Pimentel
> <Gustavo.Pimentel@synopsys.com> wrote:
> > On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann <arnd@arndb.de> 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



^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-21 10:44 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-17 13:17 [PATCH] dmaengine: dw-edma: fix endianess confusion Arnd Bergmann
2019-06-21  8:42 ` Gustavo Pimentel
2019-06-21  8:51   ` Andy Shevchenko
2019-06-21  8:56     ` Gustavo Pimentel
2019-06-21  8:55   ` Arnd Bergmann
2019-06-21 10:44     ` Gustavo Pimentel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).