| Message ID | 1251130634-15093-1-git-send-email-beckyb@kernel.crashing.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Aug 24, 2009 at 11:17:14AM -0500, Becky Bruce wrote: > Previously, this was specified as a void *, but that's not > large enough on 32-bit systems with 36-bit physical > addressing support. Change the type to dma_addr_t so it > will scale based on the size of a dma address. This looks extreml ugly to me. It seems like the typical use is to store a pointer to a structure. So what about making the direct dma case follow that general scheme instead? E.g. declare a struct direct_dma_data { dma_addr_t direct_dma_offset; }; and have one normal instace of it, and one per weird cell device.
On Mon, 2009-08-24 at 21:48 +0200, Christoph Hellwig wrote: > On Mon, Aug 24, 2009 at 11:17:14AM -0500, Becky Bruce wrote: > > Previously, this was specified as a void *, but that's not > > large enough on 32-bit systems with 36-bit physical > > addressing support. Change the type to dma_addr_t so it > > will scale based on the size of a dma address. > > This looks extreml ugly to me. It seems like the typical use is to > store a pointer to a structure. So what about making the direct > dma case follow that general scheme instead? > > E.g. declare a > > struct direct_dma_data { > dma_addr_t direct_dma_offset; > }; > > and have one normal instace of it, and one per weird cell device. Right, but we want to avoid a structure for the classic case of 32-bit systems with no iommu... I wouldn't mind doing a union here. The other option is to have a global somewhere that we make that point to or something like that but it's probably even more ugly. Cheers, Ben.
On Wed, 2009-08-26 at 22:29 +1000, Benjamin Herrenschmidt wrote: > On Mon, 2009-08-24 at 21:48 +0200, Christoph Hellwig wrote: > > On Mon, Aug 24, 2009 at 11:17:14AM -0500, Becky Bruce wrote: > > > Previously, this was specified as a void *, but that's not > > > large enough on 32-bit systems with 36-bit physical > > > addressing support. Change the type to dma_addr_t so it > > > will scale based on the size of a dma address. > > > > This looks extreml ugly to me. It seems like the typical use is to > > store a pointer to a structure. So what about making the direct > > dma case follow that general scheme instead? > > > > E.g. declare a > > > > struct direct_dma_data { > > dma_addr_t direct_dma_offset; > > }; > > > > and have one normal instace of it, and one per weird cell device. > > Right, but we want to avoid a structure for the classic case of 32-bit > systems with no iommu... > > I wouldn't mind doing a union here. That might be best, the patch as it stands is a horrible mess of casts. Stashing a dma_addr_t into a void * is sort of gross, but storing a pointer to some struct (a void *) in a dma_addr_t is _really_ gross :) cheers
On Aug 26, 2009, at 9:08 AM, Michael Ellerman wrote: > On Wed, 2009-08-26 at 22:29 +1000, Benjamin Herrenschmidt wrote: >> On Mon, 2009-08-24 at 21:48 +0200, Christoph Hellwig wrote: >>> On Mon, Aug 24, 2009 at 11:17:14AM -0500, Becky Bruce wrote: >>>> Previously, this was specified as a void *, but that's not >>>> large enough on 32-bit systems with 36-bit physical >>>> addressing support. Change the type to dma_addr_t so it >>>> will scale based on the size of a dma address. >>> >>> This looks extreml ugly to me. It seems like the typical use is to >>> store a pointer to a structure. So what about making the direct >>> dma case follow that general scheme instead? >>> >>> E.g. declare a >>> >>> struct direct_dma_data { >>> dma_addr_t direct_dma_offset; >>> }; >>> >>> and have one normal instace of it, and one per weird cell device. >> >> Right, but we want to avoid a structure for the classic case of 32- >> bit >> systems with no iommu... >> >> I wouldn't mind doing a union here. > > That might be best, the patch as it stands is a horrible mess of > casts. Let's be fair - the code before was a horrible mess of casts, I've just moved them :) > > Stashing a dma_addr_t into a void * is sort of gross, but storing a > pointer to some struct (a void *) in a dma_addr_t is _really_ gross :) Both are revolting (and storing a dma_addr_t into a void * is really gross when the void * is smaller than the dma_addr_t!!). A union might not be a bad idea, though. I'll look at doing that instead. -Becky
On Wed, 2009-08-26 at 15:20 -0500, Becky Bruce wrote: > On Aug 26, 2009, at 9:08 AM, Michael Ellerman wrote: > > > On Wed, 2009-08-26 at 22:29 +1000, Benjamin Herrenschmidt wrote: > >> On Mon, 2009-08-24 at 21:48 +0200, Christoph Hellwig wrote: > >>> On Mon, Aug 24, 2009 at 11:17:14AM -0500, Becky Bruce wrote: > >>>> Previously, this was specified as a void *, but that's not > >>>> large enough on 32-bit systems with 36-bit physical > >>>> addressing support. Change the type to dma_addr_t so it > >>>> will scale based on the size of a dma address. > >>> > >>> This looks extreml ugly to me. It seems like the typical use is to > >>> store a pointer to a structure. So what about making the direct > >>> dma case follow that general scheme instead? > >>> > >>> E.g. declare a > >>> > >>> struct direct_dma_data { > >>> dma_addr_t direct_dma_offset; > >>> }; > >>> > >>> and have one normal instace of it, and one per weird cell device. > >> > >> Right, but we want to avoid a structure for the classic case of 32- > >> bit > >> systems with no iommu... > >> > >> I wouldn't mind doing a union here. > > > > That might be best, the patch as it stands is a horrible mess of > > casts. > > Let's be fair - the code before was a horrible mess of casts, I've > just moved them :) Yeah true. Though I think we end up with more casts because there were more call sites using it as a pointer originally. But yeah it's not pretty either way. > > Stashing a dma_addr_t into a void * is sort of gross, but storing a > > pointer to some struct (a void *) in a dma_addr_t is _really_ gross :) > > Both are revolting (and storing a dma_addr_t into a void * is really > gross when the void * is smaller than the dma_addr_t!!). A union > might not be a bad idea, though. I'll look at doing that instead. Cool. That is how we're using it, sometimes it points to something sometimes it's a dma_addr_t, so I think a union will work. cheers
diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h index 67fcd7f..07818ae 100644 --- a/arch/powerpc/include/asm/device.h +++ b/arch/powerpc/include/asm/device.h @@ -15,7 +15,7 @@ struct dev_archdata { /* DMA operations on that device */ struct dma_map_ops *dma_ops; - void *dma_data; + dma_addr_t dma_data; #ifdef CONFIG_SWIOTLB dma_addr_t max_direct_dma_addr; #endif diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h index cb2ca41..cf65ebb 100644 --- a/arch/powerpc/include/asm/dma-mapping.h +++ b/arch/powerpc/include/asm/dma-mapping.h @@ -26,7 +26,7 @@ extern void *dma_direct_alloc_coherent(struct device *dev, size_t size, extern void dma_direct_free_coherent(struct device *dev, size_t size, void *vaddr, dma_addr_t dma_handle); -extern unsigned long get_dma_direct_offset(struct device *dev); +extern dma_addr_t get_dma_direct_offset(struct device *dev); #ifdef CONFIG_NOT_COHERENT_CACHE /* diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c index 87ddb3f..13eef19 100644 --- a/arch/powerpc/kernel/dma-iommu.c +++ b/arch/powerpc/kernel/dma-iommu.c @@ -11,6 +11,11 @@ * Generic iommu implementation */ +static inline struct iommu_table *get_iommu_table_base(struct device *dev) +{ + return (struct iommu_table *)dev->archdata.dma_data; +} + /* Allocates a contiguous real buffer and creates mappings over it. * Returns the virtual address of the buffer and sets dma_handle * to the dma address (mapping) of the first page. @@ -18,7 +23,7 @@ static void *dma_iommu_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle, gfp_t flag) { - return iommu_alloc_coherent(dev, dev->archdata.dma_data, size, + return iommu_alloc_coherent(dev, get_iommu_table_base(dev), size, dma_handle, device_to_mask(dev), flag, dev_to_node(dev)); } @@ -26,7 +31,7 @@ static void *dma_iommu_alloc_coherent(struct device *dev, size_t size, static void dma_iommu_free_coherent(struct device *dev, size_t size, void *vaddr, dma_addr_t dma_handle) { - iommu_free_coherent(dev->archdata.dma_data, size, vaddr, dma_handle); + iommu_free_coherent(get_iommu_table_base(dev), size, vaddr, dma_handle); } /* Creates TCEs for a user provided buffer. The user buffer must be @@ -39,8 +44,8 @@ static dma_addr_t dma_iommu_map_page(struct device *dev, struct page *page, enum dma_data_direction direction, struct dma_attrs *attrs) { - return iommu_map_page(dev, dev->archdata.dma_data, page, offset, size, - device_to_mask(dev), direction, attrs); + return iommu_map_page(dev, get_iommu_table_base(dev), page, offset, + size, device_to_mask(dev), direction, attrs); } @@ -48,7 +53,7 @@ static void dma_iommu_unmap_page(struct device *dev, dma_addr_t dma_handle, size_t size, enum dma_data_direction direction, struct dma_attrs *attrs) { - iommu_unmap_page(dev->archdata.dma_data, dma_handle, size, direction, + iommu_unmap_page(get_iommu_table_base(dev), dma_handle, size, direction, attrs); } @@ -57,7 +62,7 @@ static int dma_iommu_map_sg(struct device *dev, struct scatterlist *sglist, int nelems, enum dma_data_direction direction, struct dma_attrs *attrs) { - return iommu_map_sg(dev, dev->archdata.dma_data, sglist, nelems, + return iommu_map_sg(dev, get_iommu_table_base(dev), sglist, nelems, device_to_mask(dev), direction, attrs); } @@ -65,14 +70,14 @@ static void dma_iommu_unmap_sg(struct device *dev, struct scatterlist *sglist, int nelems, enum dma_data_direction direction, struct dma_attrs *attrs) { - iommu_unmap_sg(dev->archdata.dma_data, sglist, nelems, direction, + iommu_unmap_sg(get_iommu_table_base(dev), sglist, nelems, direction, attrs); } /* We support DMA to/from any memory page via the iommu */ static int dma_iommu_dma_supported(struct device *dev, u64 mask) { - struct iommu_table *tbl = dev->archdata.dma_data; + struct iommu_table *tbl = get_iommu_table_base(dev); if (!tbl || tbl->it_offset > mask) { printk(KERN_INFO diff --git a/arch/powerpc/kernel/dma.c b/arch/powerpc/kernel/dma.c index 21b784d..f69825f 100644 --- a/arch/powerpc/kernel/dma.c +++ b/arch/powerpc/kernel/dma.c @@ -17,14 +17,14 @@ * * This implementation supports a per-device offset that can be applied if * the address at which memory is visible to devices is not 0. Platform code - * can set archdata.dma_data to an unsigned long holding the offset. By + * can set archdata.dma_data to a dma_addr_t holding the offset. By * default the offset is PCI_DRAM_OFFSET. */ -unsigned long get_dma_direct_offset(struct device *dev) +dma_addr_t get_dma_direct_offset(struct device *dev) { if (dev) - return (unsigned long)dev->archdata.dma_data; + return dev->archdata.dma_data; return PCI_DRAM_OFFSET; } diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 7585f1f..ecbfc5d 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -1125,7 +1125,7 @@ void __devinit pcibios_setup_bus_devices(struct pci_bus *bus) /* Hook up default DMA ops */ sd->dma_ops = pci_dma_ops; - sd->dma_data = (void *)PCI_DRAM_OFFSET; + sd->dma_data = PCI_DRAM_OFFSET; /* Additional platform DMA/iommu setup */ if (ppc_md.pci_dma_dev_setup) diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c index bc7b41e..24d8d38 100644 --- a/arch/powerpc/kernel/vio.c +++ b/arch/powerpc/kernel/vio.c @@ -1233,7 +1233,8 @@ struct vio_dev *vio_register_device_node(struct device_node *of_node) vio_cmo_set_dma_ops(viodev); else viodev->dev.archdata.dma_ops = &dma_iommu_ops; - viodev->dev.archdata.dma_data = vio_build_iommu_table(viodev); + viodev->dev.archdata.dma_data = + (dma_addr_t)vio_build_iommu_table(viodev); set_dev_node(&viodev->dev, of_node_to_nid(of_node)); /* init generic 'struct device' fields: */ diff --git a/arch/powerpc/platforms/cell/beat_iommu.c b/arch/powerpc/platforms/cell/beat_iommu.c index 93b0efd..6276171 100644 --- a/arch/powerpc/platforms/cell/beat_iommu.c +++ b/arch/powerpc/platforms/cell/beat_iommu.c @@ -77,7 +77,7 @@ static void __init celleb_init_direct_mapping(void) static void celleb_dma_dev_setup(struct device *dev) { dev->archdata.dma_ops = get_pci_dma_ops(); - dev->archdata.dma_data = (void *)celleb_dma_direct_offset; + dev->archdata.dma_data = celleb_dma_direct_offset; } static void celleb_pci_dma_dev_setup(struct pci_dev *pdev) diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 416db17..26f94a6 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -663,9 +663,9 @@ static void cell_dma_dev_setup(struct device *dev) if (get_dma_ops(dev) == &dma_iommu_fixed_ops) cell_dma_dev_setup_fixed(dev); else if (get_pci_dma_ops() == &dma_iommu_ops) - archdata->dma_data = cell_get_iommu_table(dev); + archdata->dma_data = (dma_addr_t)cell_get_iommu_table(dev); else if (get_pci_dma_ops() == &dma_direct_ops) - archdata->dma_data = (void *)cell_dma_direct_offset; + archdata->dma_data = cell_dma_direct_offset; else BUG(); } @@ -977,7 +977,7 @@ static void cell_dma_dev_setup_fixed(struct device *dev) u64 addr; addr = cell_iommu_get_fixed_address(dev) + dma_iommu_fixed_base; - archdata->dma_data = (void *)addr; + archdata->dma_data = addr; dev_dbg(dev, "iommu: fixed addr = %llx\n", addr); } diff --git a/arch/powerpc/platforms/iseries/iommu.c b/arch/powerpc/platforms/iseries/iommu.c index 6c1e101..122c2ef 100644 --- a/arch/powerpc/platforms/iseries/iommu.c +++ b/arch/powerpc/platforms/iseries/iommu.c @@ -193,7 +193,7 @@ static void pci_dma_dev_setup_iseries(struct pci_dev *pdev) pdn->iommu_table = iommu_init_table(tbl, -1); else kfree(tbl); - pdev->dev.archdata.dma_data = pdn->iommu_table; + pdev->dev.archdata.dma_data = (dma_addr_t)pdn->iommu_table; } #else #define pci_dma_dev_setup_iseries NULL diff --git a/arch/powerpc/platforms/pasemi/iommu.c b/arch/powerpc/platforms/pasemi/iommu.c index a0ff03a..91f77b3 100644 --- a/arch/powerpc/platforms/pasemi/iommu.c +++ b/arch/powerpc/platforms/pasemi/iommu.c @@ -189,7 +189,7 @@ static void pci_dma_dev_setup_pasemi(struct pci_dev *dev) } #endif - dev->dev.archdata.dma_data = &iommu_table_iobmap; + dev->dev.archdata.dma_data = (dma_addr_t)&iommu_table_iobmap; } static void pci_dma_bus_setup_null(struct pci_bus *b) { } diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c index 661c8e0..ac8e5c8 100644 --- a/arch/powerpc/platforms/pseries/iommu.c +++ b/arch/powerpc/platforms/pseries/iommu.c @@ -482,7 +482,8 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev) phb->node); iommu_table_setparms(phb, dn, tbl); PCI_DN(dn)->iommu_table = iommu_init_table(tbl, phb->node); - dev->dev.archdata.dma_data = PCI_DN(dn)->iommu_table; + dev->dev.archdata.dma_data = + (dma_addr_t)PCI_DN(dn)->iommu_table; return; } @@ -494,7 +495,8 @@ static void pci_dma_dev_setup_pSeries(struct pci_dev *dev) dn = dn->parent; if (dn && PCI_DN(dn)) - dev->dev.archdata.dma_data = PCI_DN(dn)->iommu_table; + dev->dev.archdata.dma_data = + (dma_addr_t)PCI_DN(dn)->iommu_table; else printk(KERN_WARNING "iommu: Device %s has no iommu table\n", pci_name(dev)); @@ -538,7 +540,8 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) */ if (dma_window == NULL || pdn->parent == NULL) { pr_debug(" no dma window for device, linking to parent\n"); - dev->dev.archdata.dma_data = PCI_DN(pdn)->iommu_table; + dev->dev.archdata.dma_data = + (dma_addr_t)PCI_DN(pdn)->iommu_table; return; } @@ -554,7 +557,7 @@ static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) pr_debug(" found DMA window, table: %p\n", pci->iommu_table); } - dev->dev.archdata.dma_data = pci->iommu_table; + dev->dev.archdata.dma_data = (dma_addr_t)pci->iommu_table; } #else /* CONFIG_PCI */ #define pci_dma_bus_setup_pSeries NULL diff --git a/arch/powerpc/sysdev/dart_iommu.c b/arch/powerpc/sysdev/dart_iommu.c index 89639ec..da74d27 100644 --- a/arch/powerpc/sysdev/dart_iommu.c +++ b/arch/powerpc/sysdev/dart_iommu.c @@ -297,7 +297,7 @@ static void pci_dma_dev_setup_dart(struct pci_dev *dev) /* We only have one iommu table on the mac for now, which makes * things simple. Setup all PCI devices to point to this table */ - dev->dev.archdata.dma_data = &iommu_table_dart; + dev->dev.archdata.dma_data = (dma_addr_t)&iommu_table_dart; } static void pci_dma_bus_setup_dart(struct pci_bus *bus)
Previously, this was specified as a void *, but that's not large enough on 32-bit systems with 36-bit physical addressing support. Change the type to dma_addr_t so it will scale based on the size of a dma address. Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org> --- arch/powerpc/include/asm/device.h | 2 +- arch/powerpc/include/asm/dma-mapping.h | 2 +- arch/powerpc/kernel/dma-iommu.c | 21 +++++++++++++-------- arch/powerpc/kernel/dma.c | 6 +++--- arch/powerpc/kernel/pci-common.c | 2 +- arch/powerpc/kernel/vio.c | 3 ++- arch/powerpc/platforms/cell/beat_iommu.c | 2 +- arch/powerpc/platforms/cell/iommu.c | 6 +++--- arch/powerpc/platforms/iseries/iommu.c | 2 +- arch/powerpc/platforms/pasemi/iommu.c | 2 +- arch/powerpc/platforms/pseries/iommu.c | 11 +++++++---- arch/powerpc/sysdev/dart_iommu.c | 2 +- 12 files changed, 35 insertions(+), 26 deletions(-)