2011-02-12 10:34:28

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 16/19] ARM: LPAE: Use generic dma_addr_t type definition

On Mon, Jan 24, 2011 at 05:55:58PM +0000, Catalin Marinas wrote:
> From: Will Deacon <[email protected]>
>
> This patch uses the types.h implementation in asm-generic to define the
> dma_addr_t type as the same width as phys_addr_t.
>
> NOTE: this is a temporary patch until the corresponding patches unifying
> the dma_addr_t and removing the dma64_addr_t are merged into mainline.

I'm not too sure about this patch. All of the DMA devices we have only
take 32-bit addresses for their DMA, so making dma_addr_t 64-bit seems
wrong as we'll implicitly truncate these addresses.

As ARM platforms don't (sanely) support DMA, I think dropping this patch
for the time being would be a good idea, and stick with 32-bit dma_addr_t,
especially as we need to first do a sweep for dma_addr_t usage in device
driver structures (such as dma engine scatter lists.) These really should
use __le32/__be32/u32 depending on whether they're little endian, big
endian or native endian.


2011-02-14 13:01:39

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH v4 16/19] ARM: LPAE: Use generic dma_addr_t type definition

On Sat, 2011-02-12 at 10:34 +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 24, 2011 at 05:55:58PM +0000, Catalin Marinas wrote:
> > From: Will Deacon <[email protected]>
> >
> > This patch uses the types.h implementation in asm-generic to define the
> > dma_addr_t type as the same width as phys_addr_t.
> >
> > NOTE: this is a temporary patch until the corresponding patches unifying
> > the dma_addr_t and removing the dma64_addr_t are merged into mainline.
>
> I'm not too sure about this patch. All of the DMA devices we have only
> take 32-bit addresses for their DMA, so making dma_addr_t 64-bit seems
> wrong as we'll implicitly truncate these addresses.

If we don't enable LPAE, the dma_addr_t is 32-bit, so existing platforms
are not affected. With Cortex-A15, new platforms may have PCIe and be
able to access memory beyond 32-bit (if they don't support >32-bit DMA
at least for some critical devices, I'm not sure why they would use
A15).

For things like hard drives for example, it becomes problematic as pages
are allocated by the VFS layer from highmem and passed to the driver for
DMA. If we keep dma_addr_t to 32-bit you would need to use DMA bounce
even if the PCIe device supports >32-bit physical addresses.

> As ARM platforms don't (sanely) support DMA, I think dropping this patch
> for the time being would be a good idea, and stick with 32-bit dma_addr_t,
> especially as we need to first do a sweep for dma_addr_t usage in device
> driver structures (such as dma engine scatter lists.) These really should
> use __le32/__be32/u32 depending on whether they're little endian, big
> endian or native endian.

Maybe we could make the dma_addr_t size configurable (and disabled by
default) since I expect there'll be platforms capable of >32-bit DMA.

--
Catalin

2011-02-15 14:30:07

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH v4 16/19] ARM: LPAE: Use generic dma_addr_t type definition

On Mon, Feb 14, 2011 at 01:01:30PM +0000, Catalin Marinas wrote:
> Maybe we could make the dma_addr_t size configurable (and disabled by
> default) since I expect there'll be platforms capable of >32-bit DMA.

It would be far better to fix the dma_addr_t abuses. I've already fixed
those in the pl08x driver:

struct lli {
dma_addr_t src;
dma_addr_t dst;
dma_addr_t next;
u32 cctl;
};

became:

struct pl08x_lli {
u32 src;
u32 dst;
u32 lli;
u32 cctl;
};

and similar needs to be done elsewhere in ARM specific drivers.
dma_addr_t has no business being in structures that describe data which
hardware accesses.

2011-02-15 15:24:17

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH v4 16/19] ARM: LPAE: Use generic dma_addr_t type definition

On Tue, 2011-02-15 at 14:27 +0000, Russell King - ARM Linux wrote:
> On Mon, Feb 14, 2011 at 01:01:30PM +0000, Catalin Marinas wrote:
> > Maybe we could make the dma_addr_t size configurable (and disabled by
> > default) since I expect there'll be platforms capable of >32-bit DMA.
>
> It would be far better to fix the dma_addr_t abuses.

That's not a simple task, I have no idea how many drivers get used on
ARM systems. We can defer this until people start using Cortex-A15 in
real hardware and only fix the drivers they need.

BTW, can sparse help here (I haven't used it much)?

--
Catalin