On 2022-04-11 13:54, Christoph Hellwig wrote:
On Mon, Apr 11, 2022 at 06:47:44PM +0900, Wangseok Lee wrote:
�Therefore,�the�dma_set_mask(32)(in�dw_pcie_host_init())
�was�modified�to�be�performed�only�when
�the�dev-dma_mask�is�not�set�larger�than�32�bits.
So�what�sets�dev->dma_mask�to�a�larger�than�32-bit�value�here?
We�need�to�find�and�fix�that.
At the code of of_dma_configure_id() of driver/of/device.c..
In the 64bit system, if the dma start addr is used as 0x1'0000'0000
and the size is used as 0xf'0000'0000, "u64 end" is 0xf'ffff'ffff.
And the dma_mask value is changed from 0xffff'ffff'ffff'ffff to
0xf'ffff'ffffff due to the code below.
That does look rather wrong to me, as any limitation should only
be in the bus mask. Unless I'm missing something (and Robin should
know the code much better than I do) we should do something like
the patch below:
Yeah, there's some smelly history here... Originally, of_dma_configure()
pre-set the masks as an attempt to impose any restriction represented by
DT "dma-ranges" - the platform it was implemented in aid of happened to
have a 31-bit DMA range, which may well have coloured some implicit
assumptions. IIRC, when I first implemented the separate bus_dma_mask to
properly solve the general constraint problem, I left the other
mask-setting in place since even though it shouldn't have served any
purpose any more, I figured it wasn't actively harmful, and by that
point it had been around long enough that I was a little wary of opening
a can of worms if anything *had* erroneously started relying on it.
I'm not against making the change now though - I'm about to get to the
point of turning all the dma_configure stuff inside-out in the course of
my IOMMU rework anyway, so I fully expect to be breaking things and
picking up the pieces. Getting this in first so it's easily bisectable
and leaves me less code to further break seems most sensible :)
If you're happy to write up the patch, please also do the equivalent for
acpi_arch_dma_setup() too.
This is all orthogonal to why the original patch in this thread is
wrong, though. If the pcie-designware driver could somehow guarantee
that all endpoint functions present, or able to appear later, can handle
MSI addresses of some width >32, then it could set its DMA mask for the
fake DMA mapping accordingly, but that has nothing at all to do with how
many address bits might happen to be wired up on the external AXI interface.
Robin.
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 874f031442dc7..b197861fcde08 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -113,8 +113,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
{
const struct iommu_ops *iommu;
const struct bus_dma_region *map = NULL;
- u64 dma_start = 0;
- u64 mask, end, size = 0;
+ u64 dma_start = 0, size = 0;
bool coherent;
int ret;
@@ -156,6 +155,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
kfree(map);
return -EINVAL;
}
+
+ dev->bus_dma_limit = dma_start + size - 1;
+ dev->dma_range_map = map;
}
/*
@@ -169,25 +171,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
dev->dma_mask = &dev->coherent_dma_mask;
}
- if (!size && dev->coherent_dma_mask)
- size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
- else if (!size)
- size = 1ULL << 32;
-
- /*
- * Limit coherent and dma mask based on size and default mask
- * set by the driver.
- */
- end = dma_start + size - 1;
- mask = DMA_BIT_MASK(ilog2(end) + 1);
- dev->coherent_dma_mask &= mask;
- *dev->dma_mask &= mask;
- /* ...but only set bus limit and range map if we found valid dma-ranges earlier */
- if (!ret) {
- dev->bus_dma_limit = end;
- dev->dma_range_map = map;
- }
-
coherent = of_dma_is_coherent(np);
dev_dbg(dev, "device is%sdma coherent\n",
coherent ? " " : " not ");
