| Message ID | 28102387.ALpaBHpim0@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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;
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html