[RFC PATCH v3 4/7] iommu: provide helper function to configure an IOMMU for an of master

[laurent.pinchart@ideasonboard.com (Laurent Pinchart)]

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!

-- 
Regards,

Laurent Pinchart