Re: dma_mask limited to 32-bits with OF platform device

From: Roger Quadros
Date: Wed Feb 12 2020 - 07:33:41 EST

On 12/02/2020 13:37, Robin Murphy wrote:
On 2020-02-12 10:49 am, Roger Quadros wrote:
Hi,

I'd like to understand why of_dma_configure() is limiting the dma and coherent masks
instead of overriding them.

see commits
a5516219b102 ("of/platform: Initialise default DMA masks")
ee7b1f31200d ("of: fix DMA mask generation")

In of_platform_device_create_pdata(), we initialize both masks to 32-bits unconditionally,
which is fine to support legacy cases.

ÂÂÂÂÂdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
ÂÂÂÂÂÂÂÂ if (!dev->dev.dma_mask)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev->dev.dma_mask = &dev->dev.coherent_dma_mask;

Then in of_dma_configure() we limit it like so.

ÂÂÂÂÂÂÂÂ dev->coherent_dma_mask &= mask;
ÂÂÂÂÂÂÂÂ *dev->dma_mask &= mask;

This way, legitimate devices which correctly set dma-ranges in DT
will never get a dma_mask above 32-bits at all. How is this expected to work?

Because these are still just the *default* masks - although drivers are all expected to call dma_set_mask() and friends explicitly nowadays, there are sure to be some out there for 32-bit devices still assuming the default 32-bit masks are in place. Thus if platform code secretly makes them larger, Bad Things ensue. Making them *smaller* where there are platform limitations shouldn't really matter now that we have the bus_dma_limit mechanism working well, but also doesn't do any harm, so it was left in for good measure.

The current paradigm is that the device masks represent the inherent capability of the device as far as the driver knows, and external interconnect constraints are kept separately as private DMA API internals via the bus limit.

For a test, I added this in dra7.dtsi sata node. (NOTE: CONFIG_ARM_LPAE=y)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 93aa65c75b45..cd8c6cea23d5 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -571,6 +571,8 @@
ÂÂÂÂÂÂÂÂÂ sata: sata@4a141100 {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ compatible = "snps,dwc-ahci";
ÂÂÂÂÂÂÂÂÂÂÂÂÂ reg = <0x4a140000 0x1100>, <0x4a141100 0x7>;
+ÂÂÂÂÂÂÂÂÂÂÂ #size-cells = <2>;
+ÂÂÂÂÂÂÂÂÂÂÂ dma-ranges = <0x00000000 0x00000000 0x10 0x00000000>;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ phys = <&sata_phy>;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ phy-names = "sata-phy";

----------------------------- drivers/of/device.c -----------------------------
index e9127db7b067..1072cebad57a 100644
@@ -95,6 +95,7 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
ÂÂÂÂÂ const struct iommu_ops *iommu;
ÂÂÂÂÂ u64 mask, end;

+ÂÂÂ dev_info(dev, "of_dma_configure\n");
ÂÂÂÂÂ ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
ÂÂÂÂÂ if (ret < 0) {
ÂÂÂÂÂÂÂÂÂ /*
@@ -123,7 +124,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
ÂÂÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
ÂÂÂÂÂÂÂÂÂ }
-ÂÂÂÂÂÂÂ dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+ÂÂÂÂÂÂÂ dev_info(dev, "dma %llx paddr %llx size %llx\n", dma_addr, paddr, size);
+ÂÂÂÂÂÂÂ dev_info(dev, "dma_pfn_offset(%#08lx)\n", offset);
ÂÂÂÂÂ }

ÂÂÂÂÂ /*
@@ -152,6 +154,8 @@ int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma)
ÂÂÂÂÂ mask = DMA_BIT_MASK(ilog2(end) + 1);
ÂÂÂÂÂ dev->coherent_dma_mask &= mask;
ÂÂÂÂÂ *dev->dma_mask &= mask;
+
+ÂÂÂ dev_info(dev, "end %llx, mask %llx\n", end, mask);
ÂÂÂÂÂ /* ...but only set bus limit if we found valid dma-ranges earlier */
ÂÂÂÂÂ if (!ret)
ÂÂÂÂÂÂÂÂÂ dev->bus_dma_limit = end;

And I see.

[ÂÂÂ 1.134294]Â 4a140000.sata: of_platform
[ÂÂ 13.203917] ahci 4a140000.sata: of_dma_configure
[ÂÂ 13.225635] ahci 4a140000.sata: dma 0 paddr 0 size 1000000000
[ÂÂ 13.266178] ahci 4a140000.sata: dma_pfn_offset(0x000000)
[ÂÂ 13.297621] ahci 4a140000.sata: end fffffffff, mask fffffffff
[ÂÂ 13.585499] ahci 4a140000.sata: dma_mask 0xffffffff, coherent_mask 0xffffffff
[ÂÂ 13.599082] ahci 4a140000.sata: setting 64-bit mask ffffffffffffffff

Truncation of dma_mask and coherent_mask is undesired in this case.

Again, it's only the initial default masks that are truncated, and the driver appropriately replaces them with its 64-bit masks anyway once it probes. However, bus_dma_limit should still reflect that 36-bit upstream constraint, and that's what really matters. If you've found a path through a DMA API implementation which is subsequently failing to respect that limit, that's a bug in that implementation.


For now, let's say that we limit dma-ranges to 4GB size. with "dma-ranges = <0x00000000 0x00000000 0x1 0x00000000>;"
Then, dma_bus_limit is set correctly to 0xffffffff, SATA driver sets masks to 64-bit as IP supports that.

[ 13.306847] ahci 4a140000.sata: dma_mask 0xffffffffffffffff, coherent_mask 0xffffffffffffffff, dma_bus_limit 0xffffffff

However, the SATA controller still tries to do DMA above 32-bits.
dma_alloc() doesn't seem to be taking dma_bus_limit into account?

How about fixing it like so?

-ÂÂÂÂ dev->coherent_dma_mask &= mask;
-ÂÂÂ *dev->dma_mask &= mask;
+ÂÂÂÂ dev->coherent_dma_mask = mask;
+ÂÂÂÂ *dev->dma_mask = mask;

As above, making the "32-bit default" larger than 32 bits stands to break 32-bit devices with old drivers, and there's nothing to "fix" at this point anyway.

Also this comment doesn't make sense anymore?

ÂÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂÂ * Limit coherent and dma mask based on size and default mask
ÂÂÂÂÂÂÂÂÂ * set by the driver.
ÂÂÂÂÂÂÂÂÂ */

TBH that's never made much sense, unless "driver" was supposed to refer to bus code. Its continued presence is down to inertia more than any other reason :)

Robin.

--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki