| Message ID | 20180920185247.20037-5-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/5] dma-mapping: make the get_required_mask method available unconditionally | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | next/apply_patch Patch failed to apply |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On 20/09/18 19:52, Christoph Hellwig wrote: > Instead of rejecting devices with a too small bus_dma_mask we can handle > by taking the bus dma_mask into account for allocations and bounce > buffering decisions. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/linux/dma-direct.h | 3 ++- > kernel/dma/direct.c | 21 +++++++++++---------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h > index b79496d8c75b..fbca184ff5a0 100644 > --- a/include/linux/dma-direct.h > +++ b/include/linux/dma-direct.h > @@ -27,7 +27,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size) > if (!dev->dma_mask) > return false; > > - return addr + size - 1 <= *dev->dma_mask; > + return addr + size - 1 <= > + min_not_zero(*dev->dma_mask, dev->bus_dma_mask); > } > #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */ > > diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c > index 3c404e33d946..64466b7ef67b 100644 > --- a/kernel/dma/direct.c > +++ b/kernel/dma/direct.c > @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size, > return false; > } > > - if (*dev->dma_mask >= DMA_BIT_MASK(32)) { > + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) { Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due to a global DT property, we'll now scream where we didn't before even though the bus mask is almost certainly irrelevant - is that desirable? > dev_err(dev, > - "%s: overflow %pad+%zu of device mask %llx\n", > - caller, &dma_addr, size, *dev->dma_mask); > + "%s: overflow %pad+%zu of device mask %llx bus mask %llx\n", > + caller, &dma_addr, size, > + *dev->dma_mask, dev->bus_dma_mask); > } > return false; > } > @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev) > { > u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT); > > + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma) > + max_dma = dev->bus_dma_mask; Again, I think we could just do another min_not_zero() here. A device wired to address only one single page of RAM isn't a realistic prospect (and we could just flip the -1 and the shift in the max_dma calculation if we *really* wanted to support such things). > + > return (1ULL << (fls64(max_dma) - 1)) * 2 - 1; > } > > static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask, > u64 *phys_mask) > { > + if (dev->bus_dma_mask && dev->bus_dma_mask < dma_mask) > + dma_mask = dev->bus_dma_mask; > + Similarly, can't we assume dma_mask to be nonzero here too? It feels like we really shouldn't have managed to get this far without one. Robin. > if (force_dma_unencrypted()) > *phys_mask = __dma_to_phys(dev, dma_mask); > else > @@ -87,7 +94,7 @@ static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask, > static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size) > { > return phys_to_dma_direct(dev, phys) + size - 1 <= > - dev->coherent_dma_mask; > + min_not_zero(dev->coherent_dma_mask, dev->bus_dma_mask); > } > > void *dma_direct_alloc_pages(struct device *dev, size_t size, > @@ -291,12 +298,6 @@ int dma_direct_supported(struct device *dev, u64 mask) > if (mask < phys_to_dma(dev, DMA_BIT_MASK(32))) > return 0; > #endif > - /* > - * Upstream PCI/PCIe bridges or SoC interconnects may not carry > - * as many DMA address bits as the device itself supports. > - */ > - if (dev->bus_dma_mask && mask > dev->bus_dma_mask) > - return 0; > return 1; > } > >
On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote: >> } >> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */ >> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >> index 3c404e33d946..64466b7ef67b 100644 >> --- a/kernel/dma/direct.c >> +++ b/kernel/dma/direct.c >> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size, >> return false; >> } >> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) { >> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) { > > Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due > to a global DT property, we'll now scream where we didn't before even > though the bus mask is almost certainly irrelevant - is that desirable? This is just the reporting in the error case - we'll only hit this IFF dma_capable already returned false. But if you don't want a message here we can probably drop this hunk. >> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev) >> { >> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT); >> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma) >> + max_dma = dev->bus_dma_mask; > > Again, I think we could just do another min_not_zero() here. A device wired > to address only one single page of RAM isn't a realistic prospect (and we > could just flip the -1 and the shift in the max_dma calculation if we > *really* wanted to support such things). This just seemed more readable to me than min_not_zero, but if others prefer min_not_zero I can switch.
On 27/09/18 16:32, Christoph Hellwig wrote: > On Thu, Sep 27, 2018 at 03:58:04PM +0100, Robin Murphy wrote: >>> } >>> #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */ >>> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c >>> index 3c404e33d946..64466b7ef67b 100644 >>> --- a/kernel/dma/direct.c >>> +++ b/kernel/dma/direct.c >>> @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size, >>> return false; >>> } >>> - if (*dev->dma_mask >= DMA_BIT_MASK(32)) { >>> + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) { >> >> Hmm... say *dev->dma_mask is 31 bits and dev->bus_dma_mask is 40 bits due >> to a global DT property, we'll now scream where we didn't before even >> though the bus mask is almost certainly irrelevant - is that desirable? > > This is just the reporting in the error case - we'll only hit this > IFF dma_capable already returned false. But if you don't want a message > here we can probably drop this hunk. It was only a question of consistency - whatever the reason was to avoid warning for small device masks before, it's not obvious why it wouldn't still apply in the presence of nonzero but larger bus mask. The fact that the current check is there at all implied to me that we're expecting less-capable device to hit this often and thus wanted to avoid being noisy. If that's not the case then it's fine as-is AFAIC. >>> @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev) >>> { >>> u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT); >>> + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma) >>> + max_dma = dev->bus_dma_mask; >> >> Again, I think we could just do another min_not_zero() here. A device wired >> to address only one single page of RAM isn't a realistic prospect (and we >> could just flip the -1 and the shift in the max_dma calculation if we >> *really* wanted to support such things). > > This just seemed more readable to me than min_not_zero, but if others > prefer min_not_zero I can switch. Nah, just checking whether there were any intentionally different assumptions compared to the couple of other places in the patch where min_not_zero() *is* used. If it's purely a style thing then no worries (personally I'd have written it yet another way anyway). Robin.
On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote: >> This just seemed more readable to me than min_not_zero, but if others >> prefer min_not_zero I can switch. > > Nah, just checking whether there were any intentionally different > assumptions compared to the couple of other places in the patch where > min_not_zero() *is* used. If it's purely a style thing then no worries > (personally I'd have written it yet another way anyway). I'm curious: how would you have written it?
On 27/09/18 17:27, Christoph Hellwig wrote: > On Thu, Sep 27, 2018 at 05:14:56PM +0100, Robin Murphy wrote: >>> This just seemed more readable to me than min_not_zero, but if others >>> prefer min_not_zero I can switch. >> >> Nah, just checking whether there were any intentionally different >> assumptions compared to the couple of other places in the patch where >> min_not_zero() *is* used. If it's purely a style thing then no worries >> (personally I'd have written it yet another way anyway). > > I'm curious: how would you have written it? Come to think of it, I actually already have, in iommu-dma: if (dev->bus_dma_mask) mask &= dev->bus_dma_mask; but of course it's not so pretty for those cases where you don't already have the local variable ready to go. Robin.
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index b79496d8c75b..fbca184ff5a0 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -27,7 +27,8 @@ static inline bool dma_capable(struct device *dev, dma_addr_t addr, size_t size) if (!dev->dma_mask) return false; - return addr + size - 1 <= *dev->dma_mask; + return addr + size - 1 <= + min_not_zero(*dev->dma_mask, dev->bus_dma_mask); } #endif /* !CONFIG_ARCH_HAS_PHYS_TO_DMA */ diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 3c404e33d946..64466b7ef67b 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -43,10 +43,11 @@ check_addr(struct device *dev, dma_addr_t dma_addr, size_t size, return false; } - if (*dev->dma_mask >= DMA_BIT_MASK(32)) { + if (*dev->dma_mask >= DMA_BIT_MASK(32) || dev->bus_dma_mask) { dev_err(dev, - "%s: overflow %pad+%zu of device mask %llx\n", - caller, &dma_addr, size, *dev->dma_mask); + "%s: overflow %pad+%zu of device mask %llx bus mask %llx\n", + caller, &dma_addr, size, + *dev->dma_mask, dev->bus_dma_mask); } return false; } @@ -65,12 +66,18 @@ u64 dma_direct_get_required_mask(struct device *dev) { u64 max_dma = phys_to_dma_direct(dev, (max_pfn - 1) << PAGE_SHIFT); + if (dev->bus_dma_mask && dev->bus_dma_mask < max_dma) + max_dma = dev->bus_dma_mask; + return (1ULL << (fls64(max_dma) - 1)) * 2 - 1; } static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask, u64 *phys_mask) { + if (dev->bus_dma_mask && dev->bus_dma_mask < dma_mask) + dma_mask = dev->bus_dma_mask; + if (force_dma_unencrypted()) *phys_mask = __dma_to_phys(dev, dma_mask); else @@ -87,7 +94,7 @@ static gfp_t __dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask, static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size) { return phys_to_dma_direct(dev, phys) + size - 1 <= - dev->coherent_dma_mask; + min_not_zero(dev->coherent_dma_mask, dev->bus_dma_mask); } void *dma_direct_alloc_pages(struct device *dev, size_t size, @@ -291,12 +298,6 @@ int dma_direct_supported(struct device *dev, u64 mask) if (mask < phys_to_dma(dev, DMA_BIT_MASK(32))) return 0; #endif - /* - * Upstream PCI/PCIe bridges or SoC interconnects may not carry - * as many DMA address bits as the device itself supports. - */ - if (dev->bus_dma_mask && mask > dev->bus_dma_mask) - return 0; return 1; }
Instead of rejecting devices with a too small bus_dma_mask we can handle by taking the bus dma_mask into account for allocations and bounce buffering decisions. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/linux/dma-direct.h | 3 ++- kernel/dma/direct.c | 21 +++++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-)