| Message ID | 0819179085df6c41c70e83a2c5c138b95c0386b3.1502468875.git.robin.murphy@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 11, 2017 at 05:29:56PM +0100, Robin Murphy wrote: > Historically, DMA masks have suffered some ambiguity between whether > they represent the range of physical memory a device can access, or the > address bits a device is capable of driving, particularly since on many > platforms the two are equivalent. Whilst there are some stragglers left > (dma_max_pfn(), I'm looking at you...), the majority of DMA code has > been cleaned up to follow the latter definition, not least since it is > the only one which makes sense once IOMMUs are involved. I think it always was supposed to be the latter, but that doesn't mean that everyone got the message :) > Either of these patches alone should be sufficient to un-break RPi3, > and they apply independently, so I'm quite happy for one to go in as a > fix now and the other to wait for 4.14. This one is something I'm comfortable feeding to Linus for 4.13 if I get a few ACKs from people familar with the OF code and intended meaning of the ranges in the device tree.
On Fri, Aug 11, 2017 at 05:29:57PM +0100, Robin Murphy wrote: > The good news is that DT already gives us the ammunition to do the right > thing - anything lacking a "dma-ranges" property should be considered > not to have a mapping of DMA address space from its children to its > parent, thus anything for which of_dma_get_range() does not succeed does > not need DMA configuration. That sounds like a good heuristic. At least in theory apparently :( > The bad news is that strictly enforcing that would likely break just > about every FDT platform out there, since most authors have either not > considered the property at all or have mistakenly assumed that omitting > "dma-ranges" is equivalent to including the empty property. Thus we have > little choice but to special-case platform, AMBA and PCI devices so they > continue to receive configuration unconditionally as before. At least > anything new will have to get it right in future... That still sounds like a useful compromise. > ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > if (ret < 0) { > + /* > + * For legacy reasons, we have to assume some devices need > + * DMA configuration regardless of whether "dma-ranges" is > + * correctly specified or not. > + */ > + if (!dev_is_pci(dev) && > +#ifdef CONFIG_ARM_AMBA > + dev->bus != &amba_bustype && > +#endif > + dev->bus != &platform_bus_type) > + return ret == -ENODEV ? 0 : ret; > + It would be really nice to have dev_is_amba and dev_is_plaform helpers to reduce the ifdef mess. But it should be okay even without that cleanup. > + if (!size) > + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); I find this way to deal with an overflow really odd, but given that it's just moved around I'm not going to complain.
+linuxppc-dev On Fri, Aug 11, 2017 at 11:29 AM, Robin Murphy <robin.murphy@arm.com> wrote: > Moving DMA configuration to happen later at driver probe time had the > unnoticed side-effect that we now perform DMA configuration for *every* > device represented in DT, rather than only those explicitly created by > the of_platform and PCI code. > > As Christoph points out, this is not really the best thing to do. Whilst > there may well be other DMA-capable buses that can benefit from having > their children automatically configured after the bridge has probed, > there are also plenty of others like USB, MDIO, etc. that definitely do > not support DMA and should not be indiscriminately processed. > > The good news is that DT already gives us the ammunition to do the right > thing - anything lacking a "dma-ranges" property should be considered > not to have a mapping of DMA address space from its children to its > parent, thus anything for which of_dma_get_range() does not succeed does > not need DMA configuration. > > The bad news is that strictly enforcing that would likely break just > about every FDT platform out there, since most authors have either not > considered the property at all or have mistakenly assumed that omitting > "dma-ranges" is equivalent to including the empty property. Thus we have > little choice but to special-case platform, AMBA and PCI devices so they > continue to receive configuration unconditionally as before. At least > anything new will have to get it right in future... By "anything new", you mean new buses, not new platforms, right? What's a platform bus device today could be a different kernel bus type tomorrow with no DT change. So this isn't really enforceable. I don't completely agree that omitting dma-ranges is wrong and that new DTs have to have dma-ranges simply because there is much precedent of DTs with dma-ranges omitted (just go look at PPC). If a bus has no bus to cpu address translation nor size restrictions, then no dma-ranges should be allowed. Of course, DT standards can and do evolve and we could decide to be stricter here, but that hasn't happened. If it does, then we need to make that clear in the spec and enforce it. Rob > > Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices") > Reported-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/of/device.c | 48 ++++++++++++++++++++++++++++++++---------------- > 1 file changed, 32 insertions(+), 16 deletions(-) > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index e0a28ea341fe..04c4c952dc57 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -9,6 +9,9 @@ > #include <linux/module.h> > #include <linux/mod_devicetable.h> > #include <linux/slab.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/amba/bus.h> > > #include <asm/errno.h> > #include "of_private.h" > @@ -84,31 +87,28 @@ int of_device_add(struct platform_device *ofdev) > */ > int of_dma_configure(struct device *dev, struct device_node *np) > { > - u64 dma_addr, paddr, size; > + u64 dma_addr, paddr, size = 0; > int ret; > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > u64 mask; > > - /* > - * Set default coherent_dma_mask to 32 bit. Drivers are expected to > - * setup the correct supported mask. > - */ > - if (!dev->coherent_dma_mask) > - dev->coherent_dma_mask = DMA_BIT_MASK(32); > - > - /* > - * Set it to coherent_dma_mask by default if the architecture > - * code has not set it. > - */ > - if (!dev->dma_mask) > - dev->dma_mask = &dev->coherent_dma_mask; > - > ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > if (ret < 0) { > + /* > + * For legacy reasons, we have to assume some devices need > + * DMA configuration regardless of whether "dma-ranges" is > + * correctly specified or not. > + */ > + if (!dev_is_pci(dev) && > +#ifdef CONFIG_ARM_AMBA > + dev->bus != &amba_bustype && > +#endif > + dev->bus != &platform_bus_type) > + return ret == -ENODEV ? 0 : ret; > + > dma_addr = offset = 0; > - size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > } else { > offset = PFN_DOWN(paddr - dma_addr); > > @@ -129,6 +129,22 @@ int of_dma_configure(struct device *dev, struct device_node *np) > dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); > } > > + /* > + * Set default coherent_dma_mask to 32 bit. Drivers are expected to > + * setup the correct supported mask. > + */ > + if (!dev->coherent_dma_mask) > + dev->coherent_dma_mask = DMA_BIT_MASK(32); > + /* > + * Set it to coherent_dma_mask by default if the architecture > + * code has not set it. > + */ > + if (!dev->dma_mask) > + dev->dma_mask = &dev->coherent_dma_mask; > + > + if (!size) > + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > + > dev->dma_pfn_offset = offset; > > /* > -- > 2.13.4.dirty >
On Fri, Aug 11, 2017 at 11:29 AM, Robin Murphy <robin.murphy@arm.com> wrote: > Historically, DMA masks have suffered some ambiguity between whether > they represent the range of physical memory a device can access, or the > address bits a device is capable of driving, particularly since on many > platforms the two are equivalent. Whilst there are some stragglers left > (dma_max_pfn(), I'm looking at you...), the majority of DMA code has > been cleaned up to follow the latter definition, not least since it is > the only one which makes sense once IOMMUs are involved. > > In this respect, of_dma_configure() has always done the wrong thing in > how it generates initial masks based on "dma-ranges". Although rounding > down did not affect the TI Keystone platform where dma_addr + size is > already a power of two, in any other case it results in a mask which is > at best unnecessarily constrained and at worst unusable. > > BCM2837 illustrates the problem nicely, where we have a DMA base of 3GB > and a size of 1GB - 16MB, giving dma_addr + size = 0xff000000 and a > resultant mask of 0x7fffffff, which is then insufficient to even cover > the necessary offset, effectively making all DMA addresses out-of-range. > This has been hidden until now (mostly because we don't yet prevent > drivers from simply overwriting this initial mask later upon probe), but > due to recent changes elsewhere now shows up as USB being broken on > Raspberry Pi 3. > > Make it right by rounding up instead of down, such that the mask > correctly correctly describes all possisble bits the device needs to > emit. > > Fixes: 9a6d7298b083 ("of: Calculate device DMA masks based on DT dma-range size") > Reported-by: Stefan Wahren <stefan.wahren@i2se.com> > Reported-by: Andreas Färber <afaerber@suse.de> > Reported-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > > Either of these patches alone should be sufficient to un-break RPi3, > and they apply independently, so I'm quite happy for one to go in as a > fix now and the other to wait for 4.14. Acked-by: Rob Herring <robh@kernel.org> > > Robin. > > drivers/of/device.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 28c38c756f92..e0a28ea341fe 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -89,6 +89,7 @@ int of_dma_configure(struct device *dev, struct device_node *np) > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + u64 mask; > > /* > * Set default coherent_dma_mask to 32 bit. Drivers are expected to > @@ -134,10 +135,9 @@ int of_dma_configure(struct device *dev, struct device_node *np) > * Limit coherent and dma mask based on size and default mask > * set by the driver. > */ > - dev->coherent_dma_mask = min(dev->coherent_dma_mask, > - DMA_BIT_MASK(ilog2(dma_addr + size))); > - *dev->dma_mask = min((*dev->dma_mask), > - DMA_BIT_MASK(ilog2(dma_addr + size))); > + mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); > + dev->coherent_dma_mask &= mask; > + *dev->dma_mask &= mask; > > coherent = of_dma_is_coherent(np); > dev_dbg(dev, "device is%sdma coherent\n", > -- > 2.13.4.dirty >
On 14/08/17 21:08, Rob Herring wrote: > +linuxppc-dev > > On Fri, Aug 11, 2017 at 11:29 AM, Robin Murphy <robin.murphy@arm.com> wrote: >> Moving DMA configuration to happen later at driver probe time had the >> unnoticed side-effect that we now perform DMA configuration for *every* >> device represented in DT, rather than only those explicitly created by >> the of_platform and PCI code. >> >> As Christoph points out, this is not really the best thing to do. Whilst >> there may well be other DMA-capable buses that can benefit from having >> their children automatically configured after the bridge has probed, >> there are also plenty of others like USB, MDIO, etc. that definitely do >> not support DMA and should not be indiscriminately processed. >> >> The good news is that DT already gives us the ammunition to do the right >> thing - anything lacking a "dma-ranges" property should be considered >> not to have a mapping of DMA address space from its children to its >> parent, thus anything for which of_dma_get_range() does not succeed does >> not need DMA configuration. >> >> The bad news is that strictly enforcing that would likely break just >> about every FDT platform out there, since most authors have either not >> considered the property at all or have mistakenly assumed that omitting >> "dma-ranges" is equivalent to including the empty property. Thus we have >> little choice but to special-case platform, AMBA and PCI devices so they >> continue to receive configuration unconditionally as before. At least >> anything new will have to get it right in future... > > By "anything new", you mean new buses, not new platforms, right? > What's a platform bus device today could be a different kernel bus > type tomorrow with no DT change. So this isn't really enforceable. Indeed, it would be virtually impossible to do anything on a per-platform basis, but I think per-bus is a workable compromise - if someone changes their hypothetical bus driver in a way that would affect deployed DTs that can't be updated, at worst they can still add their new bus type to the special case list at the same time. > I don't completely agree that omitting dma-ranges is wrong and that > new DTs have to have dma-ranges simply because there is much precedent > of DTs with dma-ranges omitted (just go look at PPC). If a bus has no > bus to cpu address translation nor size restrictions, then no > dma-ranges should be allowed. Sure, I agree that that genie is never going back in the bottle, but people seem to manage to get empty "ranges" right to differentiate between memory-mapped vs. non-memory-mapped buses, so it would be nice to encourage getting the other direction right as well. For the immediate issue at hand, I guess the alternative plan of attack would be to stick a flag in struct bus_type for the bus drivers themselves to opt into generic DMA configuration. That at least keeps everything within the kernel (and come to think of it probably works neatly for modular bus types as well). Robin. > Of course, DT standards can and do > evolve and we could decide to be stricter here, but that hasn't > happened. If it does, then we need to make that clear in the spec and > enforce it. > > Rob > > >> >> Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices") >> Reported-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/of/device.c | 48 ++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 32 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index e0a28ea341fe..04c4c952dc57 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -9,6 +9,9 @@ >> #include <linux/module.h> >> #include <linux/mod_devicetable.h> >> #include <linux/slab.h> >> +#include <linux/pci.h> >> +#include <linux/platform_device.h> >> +#include <linux/amba/bus.h> >> >> #include <asm/errno.h> >> #include "of_private.h" >> @@ -84,31 +87,28 @@ int of_device_add(struct platform_device *ofdev) >> */ >> int of_dma_configure(struct device *dev, struct device_node *np) >> { >> - u64 dma_addr, paddr, size; >> + u64 dma_addr, paddr, size = 0; >> int ret; >> bool coherent; >> unsigned long offset; >> const struct iommu_ops *iommu; >> u64 mask; >> >> - /* >> - * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> - * setup the correct supported mask. >> - */ >> - if (!dev->coherent_dma_mask) >> - dev->coherent_dma_mask = DMA_BIT_MASK(32); >> - >> - /* >> - * Set it to coherent_dma_mask by default if the architecture >> - * code has not set it. >> - */ >> - if (!dev->dma_mask) >> - dev->dma_mask = &dev->coherent_dma_mask; >> - >> ret = of_dma_get_range(np, &dma_addr, &paddr, &size); >> if (ret < 0) { >> + /* >> + * For legacy reasons, we have to assume some devices need >> + * DMA configuration regardless of whether "dma-ranges" is >> + * correctly specified or not. >> + */ >> + if (!dev_is_pci(dev) && >> +#ifdef CONFIG_ARM_AMBA >> + dev->bus != &amba_bustype && >> +#endif >> + dev->bus != &platform_bus_type) >> + return ret == -ENODEV ? 0 : ret; >> + >> dma_addr = offset = 0; >> - size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); >> } else { >> offset = PFN_DOWN(paddr - dma_addr); >> >> @@ -129,6 +129,22 @@ int of_dma_configure(struct device *dev, struct device_node *np) >> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset); >> } >> >> + /* >> + * Set default coherent_dma_mask to 32 bit. Drivers are expected to >> + * setup the correct supported mask. >> + */ >> + if (!dev->coherent_dma_mask) >> + dev->coherent_dma_mask = DMA_BIT_MASK(32); >> + /* >> + * Set it to coherent_dma_mask by default if the architecture >> + * code has not set it. >> + */ >> + if (!dev->dma_mask) >> + dev->dma_mask = &dev->coherent_dma_mask; >> + >> + if (!size) >> + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); >> + >> dev->dma_pfn_offset = offset; >> >> /* >> -- >> 2.13.4.dirty >>
On Tue, Aug 15, 2017 at 5:18 AM, Robin Murphy <robin.murphy@arm.com> wrote: > On 14/08/17 21:08, Rob Herring wrote: >> +linuxppc-dev >> >> On Fri, Aug 11, 2017 at 11:29 AM, Robin Murphy <robin.murphy@arm.com> wrote: >>> Moving DMA configuration to happen later at driver probe time had the >>> unnoticed side-effect that we now perform DMA configuration for *every* >>> device represented in DT, rather than only those explicitly created by >>> the of_platform and PCI code. >>> >>> As Christoph points out, this is not really the best thing to do. Whilst >>> there may well be other DMA-capable buses that can benefit from having >>> their children automatically configured after the bridge has probed, >>> there are also plenty of others like USB, MDIO, etc. that definitely do >>> not support DMA and should not be indiscriminately processed. >>> >>> The good news is that DT already gives us the ammunition to do the right >>> thing - anything lacking a "dma-ranges" property should be considered >>> not to have a mapping of DMA address space from its children to its >>> parent, thus anything for which of_dma_get_range() does not succeed does >>> not need DMA configuration. >>> >>> The bad news is that strictly enforcing that would likely break just >>> about every FDT platform out there, since most authors have either not >>> considered the property at all or have mistakenly assumed that omitting >>> "dma-ranges" is equivalent to including the empty property. Thus we have >>> little choice but to special-case platform, AMBA and PCI devices so they >>> continue to receive configuration unconditionally as before. At least >>> anything new will have to get it right in future... >> >> By "anything new", you mean new buses, not new platforms, right? >> What's a platform bus device today could be a different kernel bus >> type tomorrow with no DT change. So this isn't really enforceable. > > Indeed, it would be virtually impossible to do anything on a > per-platform basis, but I think per-bus is a workable compromise - if > someone changes their hypothetical bus driver in a way that would affect > deployed DTs that can't be updated, at worst they can still add their > new bus type to the special case list at the same time. > >> I don't completely agree that omitting dma-ranges is wrong and that >> new DTs have to have dma-ranges simply because there is much precedent >> of DTs with dma-ranges omitted (just go look at PPC). If a bus has no >> bus to cpu address translation nor size restrictions, then no >> dma-ranges should be allowed. > > Sure, I agree that that genie is never going back in the bottle, but > people seem to manage to get empty "ranges" right to differentiate > between memory-mapped vs. non-memory-mapped buses, so it would be nice > to encourage getting the other direction right as well. Well, they get ranges right because they don't get an address otherwise. The problem on dma-ranges is the bus we describe is the slave side, not the master side. So a given bus may have a mixture of DMA capable devices and non-DMA capable devices. If we're still setting DMA masks on all the devices, what have we gained? Maybe we could get stricter on PCI buses at least. > For the immediate issue at hand, I guess the alternative plan of attack > would be to stick a flag in struct bus_type for the bus drivers > themselves to opt into generic DMA configuration. That at least keeps > everything within the kernel (and come to think of it probably works > neatly for modular bus types as well). I'm fine with the change as is, it's really just the commit text I'm commenting on. Rob
Thanks Robin, I've applied this to the dma-mapping tree and will send it to Linus before the next rc.
On Tue, Aug 15, 2017 at 09:19:50AM -0500, Rob Herring wrote: > > For the immediate issue at hand, I guess the alternative plan of attack > > would be to stick a flag in struct bus_type for the bus drivers > > themselves to opt into generic DMA configuration. That at least keeps > > everything within the kernel (and come to think of it probably works > > neatly for modular bus types as well). > > I'm fine with the change as is, it's really just the commit text I'm > commenting on. Robin, can you resend this with an updated commit text?
diff --git a/drivers/of/device.c b/drivers/of/device.c index 28c38c756f92..e0a28ea341fe 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -89,6 +89,7 @@ int of_dma_configure(struct device *dev, struct device_node *np) bool coherent; unsigned long offset; const struct iommu_ops *iommu; + u64 mask; /* * Set default coherent_dma_mask to 32 bit. Drivers are expected to @@ -134,10 +135,9 @@ int of_dma_configure(struct device *dev, struct device_node *np) * Limit coherent and dma mask based on size and default mask * set by the driver. */ - dev->coherent_dma_mask = min(dev->coherent_dma_mask, - DMA_BIT_MASK(ilog2(dma_addr + size))); - *dev->dma_mask = min((*dev->dma_mask), - DMA_BIT_MASK(ilog2(dma_addr + size))); + mask = DMA_BIT_MASK(ilog2(dma_addr + size - 1) + 1); + dev->coherent_dma_mask &= mask; + *dev->dma_mask &= mask; coherent = of_dma_is_coherent(np); dev_dbg(dev, "device is%sdma coherent\n",
Historically, DMA masks have suffered some ambiguity between whether they represent the range of physical memory a device can access, or the address bits a device is capable of driving, particularly since on many platforms the two are equivalent. Whilst there are some stragglers left (dma_max_pfn(), I'm looking at you...), the majority of DMA code has been cleaned up to follow the latter definition, not least since it is the only one which makes sense once IOMMUs are involved. In this respect, of_dma_configure() has always done the wrong thing in how it generates initial masks based on "dma-ranges". Although rounding down did not affect the TI Keystone platform where dma_addr + size is already a power of two, in any other case it results in a mask which is at best unnecessarily constrained and at worst unusable. BCM2837 illustrates the problem nicely, where we have a DMA base of 3GB and a size of 1GB - 16MB, giving dma_addr + size = 0xff000000 and a resultant mask of 0x7fffffff, which is then insufficient to even cover the necessary offset, effectively making all DMA addresses out-of-range. This has been hidden until now (mostly because we don't yet prevent drivers from simply overwriting this initial mask later upon probe), but due to recent changes elsewhere now shows up as USB being broken on Raspberry Pi 3. Make it right by rounding up instead of down, such that the mask correctly correctly describes all possisble bits the device needs to emit. Fixes: 9a6d7298b083 ("of: Calculate device DMA masks based on DT dma-range size") Reported-by: Stefan Wahren <stefan.wahren@i2se.com> Reported-by: Andreas Färber <afaerber@suse.de> Reported-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- Either of these patches alone should be sufficient to un-break RPi3, and they apply independently, so I'm quite happy for one to go in as a fix now and the other to wait for 4.14. Robin. drivers/of/device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)