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

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

[Arnd Bergmann]

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:

mmc/host/dw_mmc.c: In function 'dw_mci_edmac_start_dma':
mmc/host/dw_mmc.c:702:17: warning: cast from pointer to integer of different size
  cfg.dst_addr = (dma_addr_t)(host->phy_regs + fifo_offset);
                 ^
mmc/host/dw_mmc-pltfm.c: In function 'dw_mci_pltfm_register':
mmc/host/dw_mmc-pltfm.c:63:19: warning: cast to pointer from integer of different size
  host->phy_regs = (void *)(regs->start);

This changes the code to use resource_size_t, which gets rid of the
warning, the bug and the useless casts.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
>From the arm64 allmodconfig build reports

diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 7e1d13b68b06..81bdeeb05a4d 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -60,7 +60,7 @@ int dw_mci_pltfm_register(struct platform_device *pdev,
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	/* Get registers' physical base address */
-	host->phy_regs = (void *)(regs->start);
+	host->phy_regs = regs->start;
 	host->regs = devm_ioremap_resource(&pdev->dev, regs);
 	if (IS_ERR(host->regs))
 		return PTR_ERR(host->regs);
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 7a6cedbe48a8..fb204ee6ff89 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -699,7 +699,7 @@ static int dw_mci_edmac_start_dma(struct dw_mci *host,
 	int ret = 0;
 
 	/* 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;
 	cfg.src_addr = cfg.dst_addr;
 	cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 	cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h
index f67b2ec18e6d..7776afb0ffa5 100644
--- a/include/linux/mmc/dw_mmc.h
+++ b/include/linux/mmc/dw_mmc.h
@@ -172,7 +172,7 @@ struct dw_mci {
 	/* For edmac */
 	struct dw_mci_dma_slave *dms;
 	/* Registers's physical base address */
-	void                    *phy_regs;
+	resource_size_t		phy_regs;
 
 	u32			cmd_status;
 	u32			data_status;

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

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

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

[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 <arnd@arndb.de> wrote:
> On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
>> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> 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

[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 <arnd@arndb.de> wrote:
> > On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
> >> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> 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

[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 <arnd@arndb.de> wrote:
> On Wednesday 18 November 2015 11:35:27 Andy Shevchenko wrote:
>> On Fri, Nov 13, 2015 at 11:35 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 13 November 2015 03:10:13 Andy Shevchenko wrote:
>> >> On Thu, Nov 12, 2015 at 4:14 PM, Arnd Bergmann <arnd@arndb.de> 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

[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

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

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

[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 <arnd@arndb.de> wrote:
> On Wednesday 18 November 2015 18:17:32 Andy Shevchenko wrote:
>> On Wed, Nov 18, 2015 at 5:45 PM, Arnd Bergmann <arnd@arndb.de> 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