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.dea...@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 */
--
Regards,
Laurent Pinchart
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
