| Message ID | 20210106034124.30560-7-tientzu@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Restricted DMA | expand |
On 1/5/21 7:41 PM, Claire Chang wrote: > If a device is not behind an IOMMU, we look up the device node and set > up the restricted DMA when the restricted-dma-pool is presented. > > Signed-off-by: Claire Chang <tientzu@chromium.org> > --- [snip] > +int of_dma_set_restricted_buffer(struct device *dev) > +{ > + struct device_node *node; > + int count, i; > + > + if (!dev->of_node) > + return 0; > + > + count = of_property_count_elems_of_size(dev->of_node, "memory-region", > + sizeof(phandle)); You could have an early check for count < 0, along with an error message, if that is deemed useful. > + for (i = 0; i < count; i++) { > + node = of_parse_phandle(dev->of_node, "memory-region", i); > + if (of_device_is_compatible(node, "restricted-dma-pool")) And you may want to add here an of_device_is_available(node). A platform that provides the Device Tree firmware and try to support multiple different SoCs may try to determine if an IOMMU is present, and if it is, it could be marking the restriced-dma-pool region with a 'status = "disabled"' property, or any variant of that scheme. > + return of_reserved_mem_device_init_by_idx( > + dev, dev->of_node, i); This does not seem to be supporting more than one memory region, did not you want something like instead: ret = of_reserved_mem_device_init_by_idx(...); if (ret) return ret; > + } > + > + return 0; > +} > diff --git a/drivers/of/device.c b/drivers/of/device.c > index aedfaaafd3e7..e2c7409956ab 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -182,6 +182,10 @@ int of_dma_configure_id(struct device *dev, struct device_node *np, > arch_setup_dma_ops(dev, dma_start, size, iommu, coherent); > > dev->dma_range_map = map; > + > + if (!iommu) > + return of_dma_set_restricted_buffer(dev); > + > return 0; > } > EXPORT_SYMBOL_GPL(of_dma_configure_id); > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index d9e6a324de0a..28a2dfa197ba 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -161,12 +161,17 @@ struct bus_dma_region; > #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA) > int of_dma_get_range(struct device_node *np, > const struct bus_dma_region **map); > +int of_dma_set_restricted_buffer(struct device *dev); > #else > static inline int of_dma_get_range(struct device_node *np, > const struct bus_dma_region **map) > { > return -ENODEV; > } > +static inline int of_dma_get_restricted_buffer(struct device *dev) > +{ > + return -ENODEV; > +} > #endif > > #endif /* _LINUX_OF_PRIVATE_H */ >
On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 1/5/21 7:41 PM, Claire Chang wrote: > > If a device is not behind an IOMMU, we look up the device node and set > > up the restricted DMA when the restricted-dma-pool is presented. > > > > Signed-off-by: Claire Chang <tientzu@chromium.org> > > --- > > [snip] > > > +int of_dma_set_restricted_buffer(struct device *dev) > > +{ > > + struct device_node *node; > > + int count, i; > > + > > + if (!dev->of_node) > > + return 0; > > + > > + count = of_property_count_elems_of_size(dev->of_node, "memory-region", > > + sizeof(phandle)); > > You could have an early check for count < 0, along with an error > message, if that is deemed useful. > > > + for (i = 0; i < count; i++) { > > + node = of_parse_phandle(dev->of_node, "memory-region", i); > > + if (of_device_is_compatible(node, "restricted-dma-pool")) > > And you may want to add here an of_device_is_available(node). A platform > that provides the Device Tree firmware and try to support multiple > different SoCs may try to determine if an IOMMU is present, and if it > is, it could be marking the restriced-dma-pool region with a 'status = > "disabled"' property, or any variant of that scheme. This function is called only when there is no IOMMU present (check in drivers/of/device.c). I can still add of_device_is_available(node) here if you think it's helpful. > > > + return of_reserved_mem_device_init_by_idx( > > + dev, dev->of_node, i); > > This does not seem to be supporting more than one memory region, did not > you want something like instead: > > ret = of_reserved_mem_device_init_by_idx(...); > if (ret) > return ret; > Yes. This implement only supports one restriced-dma-pool memory region with the assumption that there is only one memory region with the compatible string, restricted-dma-pool, in the dts. IIUC, it's similar to shared-dma-pool. > > + } > > + > > + return 0; > > +} > > diff --git a/drivers/of/device.c b/drivers/of/device.c > > index aedfaaafd3e7..e2c7409956ab 100644 > > --- a/drivers/of/device.c > > +++ b/drivers/of/device.c > > @@ -182,6 +182,10 @@ int of_dma_configure_id(struct device *dev, struct device_node *np, > > arch_setup_dma_ops(dev, dma_start, size, iommu, coherent); > > > > dev->dma_range_map = map; > > + > > + if (!iommu) > > + return of_dma_set_restricted_buffer(dev); > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(of_dma_configure_id); > > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > > index d9e6a324de0a..28a2dfa197ba 100644 > > --- a/drivers/of/of_private.h > > +++ b/drivers/of/of_private.h > > @@ -161,12 +161,17 @@ struct bus_dma_region; > > #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA) > > int of_dma_get_range(struct device_node *np, > > const struct bus_dma_region **map); > > +int of_dma_set_restricted_buffer(struct device *dev); > > #else > > static inline int of_dma_get_range(struct device_node *np, > > const struct bus_dma_region **map) > > { > > return -ENODEV; > > } > > +static inline int of_dma_get_restricted_buffer(struct device *dev) > > +{ > > + return -ENODEV; > > +} > > #endif > > > > #endif /* _LINUX_OF_PRIVATE_H */ > > > > > -- > Florian
On 1/14/21 1:08 AM, Claire Chang wrote: > On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> On 1/5/21 7:41 PM, Claire Chang wrote: >>> If a device is not behind an IOMMU, we look up the device node and set >>> up the restricted DMA when the restricted-dma-pool is presented. >>> >>> Signed-off-by: Claire Chang <tientzu@chromium.org> >>> --- >> >> [snip] >> >>> +int of_dma_set_restricted_buffer(struct device *dev) >>> +{ >>> + struct device_node *node; >>> + int count, i; >>> + >>> + if (!dev->of_node) >>> + return 0; >>> + >>> + count = of_property_count_elems_of_size(dev->of_node, "memory-region", >>> + sizeof(phandle)); >> >> You could have an early check for count < 0, along with an error >> message, if that is deemed useful. >> >>> + for (i = 0; i < count; i++) { >>> + node = of_parse_phandle(dev->of_node, "memory-region", i); >>> + if (of_device_is_compatible(node, "restricted-dma-pool")) >> >> And you may want to add here an of_device_is_available(node). A platform >> that provides the Device Tree firmware and try to support multiple >> different SoCs may try to determine if an IOMMU is present, and if it >> is, it could be marking the restriced-dma-pool region with a 'status = >> "disabled"' property, or any variant of that scheme. > > This function is called only when there is no IOMMU present (check in > drivers/of/device.c). I can still add of_device_is_available(node) > here if you think it's helpful. I believe it is, since boot loader can have a shared Device Tree blob skeleton and do various adaptations based on the chip (that's what we do) and adding a status property is much simpler than insertion new nodes are run time. > >> >>> + return of_reserved_mem_device_init_by_idx( >>> + dev, dev->of_node, i); >> >> This does not seem to be supporting more than one memory region, did not >> you want something like instead: >> >> ret = of_reserved_mem_device_init_by_idx(...); >> if (ret) >> return ret; >> > > Yes. This implement only supports one restriced-dma-pool memory region > with the assumption that there is only one memory region with the > compatible string, restricted-dma-pool, in the dts. IIUC, it's similar > to shared-dma-pool. Then if here is such a known limitation it should be both documented and enforced here, you shouldn ot be iterating over all of the phandles that you find, stop at the first one and issue a warning if count > 1?
On Fri, Jan 15, 2021 at 2:52 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 1/14/21 1:08 AM, Claire Chang wrote: > > On Wed, Jan 13, 2021 at 7:48 AM Florian Fainelli <f.fainelli@gmail.com> wrote: > >> > >> On 1/5/21 7:41 PM, Claire Chang wrote: > >>> If a device is not behind an IOMMU, we look up the device node and set > >>> up the restricted DMA when the restricted-dma-pool is presented. > >>> > >>> Signed-off-by: Claire Chang <tientzu@chromium.org> > >>> --- > >> > >> [snip] > >> > >>> +int of_dma_set_restricted_buffer(struct device *dev) > >>> +{ > >>> + struct device_node *node; > >>> + int count, i; > >>> + > >>> + if (!dev->of_node) > >>> + return 0; > >>> + > >>> + count = of_property_count_elems_of_size(dev->of_node, "memory-region", > >>> + sizeof(phandle)); > >> > >> You could have an early check for count < 0, along with an error > >> message, if that is deemed useful. > >> > >>> + for (i = 0; i < count; i++) { > >>> + node = of_parse_phandle(dev->of_node, "memory-region", i); > >>> + if (of_device_is_compatible(node, "restricted-dma-pool")) > >> > >> And you may want to add here an of_device_is_available(node). A platform > >> that provides the Device Tree firmware and try to support multiple > >> different SoCs may try to determine if an IOMMU is present, and if it > >> is, it could be marking the restriced-dma-pool region with a 'status = > >> "disabled"' property, or any variant of that scheme. > > > > This function is called only when there is no IOMMU present (check in > > drivers/of/device.c). I can still add of_device_is_available(node) > > here if you think it's helpful. > > I believe it is, since boot loader can have a shared Device Tree blob > skeleton and do various adaptations based on the chip (that's what we > do) and adding a status property is much simpler than insertion new > nodes are run time. > > > > >> > >>> + return of_reserved_mem_device_init_by_idx( > >>> + dev, dev->of_node, i); > >> > >> This does not seem to be supporting more than one memory region, did not > >> you want something like instead: > >> > >> ret = of_reserved_mem_device_init_by_idx(...); > >> if (ret) > >> return ret; > >> > > > > Yes. This implement only supports one restriced-dma-pool memory region > > with the assumption that there is only one memory region with the > > compatible string, restricted-dma-pool, in the dts. IIUC, it's similar > > to shared-dma-pool. > > Then if here is such a known limitation it should be both documented and > enforced here, you shouldn ot be iterating over all of the phandles that > you find, stop at the first one and issue a warning if count > 1? What I have in mind is there might be multiple memory regions, but only one is for restriced-dma-pool. Say, if you want a separated region for coherent DMA and only do streaming DMA in this restriced-dma-pool region, you can add another reserved-memory node with shared-dma-pool in dts and the current implementation will try to allocate the memory via dma_alloc_from_dev_coherent() first (see dma_alloc_attrs() in /kernel/dma/mapping.c). Or if you have vendor specific memory region, you can still set up restriced-dma-pool by adding another reserved-memory node in dts. Dose this make sense to you? I'll document this for sure. > -- > Florian
diff --git a/drivers/of/address.c b/drivers/of/address.c index 73ddf2540f3f..94eca8249854 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -8,6 +8,7 @@ #include <linux/logic_pio.h> #include <linux/module.h> #include <linux/of_address.h> +#include <linux/of_reserved_mem.h> #include <linux/pci.h> #include <linux/pci_regs.h> #include <linux/sizes.h> @@ -1094,3 +1095,23 @@ bool of_dma_is_coherent(struct device_node *np) return false; } EXPORT_SYMBOL_GPL(of_dma_is_coherent); + +int of_dma_set_restricted_buffer(struct device *dev) +{ + struct device_node *node; + int count, i; + + if (!dev->of_node) + return 0; + + count = of_property_count_elems_of_size(dev->of_node, "memory-region", + sizeof(phandle)); + for (i = 0; i < count; i++) { + node = of_parse_phandle(dev->of_node, "memory-region", i); + if (of_device_is_compatible(node, "restricted-dma-pool")) + return of_reserved_mem_device_init_by_idx( + dev, dev->of_node, i); + } + + return 0; +} diff --git a/drivers/of/device.c b/drivers/of/device.c index aedfaaafd3e7..e2c7409956ab 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -182,6 +182,10 @@ int of_dma_configure_id(struct device *dev, struct device_node *np, arch_setup_dma_ops(dev, dma_start, size, iommu, coherent); dev->dma_range_map = map; + + if (!iommu) + return of_dma_set_restricted_buffer(dev); + return 0; } EXPORT_SYMBOL_GPL(of_dma_configure_id); diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index d9e6a324de0a..28a2dfa197ba 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -161,12 +161,17 @@ struct bus_dma_region; #if defined(CONFIG_OF_ADDRESS) && defined(CONFIG_HAS_DMA) int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map); +int of_dma_set_restricted_buffer(struct device *dev); #else static inline int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) { return -ENODEV; } +static inline int of_dma_get_restricted_buffer(struct device *dev) +{ + return -ENODEV; +} #endif #endif /* _LINUX_OF_PRIVATE_H */
If a device is not behind an IOMMU, we look up the device node and set up the restricted DMA when the restricted-dma-pool is presented. Signed-off-by: Claire Chang <tientzu@chromium.org> --- drivers/of/address.c | 21 +++++++++++++++++++++ drivers/of/device.c | 4 ++++ drivers/of/of_private.h | 5 +++++ 3 files changed, 30 insertions(+)