Hi Robin,
>> From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>>
>> Failures to look up an IOMMU when parsing the DT iommus property need to
>> be handled separately from the .of_xlate() failures to support deferred
>> probing.
>>
>> The lack of a registered IOMMU can be caused by the lack of a driver for
>> the IOMMU, the IOMMU device probe not having been performed yet, having
>> been deferred, or having failed.
>>
>> The first case occurs when the device tree describes the bus master and
>> IOMMU topology correctly but no device driver exists for the IOMMU yet
>> or the device driver has not been compiled in. Return NULL, the caller
>> will configure the device without an IOMMU.
>>
>> The second and third cases are handled by deferring the probe of the bus
>> master device which will eventually get reprobed after the IOMMU.
>>
>> The last case is currently handled by deferring the probe of the bus
>> master device as well. A mechanism to either configure the bus master
>> device without an IOMMU or to fail the bus master device probe depending
>> on whether the IOMMU is optional or mandatory would be a good
>> enhancement.
>>
>> The current iommu framework handles pci and non-pci devices separately,
>> so taken care of both the paths in this patch. The iommu's add_device
>> callback is invoked after the master's configuration data is added in
>> xlate. This is needed because the iommu core calls add_device callback
>> during the BUS_ADD_DEVICE notifier, which is of no use now. Eventually
>> that call has to be removed.
>
>Laurent's signoff seems to have gone missing here.
Ah, preserved his authorship, but missed this. Will add it back.
>
>> Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
>> ---
>> drivers/iommu/of_iommu.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
>> drivers/of/device.c | 7 ++++++-
>> include/linux/of_device.h | 6 ++++--
>> 3 files changed, 53 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 5b82862..1a5e28b 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -23,6 +23,7 @@
>> #include <linux/of.h>
>> #include <linux/of_iommu.h>
>> #include <linux/of_pci.h>
>> +#include <linux/pci.h>
>> #include <linux/slab.h>
>>
>> static const struct of_device_id __iommu_of_table_sentinel
>> @@ -167,12 +168,29 @@ static const struct iommu_ops
>> return NULL;
>>
>> ops = of_iommu_get_ops(iommu_spec.np);
>> +
>> + if (!ops) {
>> + const struct of_device_id *oid;
>> +
>> + oid = of_match_node(&__iommu_of_table, iommu_spec.np);
>> + ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
>
>It would seem even simpler and more convenient to roll this into the end
>of of_iommu_get_ops():
ok, understand. Will move this there.
>
> if (!ops && of_match_node(&__iommu_of_table, iommu_spec.np))
> ops = ERR_PTR(-EPROBE_DEFER);
>
>then just fix up the existing !ops checks to !IS_ERR(ops) appropriately.
ok.
>
>> + return ops;
>> + }
>> +
>> if (!ops || !ops->of_xlate ||
>> iommu_fwspec_init(&pdev->dev, &iommu_spec.np->fwnode, ops) ||
>> ops->of_xlate(&pdev->dev, &iommu_spec))
>> ops = NULL;
>>
>> + if (ops && ops->add_device) {
>> + ops = (ops->add_device(&pdev->dev) == 0) ? ops : NULL;
>
>This is a really obtuse way of writing
>
> if (ops->add_device(...))
> ops = NULL;
ok, will change it this way.
>
>However, given that we're now returning an ERR_PTR, it would be worth
>capturing the return value of add_device and propagating the error back
>up - if the IOMMU driver has refused one of its masters for some reason,
>it probably isn't safe to allow that device to do DMA either way, so we
>ought to prevent it probing at all.
>
ok, will return the err value instead of NULL here.
>> +
>> + if (!ops)
>> + dev_err(&pdev->dev, "Failed to setup iommu ops\n");
>
>Given the above, I think this should be more along the lines of "Device
>rejected by IOMMU: %d" with the actual error code as well. It's one of
>those "if you ever see it, you're going to have to debug it" cases, so
>the clearer the better.
>
ok, will reword the print.
>> + }
>> +
>> of_node_put(iommu_spec.np);
>> +
>> return ops;
>> }
>>
>> @@ -183,9 +201,15 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>> struct device_node *np;
>> const struct iommu_ops *ops = NULL;
>> int idx = 0;
>> + struct device *bridge;
>> +
>> + if (dev_is_pci(dev)) {
>> + bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>>
>> - if (dev_is_pci(dev))
>> - return of_pci_iommu_configure(to_pci_dev(dev), master_np);
>> + if (bridge && bridge->parent && bridge->parent->of_node)
>> + return of_pci_iommu_configure(to_pci_dev(dev),
>> + bridge->parent->of_node);
>
> else fall through to treating it as a platform device?
>
ha, surely wrong. Will correct this and move it to the of_pci_iommu_configure helper.
>...that's not right. Anyway, this is PCI-specific stuff so should be in
>the PCI-specific helper function.
>
>> + }
>>
>> /*
>> * We don't currently walk up the tree looking for a parent IOMMU.
>> @@ -198,6 +222,14 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>> np = iommu_spec.np;
>> ops = of_iommu_get_ops(np);
>>
>> + if (!ops) {
>> + const struct of_device_id *oid;
>> +
>> + oid = of_match_node(&__iommu_of_table, np);
>> + ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
>> + goto err_put_node;
>> + }
>
>Same comment as above. Especially since moving it to of_iommu_get_ops()
>would obviate the duplication.
ok.
>
>> +
>> if (!ops || !ops->of_xlate ||
>> iommu_fwspec_init(dev, &np->fwnode, ops) ||
>> ops->of_xlate(dev, &iommu_spec))
>> @@ -207,11 +239,18 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>> idx++;
>> }
>>
>> + if (ops && ops->add_device) {
>> + ops = (ops->add_device(dev) == 0) ? ops : NULL;
>> +
>> + if (!ops)
>> + dev_err(dev, "Failed to setup iommu_ops\n");
>> + }
>> +
>
>It would be nice to avoid duplicating this as well.
ok, sure, will correct.
Regards,
Sricharan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
