* Re: [PATCH] of: Fix DMA mask assignment
[not found] ` <CAL_JsqKAuyePWUC-K21uZEjBU29EZA3WUQOYQdwAZUMOz+mEFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-03-22 16:46 ` Russell King - ARM Linux
2017-03-22 17:57 ` Robin Murphy
1 sibling, 0 replies; 4+ messages in thread
From: Russell King - ARM Linux @ 2017-03-22 16:46 UTC (permalink / raw)
To: Rob Herring
Cc: Robin Murphy, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Murali Karicheri, Frank Rowand, Arnd Bergmann,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Wed, Mar 22, 2017 at 10:19:58AM -0500, Rob Herring wrote:
> On Tue, Mar 7, 2017 at 12:12 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> > As we start moving towards mandating that device DMA addressing
> > capabilities are correctly described in DT for platforms like arm64,
>
> Why? There's no point to dma-ranges if the bus is not restricting the
> device. dma-ranges is supposed to apply to a bus, not a device. For
> example, using dma-ranges for a 32-bit device on a 64-bit platform is
> wrong. To put it another way, devices know their addressing
> capability, so we don't need to describe that in DT, only external
> restrictions outside of the device.
>
> > it
> > turns out that actually assigning DMA masks wider than 32 bits from
> > "dma-ranges" is made largely impossible by the current code. New PCI
> > devices are passed in with 32-bit masks, while new platform devices are
> > passed in with their masks unset, so we install default 32-bit masks for
> > them. As a result, subsequently taking the minimum between any existing
> > mask and the size given by "dma-ranges" means that we can never exceed
> > 32 bits for the vast majority of devices.
> >
> > Since the DT can be assumed to describe the hardware appropriately, and
> > the device's driver is expected by the DMA API to explicitly set a mask
> > later when it probes, we can safely let "dma-ranges" unconditionally
> > override any initial mask from device creation (and as a small bonus
> > deduplicate the calculation in the process).
> >
> > CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> > Fixes: 9a6d7298b083 ("of: Calculate device DMA masks based on DT dma-range size")
>
> Changing behavior since 4.1 doesn't really seem like a fix. A fix
> should generally be tagged for stable and I'd be nervous on this one.
>
> Are we sure all the things a driver may do still work the same? A
> driver changing the default 32-bit mask to a 64-bit mask for example.
I'll re-iterate about the DMA API requirements. All drivers that perform
DMA are expected to perform DMA mask negotiation with the architecture
code. That means all drivers should use:
dma_set_mask()
if they intend to use the streaming DMA APIs,
dma_set_coherent_mask()
if they intend to use the coherent DMA API, or
dma_set_mask_and_coherent()
if they intend to use both APIs. Drivers that omit these calls are
buggy.
It is also required that drivers check the return value from these
calls, since failure means that (eg) they are not allowed to perform
64-bit DMA.
The mask that the driver passes in to these functions is determined
by two things:
(a) how many address lines the device supports
(b) the addressing mode that the driver wishes to enable for the device
For example, on PCI, to use 64-bit DMA, DAC addressing is necessary.
A PCI driver must have successfully negotiated a >32-bit DMA mask in
order to use DAC addressing, as this indicates whether the upstream
supports DAC addressing cycles.
There may be other restrictions that the device driver is unaware of.
So, these three functions are part of a negotiation that drivers are
expected to perform, and moving this information into DT does not
negate the need for drivers to perform this negotiation.
Drivers that use the DMA mask coerce() functions are fundamentally
buggy drivers that either (a) didn't set a DMA mask before there was
code in DT/elsewhere to provide the default mask, or (b) are overriding
the negotiation mechanism. No new drivers should ever use the DMA mask
coerce functions.
Hope this helps.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] of: Fix DMA mask assignment
[not found] ` <CAL_JsqKAuyePWUC-K21uZEjBU29EZA3WUQOYQdwAZUMOz+mEFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-03-22 16:46 ` Russell King - ARM Linux
@ 2017-03-22 17:57 ` Robin Murphy
1 sibling, 0 replies; 4+ messages in thread
From: Robin Murphy @ 2017-03-22 17:57 UTC (permalink / raw)
To: Rob Herring
Cc: Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Arnd Bergmann, Murali Karicheri
Hi Rob,
On 22/03/17 15:19, Rob Herring wrote:
> On Tue, Mar 7, 2017 at 12:12 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>> As we start moving towards mandating that device DMA addressing
>> capabilities are correctly described in DT for platforms like arm64,
>
> Why? There's no point to dma-ranges if the bus is not restricting the
> device. dma-ranges is supposed to apply to a bus, not a device. For
> example, using dma-ranges for a 32-bit device on a 64-bit platform is
> wrong. To put it another way, devices know their addressing
> capability, so we don't need to describe that in DT, only external
> restrictions outside of the device.
The "mandating dma-ranges" part was something that Arnd had mentioned,
so my interpretation of the rationale might differ slightly, but I
generally didn't disagree. Perhaps my wording is at fault here, but by
"device addressing capabilities" I do mean the capabilities of the
device as integrated into the system, (i.e. whatever interconnect it's
attached to) rather than whatever might be inherent to the device
itself. Nobody's suggesting we start using dma-ranges in some way other
than its existing definition, just that we should put it on any bus
which is not 32 bits wide (in the manner of e.g. amd-seattle-soc.dtsi).
A secondary issue here is that ePAPR/DTSpec isn't particularly clear
about dma-ranges, or more specifically what its absence implies. The
assumption which seems to exist through much of the (PPC) OF code is
that it should be present on any bus which allows DMA through to its
parent, but then for FDT at least we end up sort-of respecting that but
mostly ignoring it such that missing dma-ranges is assumed equivalent to
empty dma-ranges (and thus we also have no way to actually say "children
of this bus that might think they can do DMA, can't").
Now, "devices know their addressing capability" is precisely the problem
that the original commit referenced here was trying to tackle, but the
code turns out to only work correctly in the case where the dma-ranges
size is *less* than 32 bits (it's 31 bits on Keystone, which was the
original platform in question). More generally on 64-bit systems we're
ending up with configurations like a 64-bit NVMe device on PCIe behind
an IOMMU with 48-bit-wide interfaces, neither of which are aware that
only 32/44/etc. bits of address are wired up on the SoC buses in between
it all, and truncation-based hilarity ensues. In order to make use of
dma-ranges as the correct tool for the job, this first wants un-breaking
to properly interpret buses with n-bit limitations where 32 < n < 64.
Then we can actually start preventing drivers from setting wider masks
where a bus limitation exists.
The problem that arises then is what to do when no range is described -
do we change the default mask to 64 bits to represent "no limit" and let
any remaining (bad) drivers relying implicitly on the (deeply-ingrained)
32-bit default start subtly going wrong, or do we play it safe and stick
with the 32-bit default, but with the side effect that nobody can then
DMA above 4GB unless they set an explicitly larger dma-ranges. I believe
the latter was what Arnd was in favour of, and which is alluded to in
this commit message, but in truth I'm equally happy to take either
approach. What I *don't* want is to end up with a bunch of
special-casing where we sometimes allow drivers to enlarge their
device's DMA mask and sometimes don't depending on DT vs. ACPI, some
property present vs. absent, wind direction etc.
>> it
>> turns out that actually assigning DMA masks wider than 32 bits from
>> "dma-ranges" is made largely impossible by the current code. New PCI
>> devices are passed in with 32-bit masks, while new platform devices are
>> passed in with their masks unset, so we install default 32-bit masks for
>> them. As a result, subsequently taking the minimum between any existing
>> mask and the size given by "dma-ranges" means that we can never exceed
>> 32 bits for the vast majority of devices.
>>
>> Since the DT can be assumed to describe the hardware appropriately, and
>> the device's driver is expected by the DMA API to explicitly set a mask
>> later when it probes, we can safely let "dma-ranges" unconditionally
>> override any initial mask from device creation (and as a small bonus
>> deduplicate the calculation in the process).
>>
>> CC: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
>> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>> Fixes: 9a6d7298b083 ("of: Calculate device DMA masks based on DT dma-range size")
>
> Changing behavior since 4.1 doesn't really seem like a fix. A fix
> should generally be tagged for stable and I'd be nervous on this one.
Fair enough - some maintainers seem to like fixes tags on anything that
purely corrects something introduced by a previous patch, even if
non-critical, and I don't always guess right :) FWIW I wouldn't consider
this one for stable either (I'd have added the tag myself if so).
> Are we sure all the things a driver may do still work the same? A
> driver changing the default 32-bit mask to a 64-bit mask for example.
Yes, the only change here is that if dma-ranges is present, then its
actual specified size will be now propagated to a newly created device's
initial DMA mask, as opposed to being munged with a fixed 32 bits and/or
whatever the bus code set (which is generally also 32 bits) along the
way. The fact that drivers will then unconditionally stomp on that mask
when they probe with whatever larger or smaller mask they "know" is
appropriate remains unchanged - it's just that however we start tackling
*that* problem we need the dma-ranges info to get through unmolested in
the first place.
(It transpires that we also fail to handle it at all for PCI host
bridges in FDT where we don't have OF nodes for the children, but that's
something else; I'm looking into it.)
Robin.
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>> drivers/of/device.c | 11 ++---------
>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index b1e6bebda3f3..9bb8518b28f2 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -129,15 +129,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>> }
>>
>> dev->dma_pfn_offset = offset;
>> -
>> - /*
>> - * 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)));
>> + dev->coherent_dma_mask = DMA_BIT_MASK(ilog2(dma_addr + size));
>> + *dev->dma_mask = dev->coherent_dma_mask;
>>
>> coherent = of_dma_is_coherent(np);
>> dev_dbg(dev, "device is%sdma coherent\n",
>> --
>> 2.11.0.dirty
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread