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