RE: [PATCH] dmaengine: dw-edma: fix endianess confusion
From: Gustavo Pimentel <hidden>
Date: 2019-06-21 08:43:52
Also in:
lkml
Hi, On Mon, Jun 17, 2019 at 14:17:47, Arnd Bergmann [off-list ref] 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?
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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