Hi Rob/Robin, On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote: > From: Robin Murphy <robin.mur...@arm.com> > > Since the "dma-ranges" property is only valid for a node representing a > bus, of_dma_get_range() currently assumes the node passed in is a leaf > representing a device, and starts the walk from its parent. In cases > like PCI host controllers on typical FDT systems, however, where the PCI > endpoints are probed dynamically the initial leaf node represents the > 'bus' itself, and this logic means we fail to consider any "dma-ranges" > describing the host bridge itself. Rework the logic such that > of_dma_get_range() also works correctly starting from a bus node > containing "dma-ranges". > > While this does mean "dma-ranges" could incorrectly be in a device leaf > node, there isn't really any way in this function to ensure that a leaf > node is or isn't a bus node.
Sorry, I'm not totally sure if this is what you're pointing out with the last
sentence. But, what about the case of a bus configuring a device which also
happens to be a memory mapped bus (say a PCI platform device). It'll get it's
dma config based on its own dma-ranges which is not what we want.
> Signed-off-by: Robin Murphy <robin.mur...@arm.com>
> [robh: Allow for the bus child node to still be passed in]
> Signed-off-by: Rob Herring <r...@kernel.org>
> ---
> Resending, hit send too quickly.
>
> v2:
> - Ensure once we find dma-ranges, every parent has it.
I like this new approach.
Regards,
Nicolas
> - Only get the #{size,address}-cells after we find non-empty dma-ranges
> - Add a check on the 'dma-ranges' length
>
> This is all that remains of the dma-ranges series. I've applied the rest
> of the series prep and fixes. I dropped "of: Ratify of_dma_configure()
> interface" as the assertions that the node pointer being the parent only
> when struct device doesn't have a DT node pointer is not always
> true.
>
> I didn't include any tested-bys as this has changed a bit. A git branch
> is here[1].
>
> Rob
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dma-masks-v2
>
> drivers/of/address.c | 44 ++++++++++++++++++--------------------------
> 1 file changed, 18 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 5ce69d026584..99c1b8058559 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -930,47 +930,39 @@ int of_dma_get_range(struct device_node *np, u64
> *dma_addr, u64 *paddr, u64 *siz
> const __be32 *ranges = NULL;
> int len, naddr, nsize, pna;
> int ret = 0;
> + bool found_dma_ranges = false;
> u64 dmaaddr;
>
> - if (!node)
> - return -EINVAL;
> -
> - while (1) {
> - struct device_node *parent;
> -
> - naddr = of_n_addr_cells(node);
> - nsize = of_n_size_cells(node);
> -
> - parent = __of_get_dma_parent(node);
> - of_node_put(node);
> -
> - node = parent;
> - if (!node)
> - break;
> -
> + while (node) {
> ranges = of_get_property(node, "dma-ranges", &len);
>
> /* Ignore empty ranges, they imply no translation required */
> if (ranges && len > 0)
> break;
>
> - /*
> - * At least empty ranges has to be defined for parent node if
> - * DMA is supported
> - */
> - if (!ranges)
> - break;
> + /* Once we find 'dma-ranges', then a missing one is an error */
> + if (found_dma_ranges && !ranges) {
> + ret = -ENODEV;
> + goto out;
> + }
> + found_dma_ranges = true;
> +
> + node = of_get_next_dma_parent(node);
> }
>
> - if (!ranges) {
> + if (!node || !ranges) {
> pr_debug("no dma-ranges found for node(%pOF)\n", np);
> ret = -ENODEV;
> goto out;
> }
>
> - len /= sizeof(u32);
> -
> + naddr = of_bus_n_addr_cells(node);
> + nsize = of_bus_n_size_cells(node);
> pna = of_n_addr_cells(node);
> + if ((len / sizeof(__be32)) % (pna + naddr + nsize)) {
> + ret = -EINVAL;
> + goto out;
> + }
>
> /* dma-ranges format:
> * DMA addr : naddr cells
> @@ -978,7 +970,7 @@ int of_dma_get_range(struct device_node *np, u64
> *dma_addr, u64 *paddr, u64 *siz
> * size : nsize cells
> */
> dmaaddr = of_read_number(ranges, naddr);
> - *paddr = of_translate_dma_address(np, ranges);
> + *paddr = of_translate_dma_address(node, ranges + naddr);
> if (*paddr == OF_BAD_ADDR) {
> pr_err("translation of DMA address(%llx) to CPU address failed
> node(%pOF)\n",
> dmaaddr, np);
signature.asc
Description: This is a digitally signed message part
