[PATCH V8 08/11] drivers: acpi: Handle IOMMU lookup failure with deferred probing or error
Robin Murphy
robin.murphy at arm.com
Fri Feb 3 09:39:18 PST 2017
On 03/02/17 16:15, Sricharan wrote:
> Hi Lorenzo, Robin,
>
>> -----Original Message-----
>> From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Sricharan R
>> Sent: Friday, February 03, 2017 9:19 PM
>> To: robin.murphy at arm.com; will.deacon at arm.com; joro at 8bytes.org; lorenzo.pieralisi at arm.com; iommu at lists.linux-foundation.org;
>> linux-arm-kernel at lists.infradead.org; linux-arm-msm at vger.kernel.org; m.szyprowski at samsung.com; bhelgaas at google.com; linux-
>> pci at vger.kernel.org; linux-acpi at vger.kernel.org; tn at semihalf.com; hanjun.guo at linaro.org; okaya at codeaurora.org
>> Cc: sricharan at codeaurora.org
>> 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 at linaro.org>
>> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
>> Signed-off-by: Sricharan R <sricharan at codeaurora.org>
>> ---
>> 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 at codeaurora.org>
> 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 at codeaurora.org>
> ---
> 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
>
More information about the linux-arm-kernel
mailing list