| Message ID | 1409680587-29818-5-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
On Tuesday 02 September 2014 18:56:24 Will Deacon wrote: > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > +{ > + struct of_phandle_args iommu_spec; > + struct iommu_dma_mapping *mapping; > + struct bus_type *bus = dev->bus; > + const struct iommu_ops *ops = bus->iommu_ops; I think it would be best to not even introduce the tight coupling between bus_type and iommu_ops here, one of the main reasons we are doing this is to break that connection. Better put the iommu_ops into the iommu_data pointer that gets looked up per iommu device. > + struct iommu_data *iommu = NULL; > + int idx = 0; > + > + if (!iommu_present(bus) || !ops->of_xlate) > + return NULL; > + > + /* > + * We don't currently walk up the tree looking for a parent IOMMU. > + * See the `Notes:' section of > + * Documentation/devicetree/bindings/iommu/iommu.txt > + */ > + */ > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + "#iommu-cells", idx, > + &iommu_spec)) { > + struct device_node *np = iommu_spec.np; > + struct iommu_data *data = of_iommu_get_data(np); > + > + if (!iommu) { > + if (!ops->of_xlate(dev, &iommu_spec)) > + iommu = data; I would make the first argument of the of_xlate function the iommu_data, so the code can find the right instance. Maybe also add an extra argument at the end that can be used by the PCI code and potentially other drivers with multiple master IDs behind one "iommus" property to pass some value identifying which of the masters is meant. The format of that is unfortunately bus specific and our platform_bus really covers a number of different hardware buses (AXI, AHB, OPB, ...) but the caller should be able to provide the data in the right form that the iommu understands. I would try using a single u64 argument as a start, hoping that this covers all the buses we need. At least it's enough for PCI (bus/device/function) and for AXI (requester-id?). > + } else if (iommu != data) { > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > + dev_name(dev)); > + iommu = NULL; > + } > + > + of_node_put(np); > + > + if (!iommu) > + break; > + > + idx++; > + } > + > + if (!iommu) > + return NULL; > + > + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); > + if (!mapping) > + return NULL; > I don't think it's safe to use devm_* functions here: this is during device discovery, and this data will be freed if probe() fails or the device gets removed from a driver. Arnd
Hi Arnd, Thanks for the review. On Tue, Sep 02, 2014 at 08:10:10PM +0100, Arnd Bergmann wrote: > On Tuesday 02 September 2014 18:56:24 Will Deacon wrote: > > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > > +{ > > + struct of_phandle_args iommu_spec; > > + struct iommu_dma_mapping *mapping; > > + struct bus_type *bus = dev->bus; > > + const struct iommu_ops *ops = bus->iommu_ops; > > I think it would be best to not even introduce the tight coupling > between bus_type and iommu_ops here, one of the main reasons we > are doing this is to break that connection. > > Better put the iommu_ops into the iommu_data pointer that gets looked > up per iommu device. Yes, I'll add that. It's a bit weird, because those same ops will later be duplicated in iommu_data->domain->ops, but that's an artifact of how the domain is currently constructed by iommu_domain_alloc. > > + struct iommu_data *iommu = NULL; > > + int idx = 0; > > + > > + if (!iommu_present(bus) || !ops->of_xlate) > > + return NULL; > > + > > + /* > > + * We don't currently walk up the tree looking for a parent IOMMU. > > + * See the `Notes:' section of > > + * Documentation/devicetree/bindings/iommu/iommu.txt > > + */ > > + */ > > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > > + "#iommu-cells", idx, > > + &iommu_spec)) { > > + struct device_node *np = iommu_spec.np; > > + struct iommu_data *data = of_iommu_get_data(np); > > + > > + if (!iommu) { > > + if (!ops->of_xlate(dev, &iommu_spec)) > > + iommu = data; > > I would make the first argument of the of_xlate function the iommu_data, > so the code can find the right instance. Oops, that's what I intended. Well spotted. > Maybe also add an extra argument at the end that can be used by the > PCI code and potentially other drivers with multiple master IDs > behind one "iommus" property to pass some value identifying which of > the masters is meant. > > The format of that is unfortunately bus specific and our platform_bus > really covers a number of different hardware buses (AXI, AHB, OPB, ...) > but the caller should be able to provide the data in the right form > that the iommu understands. I would try using a single u64 argument > as a start, hoping that this covers all the buses we need. At least > it's enough for PCI (bus/device/function) and for AXI (requester-id?). Yeah, I think that will work. It's kind of like a device `index' for a device that sits behind a bridge (trying to avoid yet another ID parameter :). > > > + } else if (iommu != data) { > > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > > + dev_name(dev)); > > + iommu = NULL; > > + } > > + > > + of_node_put(np); > > + > > + if (!iommu) > > + break; > > + > > + idx++; > > + } > > + > > + if (!iommu) > > + return NULL; > > + > > + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); > > + if (!mapping) > > + return NULL; > > > > I don't think it's safe to use devm_* functions here: this is during > device discovery, and this data will be freed if probe() fails or > the device gets removed from a driver. So I can make this a standard kzalloc, but I have no idea where the corresponding kfree should live. Is there something equivalent to of_dma_unconfigure, or is this data that is expected to persist? Will
On Thursday 04 September 2014 12:26:25 Will Deacon wrote: > > > > > + } else if (iommu != data) { > > > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > > > + dev_name(dev)); > > > + iommu = NULL; > > > + } > > > + > > > + of_node_put(np); > > > + > > > + if (!iommu) > > > + break; > > > + > > > + idx++; > > > + } > > > + > > > + if (!iommu) > > > + return NULL; > > > + > > > + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); > > > + if (!mapping) > > > + return NULL; > > > > > > > I don't think it's safe to use devm_* functions here: this is during > > device discovery, and this data will be freed if probe() fails or > > the device gets removed from a driver. > > So I can make this a standard kzalloc, but I have no idea where the > corresponding kfree should live. Is there something equivalent to > of_dma_unconfigure, or is this data that is expected to persist? > Can this be a simple kfree in of_platform_device_create_pdata? Arnd
On Thu, Sep 04, 2014 at 12:59:50PM +0100, Arnd Bergmann wrote: > On Thursday 04 September 2014 12:26:25 Will Deacon wrote: > > > > > > > + } else if (iommu != data) { > > > > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > > > > + dev_name(dev)); > > > > + iommu = NULL; > > > > + } > > > > + > > > > + of_node_put(np); > > > > + > > > > + if (!iommu) > > > > + break; > > > > + > > > > + idx++; > > > > + } > > > > + > > > > + if (!iommu) > > > > + return NULL; > > > > + > > > > + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); > > > > + if (!mapping) > > > > + return NULL; > > > > > > > > > > I don't think it's safe to use devm_* functions here: this is during > > > device discovery, and this data will be freed if probe() fails or > > > the device gets removed from a driver. > > > > So I can make this a standard kzalloc, but I have no idea where the > > corresponding kfree should live. Is there something equivalent to > > of_dma_unconfigure, or is this data that is expected to persist? > > > > Can this be a simple kfree in of_platform_device_create_pdata? We could certainly hook into the error path there (when of_device_add fails), yes, but I was also thinking about device removal and whether we need to care about that. I guess not for the OF case. Will
Hi Will, Thank you for the patch. On Tuesday 02 September 2014 18:56:24 Will Deacon wrote: > The generic IOMMU device-tree bindings can be used to add arbitrary OF > masters to an IOMMU with a compliant binding. > > This patch introduces of_iommu_configure, which does exactly that. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/Kconfig | 2 +- > drivers/iommu/of_iommu.c | 52 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/dma-mapping.h | 7 ++++++ > include/linux/of_iommu.h | 8 +++++++ > 4 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index dd5112265cc9..6d13f962f156 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -15,7 +15,7 @@ if IOMMU_SUPPORT > > config OF_IOMMU > def_bool y > - depends on OF > + depends on OF && IOMMU_API > > config FSL_PAMU > bool "Freescale IOMMU support" > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index f9209666157c..1188c929ffa7 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -18,6 +18,7 @@ > */ > > #include <linux/export.h> > +#include <linux/iommu.h> > #include <linux/limits.h> > #include <linux/of.h> > #include <linux/of_iommu.h> > @@ -90,6 +91,57 @@ int of_get_dma_window(struct device_node *dn, const char > *prefix, int index, } > EXPORT_SYMBOL_GPL(of_get_dma_window); > > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > +{ > + struct of_phandle_args iommu_spec; > + struct iommu_dma_mapping *mapping; > + struct bus_type *bus = dev->bus; > + const struct iommu_ops *ops = bus->iommu_ops; > + struct iommu_data *iommu = NULL; > + int idx = 0; > + > + if (!iommu_present(bus) || !ops->of_xlate) > + return NULL; > + > + /* > + * We don't currently walk up the tree looking for a parent IOMMU. > + * See the `Notes:' section of > + * Documentation/devicetree/bindings/iommu/iommu.txt > + */ > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > + "#iommu-cells", idx, > + &iommu_spec)) { > + struct device_node *np = iommu_spec.np; > + struct iommu_data *data = of_iommu_get_data(np); > + > + if (!iommu) { > + if (!ops->of_xlate(dev, &iommu_spec)) > + iommu = data; If I understand the code correctly, this will call of_xlate for the first IOMMU reference only and silently ignore all subsequent references if they point to the same IOMMU device but with different stream/requester IDs. Is that the desired behaviour ? > + } else if (iommu != data) { > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > + dev_name(dev)); > + iommu = NULL; > + } > + > + of_node_put(np); > + > + if (!iommu) > + break; > + > + idx++; > + } > + > + if (!iommu) > + return NULL; > + > + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); > + if (!mapping) > + return NULL; > + > + mapping->iommu = iommu; > + return mapping; > +} > + > void __init of_iommu_init(void) > { > struct device_node *np; > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 0f7f7b68b0db..16bf92f71c3e 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -62,6 +62,13 @@ struct dma_map_ops { > int is_phys; > }; > > +struct iommu_data; > + > +struct iommu_dma_mapping { > + struct iommu_data *iommu; > + struct list_head node; > +}; > + > #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1)) > > #define DMA_MASK_NONE 0x0ULL > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index a39e2d78f735..f2d3b1e780d7 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -1,9 +1,12 @@ > #ifndef __OF_IOMMU_H > #define __OF_IOMMU_H > > +#include <linux/device.h> > #include <linux/iommu.h> > #include <linux/of.h> > > +struct iommu_dma_mapping; > + > #ifdef CONFIG_OF_IOMMU > > extern int of_get_dma_window(struct device_node *dn, const char *prefix, > @@ -11,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const > char *prefix, size_t *size); > > extern void of_iommu_init(void); > +extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev); > > #else > > @@ -22,6 +26,10 @@ static inline int of_get_dma_window(struct device_node > *dn, const char *prefix, } > > static inline void of_iommu_init(void) { } > +static inline struct iommu_dma_mapping *of_iommu_configure(struct device > *dev) +{ > + return NULL; > +} > > #endif /* CONFIG_OF_IOMMU */
Hi Laurent, Cheers for the review. On Wed, Sep 10, 2014 at 02:01:34PM +0100, Laurent Pinchart wrote: > On Tuesday 02 September 2014 18:56:24 Will Deacon wrote: > > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > > +{ > > + struct of_phandle_args iommu_spec; > > + struct iommu_dma_mapping *mapping; > > + struct bus_type *bus = dev->bus; > > + const struct iommu_ops *ops = bus->iommu_ops; > > + struct iommu_data *iommu = NULL; > > + int idx = 0; > > + > > + if (!iommu_present(bus) || !ops->of_xlate) > > + return NULL; > > + > > + /* > > + * We don't currently walk up the tree looking for a parent IOMMU. > > + * See the `Notes:' section of > > + * Documentation/devicetree/bindings/iommu/iommu.txt > > + */ > > + while (!of_parse_phandle_with_args(dev->of_node, "iommus", > > + "#iommu-cells", idx, > > + &iommu_spec)) { > > + struct device_node *np = iommu_spec.np; > > + struct iommu_data *data = of_iommu_get_data(np); > > + > > + if (!iommu) { > > + if (!ops->of_xlate(dev, &iommu_spec)) > > + iommu = data; > > If I understand the code correctly, this will call of_xlate for the first > IOMMU reference only and silently ignore all subsequent references if they > point to the same IOMMU device but with different stream/requester IDs. Is > that the desired behaviour ? No; I just fixed that actually. I'll send a v3 soon which has a bunch of fixes like this. Will
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index dd5112265cc9..6d13f962f156 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -15,7 +15,7 @@ if IOMMU_SUPPORT config OF_IOMMU def_bool y - depends on OF + depends on OF && IOMMU_API config FSL_PAMU bool "Freescale IOMMU support" diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index f9209666157c..1188c929ffa7 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -18,6 +18,7 @@ */ #include <linux/export.h> +#include <linux/iommu.h> #include <linux/limits.h> #include <linux/of.h> #include <linux/of_iommu.h> @@ -90,6 +91,57 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, } EXPORT_SYMBOL_GPL(of_get_dma_window); +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) +{ + struct of_phandle_args iommu_spec; + struct iommu_dma_mapping *mapping; + struct bus_type *bus = dev->bus; + const struct iommu_ops *ops = bus->iommu_ops; + struct iommu_data *iommu = NULL; + int idx = 0; + + if (!iommu_present(bus) || !ops->of_xlate) + return NULL; + + /* + * We don't currently walk up the tree looking for a parent IOMMU. + * See the `Notes:' section of + * Documentation/devicetree/bindings/iommu/iommu.txt + */ + while (!of_parse_phandle_with_args(dev->of_node, "iommus", + "#iommu-cells", idx, + &iommu_spec)) { + struct device_node *np = iommu_spec.np; + struct iommu_data *data = of_iommu_get_data(np); + + if (!iommu) { + if (!ops->of_xlate(dev, &iommu_spec)) + iommu = data; + } else if (iommu != data) { + pr_warn("Rejecting device %s with multiple IOMMU instances\n", + dev_name(dev)); + iommu = NULL; + } + + of_node_put(np); + + if (!iommu) + break; + + idx++; + } + + if (!iommu) + return NULL; + + mapping = devm_kzalloc(dev, sizeof(*mapping), GFP_KERNEL); + if (!mapping) + return NULL; + + mapping->iommu = iommu; + return mapping; +} + void __init of_iommu_init(void) { struct device_node *np; diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 0f7f7b68b0db..16bf92f71c3e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -62,6 +62,13 @@ struct dma_map_ops { int is_phys; }; +struct iommu_data; + +struct iommu_dma_mapping { + struct iommu_data *iommu; + struct list_head node; +}; + #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1)) #define DMA_MASK_NONE 0x0ULL diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index a39e2d78f735..f2d3b1e780d7 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -1,9 +1,12 @@ #ifndef __OF_IOMMU_H #define __OF_IOMMU_H +#include <linux/device.h> #include <linux/iommu.h> #include <linux/of.h> +struct iommu_dma_mapping; + #ifdef CONFIG_OF_IOMMU extern int of_get_dma_window(struct device_node *dn, const char *prefix, @@ -11,6 +14,7 @@ extern int of_get_dma_window(struct device_node *dn, const char *prefix, size_t *size); extern void of_iommu_init(void); +extern struct iommu_dma_mapping *of_iommu_configure(struct device *dev); #else @@ -22,6 +26,10 @@ static inline int of_get_dma_window(struct device_node *dn, const char *prefix, } static inline void of_iommu_init(void) { } +static inline struct iommu_dma_mapping *of_iommu_configure(struct device *dev) +{ + return NULL; +} #endif /* CONFIG_OF_IOMMU */
The generic IOMMU device-tree bindings can be used to add arbitrary OF masters to an IOMMU with a compliant binding. This patch introduces of_iommu_configure, which does exactly that. Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/iommu/Kconfig | 2 +- drivers/iommu/of_iommu.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/dma-mapping.h | 7 ++++++ include/linux/of_iommu.h | 8 +++++++ 4 files changed, 68 insertions(+), 1 deletion(-)