Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Andy Shevchenko]

On Wed, Nov 18, 2015 at 2:38 PM, Arnd Bergmann  wrote:
> On Wednesday 18 November 2015 11:35:27 Andy Shevchenko wrote:
>> On Fri, Nov 13, 2015 at 11:35 AM, Arnd Bergmann  wrote:
>> > On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
>> >> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:

[]

>> >> If dst_addr is dma_addr_t wouldn't be a problem when
>> >> resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?
>> >>
>> >> Btw, for me casting to dma_addr_t looks sane.
>> >
>> > The background here is that the address comes from a resource_size_t
>> > that describes the MMIO register area as seen from the CPU, and that
>> > is normally a phys_addr_t (resource_size_t is defined as being long
>> > enough to store a phys_addr_t or various other things depending on
>> > resource->flags).
>> >
>> > dma_addr_t strictly speaking refers to a RAM location as seen by a
>> > DMA master, and that only comes out of dma_map_*() or
>> > dma_alloc_coherent().
>> >
>> > The DMA engine wants something else here, which is an MMIO register
>> > address as seen by a DMA master, and we don't have a separate typedef
>> > for that. Almost universally all of resource_size_t, phys_addr_t and
>> > dma_addr_t are the same type, and if we ever get a platform that
>> > wants something other than a phys_addr_t to put into cfg.dst_addr,
>> > we are in deep trouble.
>>
>> DMA operates with address space covered by dma_addr_t, if you use
>> phys_addr_t you may get address out of DMA boundaries. This is should
>> be done in hardware / firmware / platform representation.
>> So, I don't see any reason not to use dma_addr_t here.
>
> As I said above, this isn't really the same as DMA: all normal
> dma_addr_t are returned from dma_alloc_* or dma_map_*, point
> to RAM and might go trhough an IOMMU, all of which is not true
> here, hence the patch to change the type to phys_addr_t.
>
> You really can't get out of bounds because the data comes from a
> phys_addr_t and refers to a fixed location in hardware. If a
> platform has registers higher than a 32-bit address, its phys_addr_t
> must be 64-bit, but its dma_addr_t not necessarily so (even though
> the two are the same almost always in practice).

I understand most of the things here, what I don't is how a platform
is supposed to work if you have the following:
a) HW, that uses register space let's say higher than 32-bit;
b) DMA engine, which should provide a DMA capability for above HW block;
c) dma_addr_t which does not cover the HW register space.

For me it clearly looks like a platform (HW / SW) configuration issue.

In case of bounce buffers I can't understand how it helps there.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Arnd Bergmann]

On Wednesday 18 November 2015 17:29:19 Andy Shevchenko wrote:
> 
> I understand most of the things here, what I don't is how a platform
> is supposed to work if you have the following:
> a) HW, that uses register space let's say higher than 32-bit;
> b) DMA engine, which should provide a DMA capability for above HW block;
> c) dma_addr_t which does not cover the HW register space.

On this platform, the current code is obviously broken, because the pointer
is 32-bit wide and cannot reach the registers. I assume you agree on that
part.

With my patch, the 64-bit resource_size_t in dw_mci helps get the
correct FIFO address to this line:

cfg.dst_addr = host->phy_regs + fifo_offset;

There, it remains broken because of the dma_addr_t being too short, and
we also need Linus' patch from https://lkml.org/lkml/2013/4/26/120 in
addition to mine.


> For me it clearly looks like a platform (HW / SW) configuration issue.

I think some people have argued in the past that we should always use
the same type for dma_addr_t, resource_size_t and phys_addr_t. That
would certainly fix the problem you describe as well. In practice,
everyone has that already, and my patch by itself fixes all the
cases where the FIFO is at a high address and dma_addr_t is already
64-bit wide.

> In case of bounce buffers I can't understand how it helps there.

Right, bounce buffers are irrelevant here, because the FIFO address
is never translated and never bounced.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Andy Shevchenko]

On Wed, Nov 18, 2015 at 5:45 PM, Arnd Bergmann  wrote:
> On Wednesday 18 November 2015 17:29:19 Andy Shevchenko wrote:
>>
>> I understand most of the things here, what I don't is how a platform
>> is supposed to work if you have the following:
>> a) HW, that uses register space let's say higher than 32-bit;
>> b) DMA engine, which should provide a DMA capability for above HW block;
>> c) dma_addr_t which does not cover the HW register space.
>
> On this platform, the current code is obviously broken, because the pointer
> is 32-bit wide and cannot reach the registers. I assume you agree on that
> part.

Yes.

> With my patch, the 64-bit resource_size_t in dw_mci helps get the
> correct FIFO address to this line:
>
> cfg.dst_addr = host->phy_regs + fifo_offset;
>
> There, it remains broken because of the dma_addr_t being too short, and
> we also need Linus' patch from https://lkml.org/lkml/2013/4/26/120 in
> addition to mine.

Wow, never applied.

>
>
>> For me it clearly looks like a platform (HW / SW) configuration issue.
>
> I think some people have argued in the past that we should always use
> the same type for dma_addr_t, resource_size_t and phys_addr_t. That
> would certainly fix the problem you describe as well. In practice,
> everyone has that already, and my patch by itself fixes all the
> cases where the FIFO is at a high address and dma_addr_t is already
> 64-bit wide.

Let me summarize.

We have to have classification by address space
1) physical
2) virtual

Therefore
resource_addr_t must be equal to phys_addr_t since it may carry any
possible physical address.

dma_addr_t is a physical address wrt DMA mask.

Correct?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Arnd Bergmann]

On Wednesday 18 November 2015 18:17:32 Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 5:45 PM, Arnd Bergmann  wrote:
> > On Wednesday 18 November 2015 17:29:19 Andy Shevchenko wrote:

> >> For me it clearly looks like a platform (HW / SW) configuration issue.
> >
> > I think some people have argued in the past that we should always use
> > the same type for dma_addr_t, resource_size_t and phys_addr_t. That
> > would certainly fix the problem you describe as well. In practice,
> > everyone has that already, and my patch by itself fixes all the
> > cases where the FIFO is at a high address and dma_addr_t is already
> > 64-bit wide.
> 
> Let me summarize.
> 
> We have to have classification by address space
> 1) physical
> 2) virtual

That classification is oversimplified, as the DMA address space
is often not the same as physical.

> Therefore
> resource_addr_t must be equal to phys_addr_t since it may carry any
> possible physical address.

Right, in theory it can also be larger than phys_addr_t, but there is no
need for that.

> dma_addr_t is a physical address wrt DMA mask.
> 
> Correct?

dma_addr_t is how a device sees RAM, it is limited by the DMA mask
that is a subset of the capabilities of the device and the buses
through which it is connected, and is subject to IOMMU translation
and possible platform or bus specific offsets from the physical
memory. I still don't know where you're getting with this.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Andy Shevchenko]

On Wed, Nov 18, 2015 at 6:22 PM, Arnd Bergmann  wrote:
> On Wednesday 18 November 2015 18:17:32 Andy Shevchenko wrote:
>> On Wed, Nov 18, 2015 at 5:45 PM, Arnd Bergmann  wrote:
>> > On Wednesday 18 November 2015 17:29:19 Andy Shevchenko wrote:
>
>> >> For me it clearly looks like a platform (HW / SW) configuration issue.
>> >
>> > I think some people have argued in the past that we should always use
>> > the same type for dma_addr_t, resource_size_t and phys_addr_t. That
>> > would certainly fix the problem you describe as well. In practice,
>> > everyone has that already, and my patch by itself fixes all the
>> > cases where the FIFO is at a high address and dma_addr_t is already
>> > 64-bit wide.
>>
>> Let me summarize.
>>
>> We have to have classification by address space
>> 1) physical
>> 2) virtual
>
> That classification is oversimplified, as the DMA address space
> is often not the same as physical.
>

>> dma_addr_t is a physical address wrt DMA mask.
>>
>> Correct?
>
> dma_addr_t is how a device sees RAM, it is limited by the DMA mask
> that is a subset of the capabilities of the device and the buses
> through which it is connected, and is subject to IOMMU translation
> and possible platform or bus specific offsets from the physical
> memory. I still don't know where you're getting with this.

This is off the review already. I'm just structuring knowledge in my head.
In principle I agree with your patch.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Arnd Bergmann]

On Wednesday 18 November 2015 11:35:27 Andy Shevchenko wrote:
> On Fri, Nov 13, 2015 at 11:35 AM, Arnd Bergmann  wrote:
> > On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
> >> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:
> >> > The dw_mmc driver stores the physical address of the MMIO registers
> >> > in a pointer, which requires the use of type casts, and is actually
> >> > broken if anyone ever has this device on a 32-bit SoC in registers
> >> > above 4GB. Gcc warns about this possibility when the driver is built
> >> > with ARM LPAE enabled:
> >>
> >> > -   host->phy_regs = (void *)(regs->start);
> >> > +   host->phy_regs = regs->start;
> >>
> >> > /* Set external dma config: burst size, burst width */
> >> > -   cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
> >> > +   cfg.dst_addr = host->phy_regs + fifo_offset;
> >>
> >> dst_addr is dma_addr_t?
> >
> > Sort of. It doesn't really fit into any of the categories, and we actually
> > had a patch to change the type in the past, see
> > https://lkml.org/lkml/2015/7/10/167. Not sure what is going on there.
> >
> >> > /* Registers's physical base address */
> >> > -   void*phy_regs;
> >> > +   resource_size_t phy_regs;
> >>
> >> If dst_addr is dma_addr_t wouldn't be a problem when
> >> resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?
> >>
> >> Btw, for me casting to dma_addr_t looks sane.
> >
> > The background here is that the address comes from a resource_size_t
> > that describes the MMIO register area as seen from the CPU, and that
> > is normally a phys_addr_t (resource_size_t is defined as being long
> > enough to store a phys_addr_t or various other things depending on
> > resource->flags).
> >
> > dma_addr_t strictly speaking refers to a RAM location as seen by a
> > DMA master, and that only comes out of dma_map_*() or
> > dma_alloc_coherent().
> >
> > The DMA engine wants something else here, which is an MMIO register
> > address as seen by a DMA master, and we don't have a separate typedef
> > for that. Almost universally all of resource_size_t, phys_addr_t and
> > dma_addr_t are the same type, and if we ever get a platform that
> > wants something other than a phys_addr_t to put into cfg.dst_addr,
> > we are in deep trouble.
> 
> DMA operates with address space covered by dma_addr_t, if you use
> phys_addr_t you may get address out of DMA boundaries. This is should
> be done in hardware / firmware / platform representation.
> So, I don't see any reason not to use dma_addr_t here.

As I said above, this isn't really the same as DMA: all normal
dma_addr_t are returned from dma_alloc_* or dma_map_*, point
to RAM and might go trhough an IOMMU, all of which is not true
here, hence the patch to change the type to phys_addr_t.

You really can't get out of bounds because the data comes from a
phys_addr_t and refers to a fixed location in hardware. If a
platform has registers higher than a 32-bit address, its phys_addr_t
must be 64-bit, but its dma_addr_t not necessarily so (even though
the two are the same almost always in practice).

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Andy Shevchenko]

On Fri, Nov 13, 2015 at 11:35 AM, Arnd Bergmann  wrote:
> On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
>> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:
>> > The dw_mmc driver stores the physical address of the MMIO registers
>> > in a pointer, which requires the use of type casts, and is actually
>> > broken if anyone ever has this device on a 32-bit SoC in registers
>> > above 4GB. Gcc warns about this possibility when the driver is built
>> > with ARM LPAE enabled:
>>
>> > -   host->phy_regs = (void *)(regs->start);
>> > +   host->phy_regs = regs->start;
>>
>> > /* Set external dma config: burst size, burst width */
>> > -   cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
>> > +   cfg.dst_addr = host->phy_regs + fifo_offset;
>>
>> dst_addr is dma_addr_t?
>
> Sort of. It doesn't really fit into any of the categories, and we actually
> had a patch to change the type in the past, see
> https://lkml.org/lkml/2015/7/10/167. Not sure what is going on there.
>
>> > /* Registers's physical base address */
>> > -   void*phy_regs;
>> > +   resource_size_t phy_regs;
>>
>> If dst_addr is dma_addr_t wouldn't be a problem when
>> resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?
>>
>> Btw, for me casting to dma_addr_t looks sane.
>
> The background here is that the address comes from a resource_size_t
> that describes the MMIO register area as seen from the CPU, and that
> is normally a phys_addr_t (resource_size_t is defined as being long
> enough to store a phys_addr_t or various other things depending on
> resource->flags).
>
> dma_addr_t strictly speaking refers to a RAM location as seen by a
> DMA master, and that only comes out of dma_map_*() or
> dma_alloc_coherent().
>
> The DMA engine wants something else here, which is an MMIO register
> address as seen by a DMA master, and we don't have a separate typedef
> for that. Almost universally all of resource_size_t, phys_addr_t and
> dma_addr_t are the same type, and if we ever get a platform that
> wants something other than a phys_addr_t to put into cfg.dst_addr,
> we are in deep trouble.

DMA operates with address space covered by dma_addr_t, if you use
phys_addr_t you may get address out of DMA boundaries. This is should
be done in hardware / firmware / platform representation.
So, I don't see any reason not to use dma_addr_t here.

>
> Arnd



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Jaehoon Chung]

Dear, Arnd.

On 11/13/2015 06:35 PM, Arnd Bergmann wrote:
> On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
>> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:
>>> The dw_mmc driver stores the physical address of the MMIO registers
>>> in a pointer, which requires the use of type casts, and is actually
>>> broken if anyone ever has this device on a 32-bit SoC in registers
>>> above 4GB. Gcc warns about this possibility when the driver is built
>>> with ARM LPAE enabled:
>>
>>> -   host->phy_regs = (void *)(regs->start);
>>> +   host->phy_regs = regs->start;
>>
>>> /* Set external dma config: burst size, burst width */
>>> -   cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
>>> +   cfg.dst_addr = host->phy_regs + fifo_offset;
>>
>> dst_addr is dma_addr_t?
> 
> Sort of. It doesn't really fit into any of the categories, and we actually
> had a patch to change the type in the past, see
> https://lkml.org/lkml/2015/7/10/167. Not sure what is going on there.

why isn't the patch applied on mainline yet? :)

Best Regards,
Jaehoon Chung

> 
>>> /* Registers's physical base address */
>>> -   void*phy_regs;
>>> +   resource_size_t phy_regs;
>>
>> If dst_addr is dma_addr_t wouldn't be a problem when
>> resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?
>>
>> Btw, for me casting to dma_addr_t looks sane.
> 
> The background here is that the address comes from a resource_size_t
> that describes the MMIO register area as seen from the CPU, and that
> is normally a phys_addr_t (resource_size_t is defined as being long
> enough to store a phys_addr_t or various other things depending on
> resource->flags).
> 
> dma_addr_t strictly speaking refers to a RAM location as seen by a
> DMA master, and that only comes out of dma_map_*() or
> dma_alloc_coherent().
> 
> The DMA engine wants something else here, which is an MMIO register
> address as seen by a DMA master, and we don't have a separate typedef
> for that. Almost universally all of resource_size_t, phys_addr_t and
> dma_addr_t are the same type, and if we ever get a platform that
> wants something other than a phys_addr_t to put into cfg.dst_addr,
> we are in deep trouble.
> 
>   Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Arnd Bergmann]

On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:
> > The dw_mmc driver stores the physical address of the MMIO registers
> > in a pointer, which requires the use of type casts, and is actually
> > broken if anyone ever has this device on a 32-bit SoC in registers
> > above 4GB. Gcc warns about this possibility when the driver is built
> > with ARM LPAE enabled:
> 
> > -   host->phy_regs = (void *)(regs->start);
> > +   host->phy_regs = regs->start;
> 
> > /* Set external dma config: burst size, burst width */
> > -   cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
> > +   cfg.dst_addr = host->phy_regs + fifo_offset;
> 
> dst_addr is dma_addr_t?

Sort of. It doesn't really fit into any of the categories, and we actually
had a patch to change the type in the past, see
https://lkml.org/lkml/2015/7/10/167. Not sure what is going on there.

> > /* Registers's physical base address */
> > -   void*phy_regs;
> > +   resource_size_t phy_regs;
> 
> If dst_addr is dma_addr_t wouldn't be a problem when
> resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?
> 
> Btw, for me casting to dma_addr_t looks sane.

The background here is that the address comes from a resource_size_t
that describes the MMIO register area as seen from the CPU, and that
is normally a phys_addr_t (resource_size_t is defined as being long
enough to store a phys_addr_t or various other things depending on
resource->flags).

dma_addr_t strictly speaking refers to a RAM location as seen by a
DMA master, and that only comes out of dma_map_*() or
dma_alloc_coherent().

The DMA engine wants something else here, which is an MMIO register
address as seen by a DMA master, and we don't have a separate typedef
for that. Almost universally all of resource_size_t, phys_addr_t and
dma_addr_t are the same type, and if we ever get a platform that
wants something other than a phys_addr_t to put into cfg.dst_addr,
we are in deep trouble.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: dw_mmc: use resource_size_t to store physical address

[Andy Shevchenko]

On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann  wrote:
> The dw_mmc driver stores the physical address of the MMIO registers
> in a pointer, which requires the use of type casts, and is actually
> broken if anyone ever has this device on a 32-bit SoC in registers
> above 4GB. Gcc warns about this possibility when the driver is built
> with ARM LPAE enabled:

> -   host->phy_regs = (void *)(regs->start);
> +   host->phy_regs = regs->start;

> /* Set external dma config: burst size, burst width */
> -   cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
> +   cfg.dst_addr = host->phy_regs + fifo_offset;

dst_addr is dma_addr_t?

> /* Registers's physical base address */
> -   void*phy_regs;
> +   resource_size_t phy_regs;

If dst_addr is dma_addr_t wouldn't be a problem when
resource_size_t is defined as 64-bit address, and dma_addr_t as 32-bit?

Btw, for me casting to dma_addr_t looks sane.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html