| Message ID | 20191008195239.12852-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 951d48855d86e72e0d6de73440fe09d363168064 |
| Headers | show |
| Series | [v2] of: Make of_dma_get_range() work on bus nodes | expand |
Hi Rob/Robin, On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote: > From: Robin Murphy <robin.murphy@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.murphy@arm.com> > [robh: Allow for the bus child node to still be passed in] > Signed-off-by: Rob Herring <robh@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);
On Tue, Oct 8, 2019 at 3:52 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > Hi Rob/Robin, > > On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote: > > From: Robin Murphy <robin.murphy@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. What I was trying to say is we just can't tell if we should be looking in the current node or the parent. 'dma-ranges' in a leaf node can be correct or incorrect. Your example is a bit different. I'm not sure that case is valid or can ever work if it is. It's certainly valid that a PCI bridge's parent has dma-ranges and now we'll actually handle any translation. The bridge itself is not a DMA-capable device, but just passing thru DMA. Do we ever need to know the parent's dma-ranges in that case? Or to put it another way, is looking at anything other than leaf dma-ranges useful? Rob
On 2019-10-08 8:52 pm, Rob Herring wrote: > From: Robin Murphy <robin.murphy@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. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > [robh: Allow for the bus child node to still be passed in] > Signed-off-by: Rob Herring <robh@kernel.org> > --- > Resending, hit send too quickly. > > v2: > - Ensure once we find dma-ranges, every parent has it. > - 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'd still like to rework of_dma_configure() so that callers don't have to pass a redundant node in the common case, but that can wait. For now, this looks good enough to un-block the various 32-bit-PCI folks at least, and we can consider further improvements on top. For the changes: Reviewed-by: Robin Murphy <robin.murphy@arm.com> Cheers, Robin. > 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); >
On Tue, 2019-10-08 at 20:03 -0500, Rob Herring wrote: > On Tue, Oct 8, 2019 at 3:52 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > Hi Rob/Robin, > > > > On Tue, 2019-10-08 at 14:52 -0500, Rob Herring wrote: > > > From: Robin Murphy <robin.murphy@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. > > What I was trying to say is we just can't tell if we should be looking > in the current node or the parent. 'dma-ranges' in a leaf node can be > correct or incorrect. > > Your example is a bit different. I'm not sure that case is valid or > can ever work if it is. It's certainly valid that a PCI bridge's > parent has dma-ranges and now we'll actually handle any translation. > The bridge itself is not a DMA-capable device, but just passing thru > DMA. Yes, you're right, I hadn't thought of it from that perspective. Thanks! > Do we ever need to know the parent's dma-ranges in that case? Or > to put it another way, is looking at anything other than leaf > dma-ranges useful? There is no need at all indeed. With that, Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> and Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> On a Raspberry Pi 4 with pcie-brcmstb.c which is still work in progress but depends on this. Regards, Nicolas
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);