| Message ID | 20170616181059.19206-43-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-06-16 at 20:10 +0200, Christoph Hellwig wrote: > Besides removing the last instance of the set_dma_mask method this also > reduced the code duplication. What is your rationale here ? (I have missed patch 0 it seems). dma_supported() was supposed to be pretty much a "const" function simply informing whether a given setup is possible. Having it perform an actual switch of ops seems to be pushing it... What if a driver wants to test various dma masks and then pick one ? Where does the API documents that if a driver calls dma_supported() it then *must* set the corresponding mask and use that ? I don't like a function that is a "boolean query" like this one to have such a major side effect. From an API standpoint, dma_set_mask() is when the mask is established, and thus when the ops switch should happen. Ben. > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/powerpc/platforms/cell/iommu.c | 25 +++++++++---------------- > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c > index 497bfbdbd967..29d4f96ed33e 100644 > --- a/arch/powerpc/platforms/cell/iommu.c > +++ b/arch/powerpc/platforms/cell/iommu.c > @@ -644,20 +644,14 @@ static void dma_fixed_unmap_sg(struct device *dev, struct scatterlist *sg, > direction, attrs); > } > > -static int dma_fixed_dma_supported(struct device *dev, u64 mask) > -{ > - return mask == DMA_BIT_MASK(64); > -} > - > -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask); > +static int dma_suported_and_switch(struct device *dev, u64 dma_mask); > > static const struct dma_map_ops dma_iommu_fixed_ops = { > .alloc = dma_fixed_alloc_coherent, > .free = dma_fixed_free_coherent, > .map_sg = dma_fixed_map_sg, > .unmap_sg = dma_fixed_unmap_sg, > - .dma_supported = dma_fixed_dma_supported, > - .set_dma_mask = dma_set_mask_and_switch, > + .dma_supported = dma_suported_and_switch, > .map_page = dma_fixed_map_page, > .unmap_page = dma_fixed_unmap_page, > .mapping_error = dma_iommu_mapping_error, > @@ -952,11 +946,8 @@ static u64 cell_iommu_get_fixed_address(struct device *dev) > return dev_addr; > } > > -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask) > +static int dma_suported_and_switch(struct device *dev, u64 dma_mask) > { > - if (!dev->dma_mask || !dma_supported(dev, dma_mask)) > - return -EIO; > - > if (dma_mask == DMA_BIT_MASK(64) && > cell_iommu_get_fixed_address(dev) != OF_BAD_ADDR) { > u64 addr = cell_iommu_get_fixed_address(dev) + > @@ -965,14 +956,16 @@ static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask) > dev_dbg(dev, "iommu: fixed addr = %llx\n", addr); > set_dma_ops(dev, &dma_iommu_fixed_ops); > set_dma_offset(dev, addr); > - } else { > + return 1; > + } > + > + if (dma_iommu_dma_supported(dev, dma_mask)) { > dev_dbg(dev, "iommu: not 64-bit, using default ops\n"); > set_dma_ops(dev, get_pci_dma_ops()); > cell_dma_dev_setup(dev); > + return 1; > } > > - *dev->dma_mask = dma_mask; > - > return 0; > } > > @@ -1127,7 +1120,7 @@ static int __init cell_iommu_fixed_mapping_init(void) > cell_iommu_setup_window(iommu, np, dbase, dsize, 0); > } > > - dma_iommu_ops.set_dma_mask = dma_set_mask_and_switch; > + dma_iommu_ops.dma_supported = dma_suported_and_switch; > set_pci_dma_ops(&dma_iommu_ops); > > return 0;
On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote: > What is your rationale here ? (I have missed patch 0 it seems). Less code duplication, more modular dma_map_ops insteance. > dma_supported() was supposed to be pretty much a "const" function > simply informing whether a given setup is possible. Having it perform > an actual switch of ops seems to be pushing it... dma_supported() is already gone from the public DMA API as it doesn't make sense to be called separately from set_dma_mask. It will be entirely gone in the next series after this one. > What if a driver wants to test various dma masks and then pick one ? > > Where does the API documents that if a driver calls dma_supported() it > then *must* set the corresponding mask and use that ? Where is the API document for _any_ of the dma routines? (A: work in progress by me, but I need to clean up the mess of arch hooks before it can make any sense) > I don't like a function that is a "boolean query" like this one to have > such a major side effect. > > >From an API standpoint, dma_set_mask() is when the mask is established, > and thus when the ops switch should happen. And that's exactly what happens at the driver API level. It just turns out the dma_capable method is they way better place to actually implement it, as the ->set_dma_mask method requires lots of code duplication while not offering any actual benefit over ->dma_capable. And because of that it's gone after this series. In theory we could rename ->dma_capable now, but it would require a _lot_ of churn. Give me another merge window or two and we should be down to be about 2 handful of dma_map_ops instance, at which point we could do all this gratious renaming a lot more easily :)
On Sun, 2017-06-18 at 00:13 -0700, Christoph Hellwig wrote: > On Sun, Jun 18, 2017 at 06:50:27AM +1000, Benjamin Herrenschmidt wrote: > > What is your rationale here ? (I have missed patch 0 it seems). > > Less code duplication, more modular dma_map_ops insteance. > > > dma_supported() was supposed to be pretty much a "const" function > > simply informing whether a given setup is possible. Having it perform > > an actual switch of ops seems to be pushing it... > > dma_supported() is already gone from the public DMA API as it doesn't > make sense to be called separately from set_dma_mask. It will be > entirely gone in the next series after this one. Ah ok, in that case it makes much more sense, we can rename it then. > > What if a driver wants to test various dma masks and then pick one ? > > > > Where does the API documents that if a driver calls dma_supported() it > > then *must* set the corresponding mask and use that ? > > Where is the API document for _any_ of the dma routines? (A: work in > progress by me, but I need to clean up the mess of arch hooks before > it can make any sense) Heh fair enough. > > I don't like a function that is a "boolean query" like this one to have > > such a major side effect. > > > > > From an API standpoint, dma_set_mask() is when the mask is established, > > > > and thus when the ops switch should happen. > > And that's exactly what happens at the driver API level. It just turns > out the dma_capable method is they way better place to actually > implement it, as the ->set_dma_mask method requires lots of code > duplication while not offering any actual benefit over ->dma_capable. > And because of that it's gone after this series. > > In theory we could rename ->dma_capable now, but it would require a > _lot_ of churn. Give me another merge window or two and we should > be down to be about 2 handful of dma_map_ops instance, at which point > we could do all this gratious renaming a lot more easily :) Sure, I get it now, as long as it's not publicly exposed to drivers we are fine. Cheers, Ben.
diff --git a/arch/powerpc/platforms/cell/iommu.c b/arch/powerpc/platforms/cell/iommu.c index 497bfbdbd967..29d4f96ed33e 100644 --- a/arch/powerpc/platforms/cell/iommu.c +++ b/arch/powerpc/platforms/cell/iommu.c @@ -644,20 +644,14 @@ static void dma_fixed_unmap_sg(struct device *dev, struct scatterlist *sg, direction, attrs); } -static int dma_fixed_dma_supported(struct device *dev, u64 mask) -{ - return mask == DMA_BIT_MASK(64); -} - -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask); +static int dma_suported_and_switch(struct device *dev, u64 dma_mask); static const struct dma_map_ops dma_iommu_fixed_ops = { .alloc = dma_fixed_alloc_coherent, .free = dma_fixed_free_coherent, .map_sg = dma_fixed_map_sg, .unmap_sg = dma_fixed_unmap_sg, - .dma_supported = dma_fixed_dma_supported, - .set_dma_mask = dma_set_mask_and_switch, + .dma_supported = dma_suported_and_switch, .map_page = dma_fixed_map_page, .unmap_page = dma_fixed_unmap_page, .mapping_error = dma_iommu_mapping_error, @@ -952,11 +946,8 @@ static u64 cell_iommu_get_fixed_address(struct device *dev) return dev_addr; } -static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask) +static int dma_suported_and_switch(struct device *dev, u64 dma_mask) { - if (!dev->dma_mask || !dma_supported(dev, dma_mask)) - return -EIO; - if (dma_mask == DMA_BIT_MASK(64) && cell_iommu_get_fixed_address(dev) != OF_BAD_ADDR) { u64 addr = cell_iommu_get_fixed_address(dev) + @@ -965,14 +956,16 @@ static int dma_set_mask_and_switch(struct device *dev, u64 dma_mask) dev_dbg(dev, "iommu: fixed addr = %llx\n", addr); set_dma_ops(dev, &dma_iommu_fixed_ops); set_dma_offset(dev, addr); - } else { + return 1; + } + + if (dma_iommu_dma_supported(dev, dma_mask)) { dev_dbg(dev, "iommu: not 64-bit, using default ops\n"); set_dma_ops(dev, get_pci_dma_ops()); cell_dma_dev_setup(dev); + return 1; } - *dev->dma_mask = dma_mask; - return 0; } @@ -1127,7 +1120,7 @@ static int __init cell_iommu_fixed_mapping_init(void) cell_iommu_setup_window(iommu, np, dbase, dsize, 0); } - dma_iommu_ops.set_dma_mask = dma_set_mask_and_switch; + dma_iommu_ops.dma_supported = dma_suported_and_switch; set_pci_dma_ops(&dma_iommu_ops); return 0;
Besides removing the last instance of the set_dma_mask method this also reduced the code duplication. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/powerpc/platforms/cell/iommu.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-)