| Message ID | 20190927002455.13169-6-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | of: dma-ranges fixes and improvements | expand |
On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > -int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) > +int of_dma_configure(struct device *dev, struct device_node *parent, bool force_dma) This creates a > 80 char line. > { > u64 dma_addr, paddr, size = 0; > int ret; > bool coherent; > unsigned long offset; > const struct iommu_ops *iommu; > + struct device_node *np; > u64 mask; > > + np = dev->of_node; > + if (!np) > + np = parent; > + if (!np) > + return -ENODEV; I have to say I find the older calling convention simpler to understand. If we want to enforce the invariant I'd rather do that explicitly: if (dev->of_node && np != dev->of_node) return -EINVAL;
On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote: > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > > -int of_dma_configure(struct device *dev, struct device_node *np, bool > > force_dma) > > +int of_dma_configure(struct device *dev, struct device_node *parent, bool > > force_dma) > > This creates a > 80 char line. > > > { > > u64 dma_addr, paddr, size = 0; > > int ret; > > bool coherent; > > unsigned long offset; > > const struct iommu_ops *iommu; > > + struct device_node *np; > > u64 mask; > > > > + np = dev->of_node; > > + if (!np) > > + np = parent; > > + if (!np) > > + return -ENODEV; > > I have to say I find the older calling convention simpler to understand. > If we want to enforce the invariant I'd rather do that explicitly: > > if (dev->of_node && np != dev->of_node) > return -EINVAL; As is, this would break Freescale Layerscape fsl-mc bus' dma_configure(): static int fsl_mc_dma_configure(struct device *dev) { struct device *dma_dev = dev; while (dev_is_fsl_mc(dma_dev)) dma_dev = dma_dev->parent; return of_dma_configure(dev, dma_dev->of_node, 0); } But I think that with this series, given the fact that we now treat the lack of dma-ranges as a 1:1 mapping instead of an error, we could rewrite the function like this: static int fsl_mc_dma_configure(struct device *dev) { return of_dma_configure(dev, false, 0); } If needed I can test this. Regards, Nicolas
On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote: > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool > > > force_dma) > > > +int of_dma_configure(struct device *dev, struct device_node *parent, bool > > > force_dma) > > > > This creates a > 80 char line. > > > > > { > > > u64 dma_addr, paddr, size = 0; > > > int ret; > > > bool coherent; > > > unsigned long offset; > > > const struct iommu_ops *iommu; > > > + struct device_node *np; > > > u64 mask; > > > > > > + np = dev->of_node; > > > + if (!np) > > > + np = parent; > > > + if (!np) > > > + return -ENODEV; > > > > I have to say I find the older calling convention simpler to understand. > > If we want to enforce the invariant I'd rather do that explicitly: > > > > if (dev->of_node && np != dev->of_node) > > return -EINVAL; > > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure(): This may break PCI too for devices that have a DT node. > static int fsl_mc_dma_configure(struct device *dev) > { > struct device *dma_dev = dev; > > while (dev_is_fsl_mc(dma_dev)) > dma_dev = dma_dev->parent; > > return of_dma_configure(dev, dma_dev->of_node, 0); > } > > But I think that with this series, given the fact that we now treat the lack of > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the function > like this: Now, I'm reconsidering allowing this abuse... It's better if the code which understands the bus structure in DT for a specific bus passes in the right thing. Maybe I should go back to Robin's version (below). OTOH, the existing assumption that 'dma-ranges' was in the immediate parent was an assumption on the bus structure which maybe doesn't always apply. diff --git a/drivers/of/device.c b/drivers/of/device.c index a45261e21144..6951450bb8f3 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct device_node *parent, bool force_ u64 mask; np = dev->of_node; - if (!np) - np = parent; + if (np) + parent = of_get_dma_parent(np); + else + np = of_node_get(parent); if (!np) return -ENODEV; - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); + of_node_put(parent); if (ret < 0) { /* * For legacy reasons, we have to assume some devices need
On Mon, 2019-09-30 at 16:24 -0500, Rob Herring wrote: > On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote: > > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool > > > > force_dma) > > > > +int of_dma_configure(struct device *dev, struct device_node *parent, > > > > bool > > > > force_dma) > > > > > > This creates a > 80 char line. > > > > > > > { > > > > u64 dma_addr, paddr, size = 0; > > > > int ret; > > > > bool coherent; > > > > unsigned long offset; > > > > const struct iommu_ops *iommu; > > > > + struct device_node *np; > > > > u64 mask; > > > > > > > > + np = dev->of_node; > > > > + if (!np) > > > > + np = parent; > > > > + if (!np) > > > > + return -ENODEV; > > > > > > I have to say I find the older calling convention simpler to understand. > > > If we want to enforce the invariant I'd rather do that explicitly: > > > > > > if (dev->of_node && np != dev->of_node) > > > return -EINVAL; > > > > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure(): > > This may break PCI too for devices that have a DT node. > > > static int fsl_mc_dma_configure(struct device *dev) > > { > > struct device *dma_dev = dev; > > > > while (dev_is_fsl_mc(dma_dev)) > > dma_dev = dma_dev->parent; > > > > return of_dma_configure(dev, dma_dev->of_node, 0); > > } > > > > But I think that with this series, given the fact that we now treat the lack > > of > > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the > > function > > like this: > > Now, I'm reconsidering allowing this abuse... It's better if the code > which understands the bus structure in DT for a specific bus passes in > the right thing. Maybe I should go back to Robin's version (below). > OTOH, the existing assumption that 'dma-ranges' was in the immediate > parent was an assumption on the bus structure which maybe doesn't > always apply. > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index a45261e21144..6951450bb8f3 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct > device_node *parent, bool force_ > u64 mask; > > np = dev->of_node; > - if (!np) > - np = parent; > + if (np) > + parent = of_get_dma_parent(np); > + else > + np = of_node_get(parent); > if (!np) > return -ENODEV; > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); > + of_node_put(parent); > if (ret < 0) { > /* > * For legacy reasons, we have to assume some devices need I spent some time thinking about your comments and researching. I came to the realization that both these solutions break the usage in drivers/gpu/drm/sun4i/sun4i_backend.c:805. In that specific case both 'dev->of_node' and 'parent' exist yet the device receiving the configuration and 'parent' aren't related in any way. IOW we can't just use 'dev->of_node' as a starting point to walk upwards the tree. We always have to respect whatever DT node the bus provided, and start there. This clashes with the current solutions, as they are based on the fact that we can use dev->of_node when present. My guess at this point, if we're forced to honor that behaviour, is that we have to create a new API for the PCI use case. Something the likes of of_dma_configure_parent(). Regards, Nicolas
On Tue, Oct 1, 2019 at 10:43 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > On Mon, 2019-09-30 at 16:24 -0500, Rob Herring wrote: > > On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne > > <nsaenzjulienne@suse.de> wrote: > > > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote: > > > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote: > > > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool > > > > > force_dma) > > > > > +int of_dma_configure(struct device *dev, struct device_node *parent, > > > > > bool > > > > > force_dma) > > > > > > > > This creates a > 80 char line. > > > > > > > > > { > > > > > u64 dma_addr, paddr, size = 0; > > > > > int ret; > > > > > bool coherent; > > > > > unsigned long offset; > > > > > const struct iommu_ops *iommu; > > > > > + struct device_node *np; > > > > > u64 mask; > > > > > > > > > > + np = dev->of_node; > > > > > + if (!np) > > > > > + np = parent; > > > > > + if (!np) > > > > > + return -ENODEV; > > > > > > > > I have to say I find the older calling convention simpler to understand. > > > > If we want to enforce the invariant I'd rather do that explicitly: > > > > > > > > if (dev->of_node && np != dev->of_node) > > > > return -EINVAL; > > > > > > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure(): > > > > This may break PCI too for devices that have a DT node. > > > > > static int fsl_mc_dma_configure(struct device *dev) > > > { > > > struct device *dma_dev = dev; > > > > > > while (dev_is_fsl_mc(dma_dev)) > > > dma_dev = dma_dev->parent; > > > > > > return of_dma_configure(dev, dma_dev->of_node, 0); > > > } > > > > > > But I think that with this series, given the fact that we now treat the lack > > > of > > > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the > > > function > > > like this: > > > > Now, I'm reconsidering allowing this abuse... It's better if the code > > which understands the bus structure in DT for a specific bus passes in > > the right thing. Maybe I should go back to Robin's version (below). > > OTOH, the existing assumption that 'dma-ranges' was in the immediate > > parent was an assumption on the bus structure which maybe doesn't > > always apply. > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c > > index a45261e21144..6951450bb8f3 100644 > > --- a/drivers/of/device.c > > +++ b/drivers/of/device.c > > @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct > > device_node *parent, bool force_ > > u64 mask; > > > > np = dev->of_node; > > - if (!np) > > - np = parent; > > + if (np) > > + parent = of_get_dma_parent(np); > > + else > > + np = of_node_get(parent); > > if (!np) > > return -ENODEV; > > > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > > + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); > > + of_node_put(parent); > > if (ret < 0) { > > /* > > * For legacy reasons, we have to assume some devices need > > I spent some time thinking about your comments and researching. I came to the > realization that both these solutions break the usage in > drivers/gpu/drm/sun4i/sun4i_backend.c:805. In that specific case both > 'dev->of_node' and 'parent' exist yet the device receiving the configuration > and 'parent' aren't related in any way. I knew there was some reason I didn't like those virtual DT nodes... That does seem to be the oddest case. Several of the others are just non-DT child platform devices. Perhaps we need a "copy the DMA config from another struct device (or parent struct device)" function to avoid using a DT function on a non-DT device. > IOW we can't just use 'dev->of_node' as a starting point to walk upwards the > tree. We always have to respect whatever DT node the bus provided, and start > there. This clashes with the current solutions, as they are based on the fact > that we can use dev->of_node when present. Yes, you are right. > My guess at this point, if we're forced to honor that behaviour, is that we > have to create a new API for the PCI use case. Something the likes of > of_dma_configure_parent(). I think of_dma_configure just has to work with the device_node of either the device or the device parent and dev->of_node is never used unless the caller sets it. Rob
On Thu, 2019-10-03 at 20:53 -0500, Rob Herring wrote: > > > > But I think that with this series, given the fact that we now treat the > > > > lack > > > > of > > > > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the > > > > function > > > > like this: > > > > > > Now, I'm reconsidering allowing this abuse... It's better if the code > > > which understands the bus structure in DT for a specific bus passes in > > > the right thing. Maybe I should go back to Robin's version (below). > > > OTOH, the existing assumption that 'dma-ranges' was in the immediate > > > parent was an assumption on the bus structure which maybe doesn't > > > always apply. > > > > > > diff --git a/drivers/of/device.c b/drivers/of/device.c > > > index a45261e21144..6951450bb8f3 100644 > > > --- a/drivers/of/device.c > > > +++ b/drivers/of/device.c > > > @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct > > > device_node *parent, bool force_ > > > u64 mask; > > > > > > np = dev->of_node; > > > - if (!np) > > > - np = parent; > > > + if (np) > > > + parent = of_get_dma_parent(np); > > > + else > > > + np = of_node_get(parent); > > > if (!np) > > > return -ENODEV; > > > > > > - ret = of_dma_get_range(np, &dma_addr, &paddr, &size); > > > + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); > > > + of_node_put(parent); > > > if (ret < 0) { > > > /* > > > * For legacy reasons, we have to assume some devices need > > > > I spent some time thinking about your comments and researching. I came to > > the > > realization that both these solutions break the usage in > > drivers/gpu/drm/sun4i/sun4i_backend.c:805. In that specific case both > > 'dev->of_node' and 'parent' exist yet the device receiving the configuration > > and 'parent' aren't related in any way. > > I knew there was some reason I didn't like those virtual DT nodes... > > That does seem to be the oddest case. Several of the others are just > non-DT child platform devices. Perhaps we need a "copy the DMA config > from another struct device (or parent struct device)" function to > avoid using a DT function on a non-DT device. > > > IOW we can't just use 'dev->of_node' as a starting point to walk upwards the > > tree. We always have to respect whatever DT node the bus provided, and start > > there. This clashes with the current solutions, as they are based on the > > fact > > that we can use dev->of_node when present. > > Yes, you are right. > > > My guess at this point, if we're forced to honor that behaviour, is that we > > have to create a new API for the PCI use case. Something the likes of > > of_dma_configure_parent(). > > I think of_dma_configure just has to work with the device_node of > either the device or the device parent and dev->of_node is never used > unless the caller sets it. Fine, so given the following two distinct uses of of_dma_configure(struct device *dev, struct device_node *np, bool ...): - dev->of_node == np: Platform bus' typical use, we imperatively have to start parsing dma-ranges from np's DMA parent, as the device we're configuring might be a bus containing dma-ranges himself. For example a platform PCIe bus. - dev->of_node != np: Here the bus is pulling some trick. The device might or might not be represented in DT and np might be a bus or a device. But one thing I realised is that device being configured never represents a memory mapped bus. Assuming this assumption is acceptable, we can traverse the DT tree starting from np and get a correct configuration as long as dma-ranges not being present is interpreted as a 1:1 mapping. The resulting code, which I tested on an RPi4, Freescale Layerscape and passes OF's unit tests, looks like this: int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) { u64 dma_addr, paddr, size = 0; struct device_node *parent; u64 mask; int ret; if (!np) return -ENODEV; parent = of_node_get(np); if (dev->of_node == parent) parent = of_get_next_dma_parent(np); ret = of_dma_get_range(parent, &dma_addr, &paddr, &size); of_node_put(parent); [...] } Would that be acceptable? Regards, Nicolas
diff --git a/drivers/of/device.c b/drivers/of/device.c index da8158392010..a45261e21144 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -75,7 +75,8 @@ int of_device_add(struct platform_device *ofdev) /** * of_dma_configure - Setup DMA configuration * @dev: Device to apply DMA configuration - * @np: Pointer to OF node having DMA configuration + * @parent: OF node of parent device having DMA configuration, if + * @dev->of_node is NULL (ignored otherwise) * @force_dma: Whether device is to be set up by of_dma_configure() even if * DMA capability is not explicitly described by firmware. * @@ -86,15 +87,22 @@ int of_device_add(struct platform_device *ofdev) * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events * to fix up DMA configuration. */ -int of_dma_configure(struct device *dev, struct device_node *np, bool force_dma) +int of_dma_configure(struct device *dev, struct device_node *parent, bool force_dma) { u64 dma_addr, paddr, size = 0; int ret; bool coherent; unsigned long offset; const struct iommu_ops *iommu; + struct device_node *np; u64 mask; + np = dev->of_node; + if (!np) + np = parent; + if (!np) + return -ENODEV; + ret = of_dma_get_range(np, &dma_addr, &paddr, &size); if (ret < 0) { /* diff --git a/include/linux/of_device.h b/include/linux/of_device.h index 8d31e39dd564..a4fe418e57f6 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -56,7 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) } int of_dma_configure(struct device *dev, - struct device_node *np, + struct device_node *parent, bool force_dma); #else /* CONFIG_OF */ @@ -107,7 +107,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu) } static inline int of_dma_configure(struct device *dev, - struct device_node *np, + struct device_node *parent, bool force_dma) { return 0;