Message-ID: <58E72123.4040607@gmail.com>
Date:   Thu, 6 Apr 2017 22:18:27 -0700
From:   Frank Rowand <frowand.list@...il.com>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of: change fixup of dma-ranges size to error

On 04/06/17 15:41, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand <frowand.list@...il.com> wrote:
>> On 04/06/17 07:03, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:18 AM,  <frowand.list@...il.com> wrote:
>>>> From: Frank Rowand <frank.rowand@...y.com>
>>>>
>>>> of_dma_get_range() has workaround code to fixup a device tree that
>>>> incorrectly specified a mask instead of a size for property
>>>> dma-ranges.  That device tree was fixed a year ago in v4.6, so
>>>> the workaround is no longer needed.  Leave a data validation
>>>> check in place, but no longer do the fixup.  Move the check
>>>> one level deeper in the call stack so that other possible users
>>>> of dma-ranges will also be protected.
>>>>
>>>> The fix to the device tree was in
>>>> commit c91cb9123cdd ("dtb: amd: Fix DMA ranges in device tree").
>>>
>>> NACK.
>>> This was by design. You can't represent a size of 2^64 or 2^32.
>>
>> I agree that being unable to represent a size of 2^32 in a u32 and
>> a size of 2^64 in a u64 is the underlying issue.
>>
>> But the code to convert a mask to a size is _not_ design, it is a
>> hack that temporarily worked around a device tree that did not follow
>> the dma-ranges binding in the ePAPR.
> 
> Since when is (2^64 - 1) not a size. It's a perfectly valid size in

I did not say (2^64 -1) is not a size.

I said that the existing code has a hack that converts what is perceived
to be a mask into a size.  The existing code is:

@@ 110,21 @@ void of_dma_configure(struct device *dev, struct device_node *np)
		size = dev->coherent_dma_mask + 1;
	} else {
		offset = PFN_DOWN(paddr - dma_addr);

		/*
		 * Add a work around to treat the size as mask + 1 in case
		 * it is defined in DT as a mask.
		 */
		if (size & 1) {
			dev_warn(dev, "Invalid size 0x%llx for dma-range\n",
				 size);
			size = size + 1;
		}

		if (!size) {
			dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
			return;
		}
		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
	}

Note the comment that says "in case it is defined in DT as a mask."

And as you stated in a review comment is 2015: "Also, we need a WARN
here so DTs get fixed."


> DT. And there's probably not a system in the world that needs access
> to that last byte. Is it completely accurate description if we
> subtract off 1? No, but it is still a valid range (so would be
> subtracting 12345).
> 
>> That device tree was corrected a year ago to provide a size instead of
>> a mask.
> 
> You are letting Linux implementation details influence your DT
> thinking. DT is much more flexible in that it supports a base address
> and size (and multiple of them) while Linux can only deal with a
> single address mask. If Linux dealt with base + size, then we wouldn't

No.  of_dma_get_range() returns two addresses and a size from the
dma-ranges property, just as it is defined in the spec.

of_dma_configure() then interprets an odd size as meaning that the
device tree incorrectly contains a mask, and then converts that mask
to a size by adding one to it.  Linux is _still_ using address and
size at this point.  It does _not_ convert this size into a mask,
but instead passes size on into arch_setup_dma_ops().

The proposed patch is to quit accepting a mask as valid data in
dma-ranges.


> be having this conversation. As long as Linux only deals with masks,
> we're going to have to have some sort of work-around to deal with
> them.
> 
>>> Well, technically you can for the latter, but then you have to grow
>>> #size-cells to 2 for an otherwise all 32-bit system which seems kind
>>> of pointless and wasteful. You could further restrict this to only
>>> allow ~0 and not just any case with bit 0 set.
>>>
>>> I'm pretty sure AMD is not the only system. There were 32-bit systems too.
>>
>> I examined all instances of property dma-ranges in in tree dts files in
>> Linux 4.11-rc1.  There are none that incorrectly specify mask instead of
>> size.
> 
> Okay, but there are ones for ranges at least. See ecx-2000.dts.

The patch does not impact the ranges property.  It only impacts the
dma-ranges property.

> 
>> #size-cells only changes to 2 for the dma-ranges property and the ranges
>> property when size is 2^32, so that is a very small amount of space.
>>
>> The patch does not allow for a size of 2^64.  If a system requires a
>> size of 2^64 then the type of size needs to increase to be larger
>> than a u64.  If you would like for the code to be defensive and
>> detect a device tree providing a size of 2^64 then I can add a
>> check to of_dma_get_range() to return -EINVAL if #size-cells > 2.
>> When that error triggers, the type of size can be changed.
> 
> #size-cells > 2 is completely broken for anything but PCI. I doubt it

Yes, that is what I said.  The current code does not support #size-cells > 2
for dma-ranges.

#size-cells > 2 for dma-ranges will lead to a problem in
of_dma_get_range(), which stuffs the value of the size into a u64.
Clearly, a 3 cell size will not fit into a u64.


> is easily fixed without some special casing (i.e. a different hack)
> until we have 128-bit support. I hope to retire before we need to
> support that.
> 
> Rob
> 

Can we get back to the basic premise of the proposed patch?

The current code in of_dma_configure() contains a hack that allows the
dma-ranges property to specify a mask instead of a size.  The binding
in the specification allows a size and does not allow a mask.

The hack was added to account for one or more dts files that did not
follow the specification.  In the mail list discussion of the hack
you said "Also, we need a WARN here so DTs get fixed."

The hack was first present in Linux 4.1.  The only in-tree dts that
incorrectly contained a mask instead of a size in dma-ranges was
arch/arm64/boot/dts/amd/amd-seattle-soc.dtsi

That .dtsi was fixed by
commit c91cb9123cdd ("dtb: amd: Fix DMA ranges in device tree")
The fix was present in Linux 4.6, May 15, 2016.

I would like to remove the hack.  I think that enough time has
elapsed to allow this change.

-Frank

Powered by blists - more mailing lists