| Message ID | 1410539695-29128-5-git-send-email-will.deacon@arm.com |
|---|---|
| State | New |
| Headers | show |
Hi Will, Thank you for the patch. On Friday 12 September 2014 17:34:52 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. A > list of iommu_dma_mapping structures are created for each device, which > represent the set of IOMMU instances through which the device can > master. The list is protected by a kref count and freed when no users > remain. It is expected that DMA-mapping code will take a reference if it > wishes to make use of the IOMMU information. > > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > drivers/iommu/Kconfig | 2 +- > drivers/iommu/of_iommu.c | 70 ++++++++++++++++++++++++++++++++++++++++++ > include/linux/dma-mapping.h | 8 ++++++ > include/linux/of_iommu.h | 10 +++++++ > 4 files changed, 89 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 13d800c4ce25..8656b63f27ee 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -18,9 +18,11 @@ > */ > > #include <linux/export.h> > +#include <linux/iommu.h> > #include <linux/limits.h> > #include <linux/of.h> > #include <linux/of_iommu.h> > +#include <linux/slab.h> > > /** > * of_get_dma_window - Parse *dma-window property and returns 0 if found. > @@ -90,6 +92,74 @@ 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 device_node *np; > + struct iommu_data *iommu = NULL; > + int idx = 0; > + > + /* > + * 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 iommu_data *data; > + > + np = iommu_spec.np; > + data = of_iommu_get_data(np); > + > + if (!data || !data->ops || !data->ops->of_xlate) > + goto err_put_node; > + > + if (!iommu) { > + iommu = data; > + } else if (iommu != data) { > + /* We don't currently support multi-IOMMU masters */ > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > + dev_name(dev)); > + goto err_put_node; > + } > + > + if (!data->ops->of_xlate(dev, &iommu_spec)) > + goto err_put_node; > + > + of_node_put(np); > + idx++; > + } > + > + if (!iommu) > + return NULL; > + > + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); > + if (!mapping) > + return NULL; > + > + kref_init(&mapping->kref); > + INIT_LIST_HEAD(&mapping->node); I might be missing something, but that doesn't seem to match the commit message. This creates a single iommu_dma_mapping per device, where is the list supposed to be populated ? > + mapping->iommu = iommu; > + return mapping; > + > +err_put_node: > + of_node_put(np); > + return NULL; > +} > + > +void of_iommu_deconfigure(struct kref *kref) > +{ > + struct iommu_dma_mapping *mapping, *curr, *next; > + > + mapping = container_of(kref, struct iommu_dma_mapping, kref); > + list_for_each_entry_safe(curr, next, &mapping->node, node) { > + list_del(&curr->node); > + kfree(curr); > + } Don't you need to also kfree(mapping) here ? > +} > + Do you expect other users of of_iommu_deconfigure() than kref_put() ? If not, how about exposing an of_iommu_put(struct iommu_dma_mapping *) that would call kref_put() internally, and hiding of_iommu_deconfigure() ? > 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 1e944e77d38d..e60e52d82db9 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -62,6 +62,14 @@ struct dma_map_ops { > int is_phys; > }; > > +struct iommu_data; > + > +struct iommu_dma_mapping { > + struct iommu_data *iommu; > + struct list_head node; > + struct kref kref; > +}; Could you please document the structure with kerneldoc ? > + > #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 0a685e0ab33e..af6179557005 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,8 @@ 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); > +extern void of_iommu_deconfigure(struct kref *kref); > > #else > > @@ -22,6 +27,11 @@ 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; > +} > +static inline void of_iommu_deconfigure(struct kref *kref) { } > > #endif /* CONFIG_OF_IOMMU */
On Thu, Sep 18, 2014 at 12:13:13PM +0100, Laurent Pinchart wrote: > Hi Will, Hi Laurent, > Thank you for the patch. Sorry for the delay in replying, I was at Connect last week and the email has backed up. > On Friday 12 September 2014 17:34:52 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. A > > list of iommu_dma_mapping structures are created for each device, which > > represent the set of IOMMU instances through which the device can > > master. The list is protected by a kref count and freed when no users > > remain. It is expected that DMA-mapping code will take a reference if it > > wishes to make use of the IOMMU information. [...] > > +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > > +{ > > + struct of_phandle_args iommu_spec; > > + struct iommu_dma_mapping *mapping; > > + struct device_node *np; > > + struct iommu_data *iommu = NULL; > > + int idx = 0; > > + > > + /* > > + * 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 iommu_data *data; > > + > > + np = iommu_spec.np; > > + data = of_iommu_get_data(np); > > + > > + if (!data || !data->ops || !data->ops->of_xlate) > > + goto err_put_node; > > + > > + if (!iommu) { > > + iommu = data; > > + } else if (iommu != data) { > > + /* We don't currently support multi-IOMMU masters */ > > + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > > + dev_name(dev)); > > + goto err_put_node; > > + } > > + > > + if (!data->ops->of_xlate(dev, &iommu_spec)) > > + goto err_put_node; > > + > > + of_node_put(np); > > + idx++; > > + } > > + > > + if (!iommu) > > + return NULL; > > + > > + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); > > + if (!mapping) > > + return NULL; > > + > > + kref_init(&mapping->kref); > > + INIT_LIST_HEAD(&mapping->node); > > I might be missing something, but that doesn't seem to match the commit > message. This creates a single iommu_dma_mapping per device, where is the list > supposed to be populated ? Right. I currently only populate the first node in the list, and the loop above barfs if we find multiple IOMMUs. I was hoping you'd add support for that later on, as you mentioned the need for this so I guess you can test it too? > > +void of_iommu_deconfigure(struct kref *kref) > > +{ > > + struct iommu_dma_mapping *mapping, *curr, *next; > > + > > + mapping = container_of(kref, struct iommu_dma_mapping, kref); > > + list_for_each_entry_safe(curr, next, &mapping->node, node) { > > + list_del(&curr->node); > > + kfree(curr); > > + } > > Don't you need to also kfree(mapping) here ? Yup, thanks. > > +} > > + > > Do you expect other users of of_iommu_deconfigure() than kref_put() ? If not, > how about exposing an of_iommu_put(struct iommu_dma_mapping *) that would call > kref_put() internally, and hiding of_iommu_deconfigure() ? That's a good idea, I'll do that. > > 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 1e944e77d38d..e60e52d82db9 100644 > > --- a/include/linux/dma-mapping.h > > +++ b/include/linux/dma-mapping.h > > @@ -62,6 +62,14 @@ struct dma_map_ops { > > int is_phys; > > }; > > > > +struct iommu_data; > > + > > +struct iommu_dma_mapping { > > + struct iommu_data *iommu; > > + struct list_head node; > > + struct kref kref; > > +}; > > Could you please document the structure with kerneldoc ? Ok, I'll try! Will
Hi Will, On Monday 22 September 2014 18:13:52 Will Deacon wrote: > On Thu, Sep 18, 2014 at 12:13:13PM +0100, Laurent Pinchart wrote: > > Hi Will, > > Hi Laurent, > > > Thank you for the patch. > > Sorry for the delay in replying, I was at Connect last week and the email > has backed up. No worries. > > On Friday 12 September 2014 17:34:52 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. A > >> list of iommu_dma_mapping structures are created for each device, which > >> represent the set of IOMMU instances through which the device can > >> master. The list is protected by a kref count and freed when no users > >> remain. It is expected that DMA-mapping code will take a reference if it > >> wishes to make use of the IOMMU information. > > [...] > > >> +struct iommu_dma_mapping *of_iommu_configure(struct device *dev) > >> +{ > >> + struct of_phandle_args iommu_spec; > >> + struct iommu_dma_mapping *mapping; > >> + struct device_node *np; > >> + struct iommu_data *iommu = NULL; > >> + int idx = 0; > >> + > >> + /* > >> + * 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 iommu_data *data; > >> + > >> + np = iommu_spec.np; > >> + data = of_iommu_get_data(np); > >> + > >> + if (!data || !data->ops || !data->ops->of_xlate) > >> + goto err_put_node; > >> + > >> + if (!iommu) { > >> + iommu = data; > >> + } else if (iommu != data) { > >> + /* We don't currently support multi-IOMMU masters */ > >> + pr_warn("Rejecting device %s with multiple IOMMU instances\n", > >> + dev_name(dev)); > >> + goto err_put_node; > >> + } > >> + > >> + if (!data->ops->of_xlate(dev, &iommu_spec)) > >> + goto err_put_node; > >> + > >> + of_node_put(np); > >> + idx++; > >> + } > >> + > >> + if (!iommu) > >> + return NULL; > >> + > >> + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); > >> + if (!mapping) > >> + return NULL; > >> + > >> + kref_init(&mapping->kref); > >> + INIT_LIST_HEAD(&mapping->node); > > > > I might be missing something, but that doesn't seem to match the commit > > message. This creates a single iommu_dma_mapping per device, where is the > > list supposed to be populated ? > > Right. I currently only populate the first node in the list, and the loop > above barfs if we find multiple IOMMUs. I was hoping you'd add support for > that later on, as you mentioned the need for this so I guess you can test it > too? I can, I was just puzzled by the mismatch between the code and commit message. As your patch series doesn't provide a complete end-to-end implementation it's not always easy to understand how you envision that implementation to work, and thus difficult to implement the required modifications to the IOMMU driver. > >> +void of_iommu_deconfigure(struct kref *kref) > >> +{ > >> + struct iommu_dma_mapping *mapping, *curr, *next; > >> + > >> + mapping = container_of(kref, struct iommu_dma_mapping, kref); > >> + list_for_each_entry_safe(curr, next, &mapping->node, node) { > >> + list_del(&curr->node); > >> + kfree(curr); > >> + } > > > > Don't you need to also kfree(mapping) here ? > > Yup, thanks. > > >> +} > >> + > > > > Do you expect other users of of_iommu_deconfigure() than kref_put() ? If > > not, how about exposing an of_iommu_put(struct iommu_dma_mapping *) that > > would call kref_put() internally, and hiding of_iommu_deconfigure() ? > > That's a good idea, I'll do that. > > >> 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 1e944e77d38d..e60e52d82db9 100644 > >> --- a/include/linux/dma-mapping.h > >> +++ b/include/linux/dma-mapping.h > >> @@ -62,6 +62,14 @@ struct dma_map_ops { > >> int is_phys; > >> }; > >> > >> +struct iommu_data; > >> + > >> +struct iommu_dma_mapping { > >> + struct iommu_data *iommu; > >> + struct list_head node; > >> + struct kref kref; > >> +}; > > > > Could you please document the structure with kerneldoc ? > > Ok, I'll try!
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 13d800c4ce25..8656b63f27ee 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -18,9 +18,11 @@ */ #include <linux/export.h> +#include <linux/iommu.h> #include <linux/limits.h> #include <linux/of.h> #include <linux/of_iommu.h> +#include <linux/slab.h> /** * of_get_dma_window - Parse *dma-window property and returns 0 if found. @@ -90,6 +92,74 @@ 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 device_node *np; + struct iommu_data *iommu = NULL; + int idx = 0; + + /* + * 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 iommu_data *data; + + np = iommu_spec.np; + data = of_iommu_get_data(np); + + if (!data || !data->ops || !data->ops->of_xlate) + goto err_put_node; + + if (!iommu) { + iommu = data; + } else if (iommu != data) { + /* We don't currently support multi-IOMMU masters */ + pr_warn("Rejecting device %s with multiple IOMMU instances\n", + dev_name(dev)); + goto err_put_node; + } + + if (!data->ops->of_xlate(dev, &iommu_spec)) + goto err_put_node; + + of_node_put(np); + idx++; + } + + if (!iommu) + return NULL; + + mapping = kzalloc(sizeof(*mapping), GFP_KERNEL); + if (!mapping) + return NULL; + + kref_init(&mapping->kref); + INIT_LIST_HEAD(&mapping->node); + mapping->iommu = iommu; + return mapping; + +err_put_node: + of_node_put(np); + return NULL; +} + +void of_iommu_deconfigure(struct kref *kref) +{ + struct iommu_dma_mapping *mapping, *curr, *next; + + mapping = container_of(kref, struct iommu_dma_mapping, kref); + list_for_each_entry_safe(curr, next, &mapping->node, node) { + list_del(&curr->node); + kfree(curr); + } +} + 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 1e944e77d38d..e60e52d82db9 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -62,6 +62,14 @@ struct dma_map_ops { int is_phys; }; +struct iommu_data; + +struct iommu_dma_mapping { + struct iommu_data *iommu; + struct list_head node; + struct kref kref; +}; + #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 0a685e0ab33e..af6179557005 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,8 @@ 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); +extern void of_iommu_deconfigure(struct kref *kref); #else @@ -22,6 +27,11 @@ 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; +} +static inline void of_iommu_deconfigure(struct kref *kref) { } #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. A list of iommu_dma_mapping structures are created for each device, which represent the set of IOMMU instances through which the device can master. The list is protected by a kref count and freed when no users remain. It is expected that DMA-mapping code will take a reference if it wishes to make use of the IOMMU information. Signed-off-by: Will Deacon <will.deacon@arm.com> --- drivers/iommu/Kconfig | 2 +- drivers/iommu/of_iommu.c | 70 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/dma-mapping.h | 8 ++++++ include/linux/of_iommu.h | 10 +++++++ 4 files changed, 89 insertions(+), 1 deletion(-)