On 03/02/17 16:15, Sricharan wrote: > Hi Lorenzo, Robin, > >> -----Original Message----- >> From: linux-arm-kernel [mailto:linux-arm-kernel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Sricharan R >> Sent: Friday, February 03, 2017 9:19 PM >> To: robin.murphy@xxxxxxx; will.deacon@xxxxxxx; joro@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; m.szyprowski@xxxxxxxxxxx; bhelgaas@xxxxxxxxxx; linux- >> pci@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; tn@xxxxxxxxxxxx; hanjun.guo@xxxxxxxxxx; okaya@xxxxxxxxxxxxxx >> Cc: sricharan@xxxxxxxxxxxxxx >> Subject: [PATCH V8 08/11] drivers: acpi: Handle IOMMU lookup failure with deferred probing or error >> >> This is an equivalent to the DT's handling of the iommu master's probe >> with deferred probing when the corrsponding iommu is not probed yet. >> 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 firmware 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. >> >> Tested-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx> >> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> >> Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx> >> --- >> drivers/acpi/arm64/iort.c | 25 ++++++++++++++++++++++++- >> drivers/acpi/scan.c | 7 +++++-- >> drivers/base/dma-mapping.c | 2 +- >> include/acpi/acpi_bus.h | 2 +- >> include/linux/acpi.h | 7 +++++-- >> 5 files changed, 36 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index bf0ed09..d01bae8 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -550,8 +550,17 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev, >> return NULL; >> >> ops = iommu_get_instance(iort_fwnode); >> + /* >> + * If the ops look-up fails, this means that either >> + * the SMMU drivers have not been probed yet or that >> + * the SMMU drivers are not built in the kernel; >> + * Depending on whether the SMMU drivers are built-in >> + * in the kernel or not, defer the IOMMU configuration >> + * or just abort it. >> + */ >> if (!ops) >> - return NULL; >> + return iort_iommu_driver_enabled(node->type) ? >> + ERR_PTR(-EPROBE_DEFER) : NULL; >> >> ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops); >> } >> @@ -625,12 +634,26 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) >> >> while (parent) { >> ops = iort_iommu_xlate(dev, parent, streamid); >> + if (IS_ERR_OR_NULL(ops)) >> + return ops; >> >> parent = iort_node_get_id(node, &streamid, >> IORT_IOMMU_TYPE, i++); >> } >> } >> >> + /* >> + * If we have reason to believe the IOMMU driver missed the initial >> + * add_device callback for dev, replay it to get things in order. >> + */ >> + if (!IS_ERR_OR_NULL(ops) && ops->add_device && >> + dev->bus && !dev->iommu_group) { >> + int err = ops->add_device(dev); >> + >> + if (err) >> + ops = ERR_PTR(err); >> + } >> + > > On the last post we discussed that the above replay hunk can be made > common. In trying to do that, i ended up with a patch like below. But not > sure if that should be a part of this series though. I tested with OF devices > and would have to be tested with ACPI devices once. Nothing changes > functionally because of this ideally. Should be split in two patches though. > > Regards, > Sricharan > > From aafbf2c97375a086327504f2367eaf9197c719b1 Mon Sep 17 00:00:00 2001 > From: Sricharan R <sricharan@xxxxxxxxxxxxxx> > Date: Fri, 3 Feb 2017 15:24:47 +0530 > Subject: [PATCH] drivers: iommu: Add iommu_add_device api > > The code to call IOMMU driver's add_device is same > for both OF and ACPI cases. So add an api which can > be shared across both the places. > > Also, now with probe-deferral the iommu master devices gets > added to the respective iommus during probe time instead > of device creation time. The xlate callbacks of iommu > drivers are also called only at probe time. As a result > the add_iommu_group which gets called when the iommu is > registered to add all devices created before the iommu > becomes dummy. Similar the BUS_NOTIFY_ADD_DEVICE notification > also is not needed. So just cleanup those code. > > Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx> > --- > drivers/acpi/arm64/iort.c | 12 +------- > drivers/iommu/iommu.c | 70 ++++++++++++----------------------------------- > drivers/iommu/of_iommu.c | 11 +------- > include/linux/iommu.h | 8 ++++++ > 4 files changed, 27 insertions(+), 74 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index ac45623..ab2a554 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -642,17 +642,7 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev) > } > } > > - /* > - * If we have reason to believe the IOMMU driver missed the initial > - * add_device callback for dev, replay it to get things in order. > - */ > - if (!IS_ERR_OR_NULL(ops) && ops->add_device && > - dev->bus && !dev->iommu_group) { > - int err = ops->add_device(dev); > - > - if (err) > - ops = ERR_PTR(err); > - } > + ops = iommu_add_device(dev, ops); > > return ops; > } > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index b752c3d..750552d 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1015,41 +1015,6 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group) > return group->default_domain; > } > > -static int add_iommu_group(struct device *dev, void *data) > -{ > - struct iommu_callback_data *cb = data; > - const struct iommu_ops *ops = cb->ops; > - int ret; > - > - if (!ops->add_device) > - return 0; > - > - WARN_ON(dev->iommu_group); > - > - ret = ops->add_device(dev); > - > - /* > - * We ignore -ENODEV errors for now, as they just mean that the > - * device is not translated by an IOMMU. We still care about > - * other errors and fail to initialize when they happen. > - */ > - if (ret == -ENODEV) > - ret = 0; > - > - return ret; > -} > - > -static int remove_iommu_group(struct device *dev, void *data) > -{ > - struct iommu_callback_data *cb = data; > - const struct iommu_ops *ops = cb->ops; > - > - if (ops->remove_device && dev->iommu_group) > - ops->remove_device(dev); > - > - return 0; > -} > - > static int iommu_bus_notifier(struct notifier_block *nb, > unsigned long action, void *data) > { > @@ -1059,13 +1024,10 @@ static int iommu_bus_notifier(struct notifier_block *nb, > unsigned long group_action = 0; > > /* > - * ADD/DEL call into iommu driver ops if provided, which may > + * DEL call into iommu driver ops if provided, which may > * result in ADD/DEL notifiers to group->notifier > */ > - if (action == BUS_NOTIFY_ADD_DEVICE) { > - if (ops->add_device) > - return ops->add_device(dev); I'm pretty sure this completely breaks x86 and anyone else... Anyway, I'd be inclined to leave this alone for now - it's essentially just a workaround to mesh the per-instance probing behaviour with the per-bus ops model, so it's bound to be a bit ugly. Moving the IOMMU core code over to a per-instance model will inevitably subsume the underlying problem, and I think a bit of duplication is probably the lesser of two evils in the meantime. On which note, I need to have a good look at Joerg's shiny new series :) Robin. > - } else if (action == BUS_NOTIFY_REMOVED_DEVICE) { > + if (action == BUS_NOTIFY_REMOVED_DEVICE) { > if (ops->remove_device && dev->iommu_group) { > ops->remove_device(dev); > return 0; > @@ -1107,9 +1069,6 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops) > { > int err; > struct notifier_block *nb; > - struct iommu_callback_data cb = { > - .ops = ops, > - }; > > nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL); > if (!nb) > @@ -1121,18 +1080,8 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops) > if (err) > goto out_free; > > - err = bus_for_each_dev(bus, NULL, &cb, add_iommu_group); > - if (err) > - goto out_err; > - > - > return 0; > > -out_err: > - /* Clean up */ > - bus_for_each_dev(bus, NULL, &cb, remove_iommu_group); > - bus_unregister_notifier(bus, nb); > - > out_free: > kfree(nb); > > @@ -1253,6 +1202,21 @@ static int __iommu_attach_device(struct iommu_domain *domain, > return ret; > } > > +const struct iommu_ops *iommu_add_device(struct device *dev, > + const struct iommu_ops *ops) > +{ > + if (!IS_ERR_OR_NULL(ops) && ops->add_device && > + dev->bus && !dev->iommu_group) { > + int err = ops->add_device(dev); > + > + if (err) > + ops = ERR_PTR(err); > + } > + > + return ops; > +} > +EXPORT_SYMBOL_GPL(iommu_add_device); > + > int iommu_attach_device(struct iommu_domain *domain, struct device *dev) > { > struct iommu_group *group; > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 2d04663..4b49dc2 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -224,17 +224,8 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > ops = of_pci_iommu_init(to_pci_dev(dev), master_np); > else > ops = of_platform_iommu_init(dev, master_np); > - /* > - * If we have reason to believe the IOMMU driver missed the initial > - * add_device callback for dev, replay it to get things in order. > - */ > - if (!IS_ERR_OR_NULL(ops) && ops->add_device && > - dev->bus && !dev->iommu_group) { > - int err = ops->add_device(dev); > > - if (err) > - ops = ERR_PTR(err); > - } > + ops = iommu_add_device(dev, ops); > > return ops; > } > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index add30c3..1d54a91 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -232,6 +232,8 @@ struct iommu_ops { > extern struct iommu_domain *iommu_domain_alloc(struct bus_type *bus); > extern struct iommu_group *iommu_group_get_by_id(int id); > extern void iommu_domain_free(struct iommu_domain *domain); > +extern const struct iommu_ops *iommu_add_device(struct device *dev, > + const struct iommu_ops *ops); > extern int iommu_attach_device(struct iommu_domain *domain, > struct device *dev); > extern void iommu_detach_device(struct iommu_domain *domain, > @@ -405,6 +407,12 @@ static inline void iommu_domain_free(struct iommu_domain *domain) > { > } > > +static inline const struct iommu_ops *iommu_add_device(struct device *dev, > + const struct iommu_ops *ops) > +{ > + return NULL; > +} > + > static inline int iommu_attach_device(struct iommu_domain *domain, > struct device *dev) > { > -- > 1.8.2.1 >
