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:

quoted

Hi Will,

Hi Laurent,

quoted

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.

quoted

On Friday 12 September 2014 17:34:52 Will Deacon wrote:

quoted

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.

[...]

quoted

quoted

+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",

quoted

quoted

+				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.

quoted

quoted

+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.

quoted

quoted

+}
+

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.

quoted

quoted

 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!

-- 
Regards,

Laurent Pinchart