Hi 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@lists.linux- >foundation.org; >>> 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... > ha, i just missed thinking about non-arm, for this super cleanup :) >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 :) > Agree, better way for this cleanup is with the per-instance model and let the series go otherwise. Regards, Sricharan
