Re: [PATCH] of: change fixup of dma-ranges size to error

[Robin Murphy]

On 07/04/17 18:09, Rob Herring wrote:
> + Robin, Sricharan
> 
> On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
>> On 04/06/17 15:41, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
 On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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."
> 
> Indeed. I agree that "let me put a mask in the DT so Linux (at some
> version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
> should be allowed to avoid growing #size-cells and because
> #size-cells=3 doesn't work.

Realistically, in the context of dma-ranges, the only case we might ever
actually need to support is 2^32 with #size-cells=1, i.e. an 32-bit
child bus mastering onto a n>32-bit parent bus at some non-zero offset.
The only way a size of 2^64 would be valid for Linux-capable hardware
would be for a 64-bit child bus mastering onto a 64-bit parent bus with
no address offset, and that can be expressed by simply leaving the
property empty.

>>> 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().
> 
> It doesn't really matter where in the implementation, but at some
> point we end up with only a mask in Linux was my point.

Note that of_dma_configure() *does* use the size itself to generate the
device's default DMA mask, but also passes it unmodified to
arch_setup_dma_ops() to potentially do finer-grained things with as
well. FWIW there exist plenty of things for which a single mask doesn't
really cut it - it's already pretty busted for cases when base + size
(legitimately) doesn't come out to a 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Robin Murphy]

On 07/04/17 18:09, Rob Herring wrote:
> + Robin, Sricharan
> 
> On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
>> On 04/06/17 15:41, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
 On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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."
> 
> Indeed. I agree that "let me put a mask in the DT so Linux (at some
> version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
> should be allowed to avoid growing #size-cells and because
> #size-cells=3 doesn't work.

Realistically, in the context of dma-ranges, the only case we might ever
actually need to support is 2^32 with #size-cells=1, i.e. an 32-bit
child bus mastering onto a n>32-bit parent bus at some non-zero offset.
The only way a size of 2^64 would be valid for Linux-capable hardware
would be for a 64-bit child bus mastering onto a 64-bit parent bus with
no address offset, and that can be expressed by simply leaving the
property empty.

>>> 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().
> 
> It doesn't really matter where in the implementation, but at some
> point we end up with only a mask in Linux was my point.

Note that of_dma_configure() *does* use the size itself to generate the
device's default DMA mask, but also passes it unmodified to
arch_setup_dma_ops() to potentially do finer-grained things with as
well. FWIW there exist plenty of things for which a single mask doesn't
really cut it - it's already pretty busted for cases when base + size
(legitimately) doesn't come out to a power of two, let alone when there
are multiple ranges with unusable gaps in-between.

>From 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/10/17 04:48, Sricharan R wrote:
> Hi Frank,
> 
> 
> 
 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.
>>>
>>> If we have no cases of what I'm concerned about, then removing it is
>>> fine. Is this a dependency for iommu series? Doesn't look like it to
>>> me.
>>
>> This patch is a replacement for patch 03/12 in the iommu series.  I
>> think that patch 03/12 of the iommu series could be dropped and my
>> patch could be applied independently of the iommu series.
>>
>> There is likely a conflict between my patch and patch 06/12 of the
>> iommu series because in my patch the first line of the patch chunk
>> of drivers/of/device.c includes a line that is changed in 06/12
>> of the iommu series.  If this is the case then the iommu series
>> should take precedence over my patch (and I should subsequently
>> fixup my patch).
>>
> 
> Ok, for which i just posted a V11 [1] with patch 03/12 from
> V10 dropped.
> 
> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1373638.html
> 
> Regards,
>  Sricharan
> 

Thanks.  I'll revisit this patch after the iommu series gets merged.

-Frank

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/10/17 04:48, Sricharan R wrote:
> Hi Frank,
> 
> 
> 
 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.
>>>
>>> If we have no cases of what I'm concerned about, then removing it is
>>> fine. Is this a dependency for iommu series? Doesn't look like it to
>>> me.
>>
>> This patch is a replacement for patch 03/12 in the iommu series.  I
>> think that patch 03/12 of the iommu series could be dropped and my
>> patch could be applied independently of the iommu series.
>>
>> There is likely a conflict between my patch and patch 06/12 of the
>> iommu series because in my patch the first line of the patch chunk
>> of drivers/of/device.c includes a line that is changed in 06/12
>> of the iommu series.  If this is the case then the iommu series
>> should take precedence over my patch (and I should subsequently
>> fixup my patch).
>>
> 
> Ok, for which i just posted a V11 [1] with patch 03/12 from
> V10 dropped.
> 
> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1373638.html
> 
> Regards,
>  Sricharan
> 

Thanks.  I'll revisit this patch after the iommu series gets merged.

-Frank

Re: [PATCH] of: change fixup of dma-ranges size to error

[Sricharan R]

Hi Frank,




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.


If we have no cases of what I'm concerned about, then removing it is
fine. Is this a dependency for iommu series? Doesn't look like it to
me.


This patch is a replacement for patch 03/12 in the iommu series.  I
think that patch 03/12 of the iommu series could be dropped and my
patch could be applied independently of the iommu series.

There is likely a conflict between my patch and patch 06/12 of the
iommu series because in my patch the first line of the patch chunk
of drivers/of/device.c includes a line that is changed in 06/12
of the iommu series.  If this is the case then the iommu series
should take precedence over my patch (and I should subsequently
fixup my patch).



Ok, for which i just posted a V11 [1] with patch 03/12 from
V10 dropped.

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1373638.html

Regards,
 Sricharan

--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of: change fixup of dma-ranges size to error

[Sricharan R]

Hi Frank,




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.


If we have no cases of what I'm concerned about, then removing it is
fine. Is this a dependency for iommu series? Doesn't look like it to
me.


This patch is a replacement for patch 03/12 in the iommu series.  I
think that patch 03/12 of the iommu series could be dropped and my
patch could be applied independently of the iommu series.

There is likely a conflict between my patch and patch 06/12 of the
iommu series because in my patch the first line of the patch chunk
of drivers/of/device.c includes a line that is changed in 06/12
of the iommu series.  If this is the case then the iommu series
should take precedence over my patch (and I should subsequently
fixup my patch).



Ok, for which i just posted a V11 [1] with patch 03/12 from
V10 dropped.

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1373638.html

Regards,
 Sricharan

--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of 
Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/07/17 10:09, Rob Herring wrote:
> + Robin, Sricharan
> 
> On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
>> On 04/06/17 15:41, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
 On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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."
> 
> Indeed. I agree that "let me put a mask in the DT so Linux (at some
> version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
> should be allowed to avoid growing #size-cells and because
> #size-cells=3 doesn't work.
> 
>>> 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().
> 
> It doesn't really matter where in the implementation, but at some
> point we end up with only a mask in Linux was my point.
> 
>> 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 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/07/17 10:09, Rob Herring wrote:
> + Robin, Sricharan
> 
> On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
>> On 04/06/17 15:41, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
 On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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."
> 
> Indeed. I agree that "let me put a mask in the DT so Linux (at some
> version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
> should be allowed to avoid growing #size-cells and because
> #size-cells=3 doesn't work.
> 
>>> 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().
> 
> It doesn't really matter where in the implementation, but at some
> point we end up with only a mask in Linux was my point.
> 
>> 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 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

+ Robin, Sricharan

On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
> On 04/06/17 15:41, Rob Herring wrote:
>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
>>> On 04/06/17 07:03, Rob Herring wrote:
 On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
> From: Frank Rowand 
>
> 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."

Indeed. I agree that "let me put a mask in the DT so Linux (at some
version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
should be allowed to avoid growing #size-cells and because
#size-cells=3 doesn't work.

>> 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().

It doesn't really matter where in the implementation, but at some
point we end up with only a mask in Linux was my point.

> 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.

Yes, I know. I'm only pointing out we 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

+ Robin, Sricharan

On Fri, Apr 7, 2017 at 12:18 AM, Frank Rowand  wrote:
> On 04/06/17 15:41, Rob Herring wrote:
>> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
>>> On 04/06/17 07:03, Rob Herring wrote:
 On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
> From: Frank Rowand 
>
> 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."

Indeed. I agree that "let me put a mask in the DT so Linux (at some
version) works" is wrong. I still think (2^32 - 1) and (2^64 - 1)
should be allowed to avoid growing #size-cells and because
#size-cells=3 doesn't work.

>> 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().

It doesn't really matter where in the implementation, but at some
point we end up with only a mask in Linux was my point.

> 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.

Yes, I know. I'm only pointing out we have other cases of size=~0 to
avoid growing #size-cells.

>>> #size-cells only changes to 2 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/06/17 15:41, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
>> On 04/06/17 07:03, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
 From: Frank Rowand 

 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 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/06/17 15:41, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
>> On 04/06/17 07:03, Rob Herring wrote:
>>> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
 From: Frank Rowand 

 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 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
> On 04/06/17 07:03, Rob Herring wrote:
>> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>>> From: Frank Rowand 
>>>
>>> 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
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
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.

> #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
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

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

On Thu, Apr 6, 2017 at 1:37 PM, Frank Rowand  wrote:
> On 04/06/17 07:03, Rob Herring wrote:
>> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>>> From: Frank Rowand 
>>>
>>> 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
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
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.

> #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
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

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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.

That device tree was corrected a year ago to provide a size instead of
a mask.

> 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.

#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.

> 
> Rob
> 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Frank Rowand]

On 04/06/17 07:03, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
>> From: Frank Rowand 
>>
>> 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.

That device tree was corrected a year ago to provide a size instead of
a mask.

> 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.

#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.

> 
> Rob
> 

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
> From: Frank Rowand 
>
> 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.
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.

Rob

Re: [PATCH] of: change fixup of dma-ranges size to error

[Rob Herring]

On Thu, Apr 6, 2017 at 1:18 AM,   wrote:
> From: Frank Rowand 
>
> 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.
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.

Rob

[PATCH] of: change fixup of dma-ranges size to error

[frowand . list]

From: Frank Rowand 

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").

Signed-off-by: Frank Rowand 
---
 drivers/of/address.c | 12 +++-
 drivers/of/device.c  | 15 ---
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..dae98923968f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -829,6 +829,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, 
u64 *paddr, u64 *siz
int len, naddr, nsize, pna;
int ret = 0;
u64 dmaaddr;
+   u64 tmp_size;
 
if (!node)
return -EINVAL;
@@ -879,7 +880,16 @@ int of_dma_get_range(struct device_node *np, u64 
*dma_addr, u64 *paddr, u64 *siz
}
*dma_addr = dmaaddr;
 
-   *size = of_read_number(ranges + naddr + pna, nsize);
+   tmp_size = of_read_number(ranges + naddr + pna, nsize);
+
+   /* check if mask specified instead of size */
+   if (tmp_size & 1) {
+   pr_debug("invalid dma-range size in node: %s\n", np->full_name);
+   ret = -EINVAL;
+   goto out;
+   }
+
+   *size = tmp_size;
 
pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
 *dma_addr, *paddr, *size);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index b1e6bebda3f3..09dedd045007 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -110,21 +110,6 @@ 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);
}
 
-- 
Frank Rowand 

[PATCH] of: change fixup of dma-ranges size to error

[frowand . list]

From: Frank Rowand 

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").

Signed-off-by: Frank Rowand 
---
 drivers/of/address.c | 12 +++-
 drivers/of/device.c  | 15 ---
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 02b2903fe9d2..dae98923968f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -829,6 +829,7 @@ int of_dma_get_range(struct device_node *np, u64 *dma_addr, 
u64 *paddr, u64 *siz
int len, naddr, nsize, pna;
int ret = 0;
u64 dmaaddr;
+   u64 tmp_size;
 
if (!node)
return -EINVAL;
@@ -879,7 +880,16 @@ int of_dma_get_range(struct device_node *np, u64 
*dma_addr, u64 *paddr, u64 *siz
}
*dma_addr = dmaaddr;
 
-   *size = of_read_number(ranges + naddr + pna, nsize);
+   tmp_size = of_read_number(ranges + naddr + pna, nsize);
+
+   /* check if mask specified instead of size */
+   if (tmp_size & 1) {
+   pr_debug("invalid dma-range size in node: %s\n", np->full_name);
+   ret = -EINVAL;
+   goto out;
+   }
+
+   *size = tmp_size;
 
pr_debug("dma_addr(%llx) cpu_addr(%llx) size(%llx)\n",
 *dma_addr, *paddr, *size);
diff --git a/drivers/of/device.c b/drivers/of/device.c
index b1e6bebda3f3..09dedd045007 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -110,21 +110,6 @@ 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);
}
 
-- 
Frank Rowand