iommu: retire bus_set_iommu()
Subsystems (18)
Robin Murphy
reviewerv4 cover iommu: retire bus_set_iommu()
v3: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
As promised, here's the rebased v4 which I hope is ready to go now. Besides the new patch for s390, there are just a few small improvements over v3 to make things even simpler and clearer.
Thanks,
Robin.
Matthew Rosato (1):
iommu/s390: Fail probe for non-PCI devices
Robin Murphy (15):
iommu/vt-d: Handle race between registration and device probe iommu/amd: Handle race between registration and device probe
iommu: Always register bus notifiers
iommu: Move bus setup to IOMMU device registration iommu/amd: Clean up bus_set_iommu()
iommu/arm-smmu: Clean up bus_set_iommu() iommu/arm-smmu-v3: Clean up bus_set_iommu() iommu/dart: Clean up bus_set_iommu()
iommu/exynos: Clean up bus_set_iommu() iommu/ipmmu-vmsa: Clean up bus_set_iommu() iommu/mtk: Clean up bus_set_iommu()
iommu/omap: Clean up bus_set_iommu() iommu/tegra-smmu: Clean up bus_set_iommu() iommu/virtio: Clean up bus_set_iommu()
iommu: Clean up bus_set_iommu()
drivers/iommu/amd/amd_iommu.h | 1 - drivers/iommu/amd/init.c | 9 +- drivers/iommu/amd/iommu.c | 25 +--- drivers/iommu/apple-dart.c | 30 +---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +------- drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +----------- drivers/iommu/arm/arm-smmu/qcom_iommu.c | 4 - drivers/iommu/exynos-iommu.c | 9 -- drivers/iommu/fsl_pamu_domain.c | 4 - drivers/iommu/intel/iommu.c | 4 +- drivers/iommu/iommu.c | 137 +++++++++----------- drivers/iommu/ipmmu-vmsa.c | 35 +---- drivers/iommu/msm_iommu.c | 2 - drivers/iommu/mtk_iommu.c | 24 +--- drivers/iommu/mtk_iommu_v1.c | 13 +- drivers/iommu/omap-iommu.c | 6 - drivers/iommu/rockchip-iommu.c | 2 - drivers/iommu/s390-iommu.c | 13 +- drivers/iommu/sprd-iommu.c | 5 - drivers/iommu/sun50i-iommu.c | 2 - drivers/iommu/tegra-smmu.c | 29 +---- drivers/iommu/virtio-iommu.c | 25 ---- include/linux/iommu.h | 1 - 23 files changed, 86 insertions(+), 431 deletions(-)
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 1/16 iommu/vt-d: Handle race between registration and device probe
Currently we rely on registering all our instances before initially allowing any .probe_device calls via bus_set_iommu(). In preparation for phasing out the latter, make sure we won't inadvertently return success for a device associated with a known but not yet registered instance, otherwise we'll run straight into iommu_group_get_for_dev() trying to use NULL ops.
That also highlights an issue with intel_iommu_get_resv_regions() taking dmar_global_lock from within a section where intel_iommu_init() already holds it, which already exists via probe_acpi_namespace_devices() when an ANDD device is probed, but gets more obvious with the upcoming change to iommu_device_register(). Since they are both read locks it manages not to deadlock in practice, and a more in-depth rework of this locking is underway, so no attempt is made to address it here.
---
v4: Update commit message
drivers/iommu/intel/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/iommu/intel/iommu.c +1 -1
@@ -4449,7 +4449,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)u8 bus, devfn; iommu =device_to_iommu (dev, &bus, &devfn); -if (!iommu) +if (!iommu || !iommu->iommu.ops)return ERR_PTR (-ENODEV ); info =kzalloc (sizeof (*info),GFP_KERNEL );
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 2/16 iommu/amd: Handle race between registration and device probe
In reply to Robin MurphyAs for the Intel driver, make sure the AMD driver can cope with seeing .probe_device calls without having to wait for all known instances to register first.
---
v4: No change
drivers/iommu/amd/iommu.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/iommu/amd/iommu.c +4 -0
@@ -1851,6 +1851,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)if (!iommu)return ERR_PTR (-ENODEV ); +/* Not registered yet? */ +if (!iommu->iommu.ops) +return ERR_PTR (-ENODEV ); +if (dev_iommu_priv_get (dev))return &iommu->iommu;
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 3/16 iommu/s390: Fail probe for non-PCI devices
In reply to Robin MurphyFrom: Matthew Rosato <[email protected]>
s390-iommu only supports pci_bus_type today.
---
v4: New
drivers/iommu/s390-iommu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/iommu/s390-iommu.c +6 -1
@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,static struct iommu_device *s390_iommu_probe_device (struct device *dev) { -struct zpci_dev *zdev =to_zpci_dev (dev); +struct zpci_dev *zdev; + +if (!dev_is_pci (dev)) +return ERR_PTR (-ENODEV ); + + zdev =to_zpci_dev (dev);return &zdev->iommu_dev; }
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 4/16 iommu: Always register bus notifiers
In reply to Robin MurphyThe number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling iommu_probe_device() before bus->iommu_ops is set will simply return -ENODEV and not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.
---
v4: Squash iommu_bus_init() entirely, for maximum simplicity. Ignoring
the return from bus_register_notifier() is common, so hopefully it's all pretty self-explanatory now.
drivers/iommu/iommu.c | 72 ++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 35 deletions(-)
drivers/iommu/iommu.c +37 -35
@@ -6,6 +6,7 @@#define pr_fmt (fmt) "iommu: " fmt +#include <linux/amba/bus.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/kernel.h>
@@ -16,11 +17,13 @@#include <linux/export.h> #include <linux/slab.h> #include <linux/errno.h> +#include <linux/host1x_context_bus.h> #include <linux/iommu.h> #include <linux/idr.h> #include <linux/err.h> #include <linux/pci.h> #include <linux/bitops.h> +#include <linux/platform_device.h> #include <linux/property.h> #include <linux/fsl/mc.h> #include <linux/module.h>
@@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {#define IOMMU_CMD_LINE_DMA_API BIT(0)#define IOMMU_CMD_LINE_STRICT BIT(1) +static int iommu_bus_notifier (struct notifier_block *nb, +unsigned long action,void *data);static int iommu_alloc_default_domain (struct iommu_group *group,struct device *dev); static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \static LIST_HEAD (iommu_device_list);static DEFINE_SPINLOCK (iommu_device_lock); +static struct bus_type *const iommu_buses[] = { + &platform_bus_type, +#ifdef CONFIG_PCI + &pci_bus_type, +#endif +#ifdef CONFIG_ARM_AMBA + &amba_bustype, +#endif +#ifdef CONFIG_FSL_MC_BUS + &fsl_mc_bus_type, +#endif +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS + &host1x_context_device_bus_type, +#endif +}; +/* * Use a function instead of an array here because the domain-type is a * bit-field, so an array would waste memory.
@@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)static int __init iommu_subsys_init (void ) { +struct notifier_block *nb; +if (!(iommu_cmd_line &IOMMU_CMD_LINE_DMA_API )) {if (IS_ENABLED (CONFIG_IOMMU_DEFAULT_PASSTHROUGH ))iommu_set_default_passthrough (false);
@@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line &IOMMU_CMD_LINE_STRICT ) ? "(set via kernel command line)" : ""); + nb =kcalloc (ARRAY_SIZE (iommu_buses),sizeof (*nb),GFP_KERNEL ); +if (!nb) +return -ENOMEM ; + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { + nb[i].notifier_call = iommu_bus_notifier; +bus_register_notifier (iommu_buses[i], &nb[i]); + } +return 0 ; }subsys_initcall (iommu_subsys_init);
@@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)return ret; } -static int iommu_bus_init (struct bus_type *bus,const struct iommu_ops *ops) -{ -struct notifier_block *nb; -int err; - - nb =kzalloc (sizeof (struct notifier_block ),GFP_KERNEL ); -if (!nb) -return -ENOMEM ; - - nb->notifier_call = iommu_bus_notifier; - - err =bus_register_notifier (bus, nb); -if (err) - goto out_free; - - err =bus_iommu_probe (bus); -if (err) - goto out_err; - - -return 0 ; - -out_err: -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); -bus_unregister_notifier (bus, nb); - -out_free: -kfree (nb); - -return err; -} -/** * bus_set_iommu - set iommu-callbacks for the bus * @bus: bus.
@@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) bus->iommu_ops = ops;/* Do IOMMU specific setup for this bus-type */ - err =iommu_bus_init (bus, ops); -if (err) + err =bus_iommu_probe (bus); +if (err) { +/* Clean up */ +bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); bus->iommu_ops =NULL ; + }return err; }
Signature · 1 line
2.36.1.dirty
RE: [PATCH v4 04/16] iommu: Always register bus notifiers
Quoted 10 lines
From: Robin Murphy <[email protected]>
Sent: Tuesday, August 16, 2022 12:20 AMThe number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.
Quoted 4 lines
Tested-by Marek Szyprowski <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Re: [PATCH v4 04/16] iommu: Always register bus notifiers
In reply to Robin MurphyQuoted 91 lines · Robin Murphy
The number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.Tested-by Marek Szyprowski <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
v4: Squash
iommu_bus_init()entirely, for maximum simplicity. Ignoring
the return frombus_register_notifier()is common, so hopefully it's all pretty self-explanatory now.drivers/iommu/iommu.c | 72 ++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 35 deletions(-)drivers/iommu/iommu.c +23
-0@@ -6,6 +6,7 @@#define pr_fmt (fmt) "iommu: " fmt +#include <linux/amba/bus.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/kernel.h> @@ -16,11 +17,13 @@#include <linux/export.h> #include <linux/slab.h> #include <linux/errno.h> +#include <linux/host1x_context_bus.h> #include <linux/iommu.h> #include <linux/idr.h> #include <linux/err.h> #include <linux/pci.h> #include <linux/bitops.h> +#include <linux/platform_device.h> #include <linux/property.h> #include <linux/fsl/mc.h> #include <linux/module.h> @@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {#define IOMMU_CMD_LINE_DMA_API BIT(0)#define IOMMU_CMD_LINE_STRICT BIT(1) +static int iommu_bus_notifier (struct notifier_block *nb, +unsigned long action,void *data);static int iommu_alloc_default_domain (struct iommu_group *group,struct device *dev); static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,@@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \static LIST_HEAD (iommu_device_list);static DEFINE_SPINLOCK (iommu_device_lock); +static struct bus_type *const iommu_buses[] = { + &platform_bus_type, +#ifdef CONFIG_PCI + &pci_bus_type, +#endif +#ifdef CONFIG_ARM_AMBA + &amba_bustype, +#endif +#ifdef CONFIG_FSL_MC_BUS + &fsl_mc_bus_type, +#endif +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS + &host1x_context_device_bus_type, +#endif +}; +/* * Use a function instead of an array here because the domain-type is a * bit-field, so an array would waste memory. @@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)static int __init iommu_subsys_init (void ) { +struct notifier_block *nb; +if (!(iommu_cmd_line &IOMMU_CMD_LINE_DMA_API )) {if (IS_ENABLED (CONFIG_IOMMU_DEFAULT_PASSTHROUGH ))iommu_set_default_passthrough (false);@@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line &IOMMU_CMD_LINE_STRICT ) ? "(set via kernel command line)" : "");
drivers/iommu/iommu.c static int __init iommu_subsys_init(void) · line 186
Quoted 10 lines
drivers/iommu/iommu.c +9
-0+ nb =kcalloc (ARRAY_SIZE (iommu_buses),sizeof (*nb),GFP_KERNEL ); +if (!nb) +return -ENOMEM ; + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { + nb[i].notifier_call = iommu_bus_notifier; +bus_register_notifier (iommu_buses[i], &nb[i]); + } +
Carrying on the community's general disdain for notifiers, can we drop the bus notifier and just call iommu_probe_device() directly from device_add()? That way, you also won't need to keep an ifdef-ed array of buses and it'll be easy to tell from driver core code that iommu stuff is happening as devices are added. And I'd probably move that call to be AFTER some of the fw_devlink stuff is done too.
Also, would it be possible to delay this until probe (sorry, don't have enough of IOMMU details in my head) and call iommu_probe_device() from really_probe() like we do to set up pinctrl, etc? That'd be ideal.
-Saravana
Quoted 62 lines
drivers/iommu/iommu.c +5
-36return 0 ; }subsys_initcall (iommu_subsys_init);@@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)return ret; } -static int iommu_bus_init (struct bus_type *bus,const struct iommu_ops *ops) -{ -struct notifier_block *nb; -int err; - - nb =kzalloc (sizeof (struct notifier_block ),GFP_KERNEL ); -if (!nb) -return -ENOMEM ; - - nb->notifier_call = iommu_bus_notifier; - - err =bus_register_notifier (bus, nb); -if (err) - goto out_free; - - err =bus_iommu_probe (bus); -if (err) - goto out_err; - - -return 0 ; - -out_err: -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); -bus_unregister_notifier (bus, nb); - -out_free: -kfree (nb); - -return err; -} -/** * bus_set_iommu - set iommu-callbacks for the bus * @bus: bus. @@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) bus->iommu_ops = ops;/* Do IOMMU specific setup for this bus-type */ - err =iommu_bus_init (bus, ops); -if (err) + err =bus_iommu_probe (bus); +if (err) { +/* Clean up */ +bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); bus->iommu_ops =NULL ; + }return err; } --2.36.1.dirty
Robin Murphy
reviewerRe: [PATCH v4 04/16] iommu: Always register bus notifiers
Quoted 93 lines · Saravana Kannan
Quoted 92 lines · Robin Murphy
The number of bus types that the IOMMU subsystem deals with is small and manageable, so pull that list into core code as a first step towards cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device()beforebus->iommu_opsis set will simply return-ENODEVand not break the notifier call chain, so there should be no harm in proactively registering all our bus notifiers at init time.Tested-by Marek Szyprowski <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
v4: Squash
iommu_bus_init()entirely, for maximum simplicity. Ignoring
the return frombus_register_notifier()is common, so hopefully it's all pretty self-explanatory now.drivers/iommu/iommu.c | 72 ++++++++++++++++++++++--------------------- 1 file changed, 37 insertions(+), 35 deletions(-)drivers/iommu/iommu.c +23
-0@@ -6,6 +6,7 @@#define pr_fmt (fmt) "iommu: " fmt +#include <linux/amba/bus.h> #include <linux/device.h> #include <linux/dma-iommu.h> #include <linux/kernel.h> @@ -16,11 +17,13 @@#include <linux/export.h> #include <linux/slab.h> #include <linux/errno.h> +#include <linux/host1x_context_bus.h> #include <linux/iommu.h> #include <linux/idr.h> #include <linux/err.h> #include <linux/pci.h> #include <linux/bitops.h> +#include <linux/platform_device.h> #include <linux/property.h> #include <linux/fsl/mc.h> #include <linux/module.h> @@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {#define IOMMU_CMD_LINE_DMA_API BIT(0)#define IOMMU_CMD_LINE_STRICT BIT(1) +static int iommu_bus_notifier (struct notifier_block *nb, +unsigned long action,void *data);static int iommu_alloc_default_domain (struct iommu_group *group,struct device *dev); static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,@@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name = \static LIST_HEAD (iommu_device_list);static DEFINE_SPINLOCK (iommu_device_lock); +static struct bus_type *const iommu_buses[] = { + &platform_bus_type, +#ifdef CONFIG_PCI + &pci_bus_type, +#endif +#ifdef CONFIG_ARM_AMBA + &amba_bustype, +#endif +#ifdef CONFIG_FSL_MC_BUS + &fsl_mc_bus_type, +#endif +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS + &host1x_context_device_bus_type, +#endif +}; +/* * Use a function instead of an array here because the domain-type is a * bit-field, so an array would waste memory. @@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)static int __init iommu_subsys_init (void ) { +struct notifier_block *nb; +if (!(iommu_cmd_line &IOMMU_CMD_LINE_DMA_API )) {if (IS_ENABLED (CONFIG_IOMMU_DEFAULT_PASSTHROUGH ))iommu_set_default_passthrough (false);@@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void) (iommu_cmd_line &IOMMU_CMD_LINE_STRICT ) ? "(set via kernel command line)" : "");
Quoted 16 lines
drivers/iommu/iommu.c static int __init iommu_subsys_init(void) · line 186
Quoted 9 lines
drivers/iommu/iommu.c +9
-0+ nb =kcalloc (ARRAY_SIZE (iommu_buses),sizeof (*nb),GFP_KERNEL ); +if (!nb) +return -ENOMEM ; + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) { + nb[i].notifier_call = iommu_bus_notifier; +bus_register_notifier (iommu_buses[i], &nb[i]); + } +Carrying on the community's general disdain for notifiers, can we drop the bus notifier and just call
iommu_probe_device()directly fromdevice_add()? That way, you also won't need to keep an ifdef-ed array of buses and it'll be easy to tell from driver core code that iommu stuff is happening as devices are added. And I'd probably move that call to be AFTER some of thefw_devlinkstuff is done too.
Yup, we're working in that general direction, this is just big and moves slow :)
One of the next steps after this is unpicking {of,acpi}_iommu_configure() for iommu_probe_device() to take ownership of firmware parsing, after which it should be sufficiently self-contained that the notifier might indeed become the worst impediment to further reasoning. We'll still want to limit things to the small subset of buses where IOMMUs are at all relevant (one of the reasons that I stopped short of trying to grovel the whole list of registered buses out of the driver core here), but since there's a bus-specific element involved in parsing the firmware bindings, I'm expecting it to work out quite neatly in the end.
Quoted 4 lines
Also, would it be possible to delay this until probe (sorry, don't have enough of IOMMU details in my head) and call
iommu_probe_device()fromreally_probe()like we do to set up pinctrl, etc? That'd be ideal.
No, that's not so good, and in fact the stuff that can currently happen at driver probe time is already problematic and responsible for various subtle breakage. There are things about the IOMMU topology that need to be known regardless of whether drivers exist for all the devices, so now that fw_devlink can take care of waiting for IOMMU drivers to load, the rest of IOMMU setup (other than DMA ops) really does want to move back to device_add() time.
Thanks,
Robin.
Quoted 66 lines
-Saravana
Quoted 62 lines
drivers/iommu/iommu.c +5
-36return 0 ; }subsys_initcall (iommu_subsys_init);@@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)return ret; } -static int iommu_bus_init (struct bus_type *bus,const struct iommu_ops *ops) -{ -struct notifier_block *nb; -int err; - - nb =kzalloc (sizeof (struct notifier_block ),GFP_KERNEL ); -if (!nb) -return -ENOMEM ; - - nb->notifier_call = iommu_bus_notifier; - - err =bus_register_notifier (bus, nb); -if (err) - goto out_free; - - err =bus_iommu_probe (bus); -if (err) - goto out_err; - - -return 0 ; - -out_err: -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); -bus_unregister_notifier (bus, nb); - -out_free: -kfree (nb); - -return err; -} -/** * bus_set_iommu - set iommu-callbacks for the bus * @bus: bus. @@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops) bus->iommu_ops = ops;/* Do IOMMU specific setup for this bus-type */ - err =iommu_bus_init (bus, ops); -if (err) + err =bus_iommu_probe (bus); +if (err) { +/* Clean up */ +bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); bus->iommu_ops =NULL ; + }return err; } --2.36.1.dirty
Robin Murphy
reviewerPATCH v4 5/16 iommu: Move bus setup to IOMMU device registration
In reply to Robin MurphyMove the bus setup to iommu_device_register(). This should allow bus_iommu_probe() to be correctly replayed for multiple IOMMU instances, and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
---
v4: Factor out the ops check in iommu_device_register() to keep the loop even simpler, and comment the nominal change in behaviour
drivers/iommu/iommu.c | 55 +++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 25 deletions(-)
drivers/iommu/iommu.c +30 -25
@@ -188,6 +188,14 @@ static int __init iommu_subsys_init(void) }subsys_initcall (iommu_subsys_init); +static int remove_iommu_group (struct device *dev,void *data) +{ +if (dev->iommu && dev->iommu->iommu_dev == data) +iommu_release_device (dev); + +return 0 ; +} +/** * iommu_device_register() - Register an IOMMU hardware instance * @iommu: IOMMU handle for the instance
@@ -199,9 +207,18 @@ subsys_initcall(iommu_subsys_init);int iommu_device_register (struct iommu_device *iommu,const struct iommu_ops *ops,struct device *hwdev) { +int err =0 ; +/* We need to be able to take module references appropriately */ if (WARN_ON (is_module_address ((unsigned long )ops) && !ops->owner))return -EINVAL ; +/* +* Temporarily enforce global restriction to a single driver. This was +* already the de-facto behaviour, since any possible combination of +* existing drivers would compete for at least the PCI or platform bus. +*/ +if (iommu_buses[0 ]->iommu_ops && iommu_buses[0 ]->iommu_ops != ops) +return -EBUSY ; iommu->ops = ops;if (hwdev)
@@ -210,12 +227,22 @@ int iommu_device_register(struct iommu_device *iommu,spin_lock (&iommu_device_lock);list_add_tail (&iommu->list, &iommu_device_list);spin_unlock (&iommu_device_lock); -return 0 ; + +for (int i =0 ; i <ARRAY_SIZE (iommu_buses) && !err; i++) { + iommu_buses[i]->iommu_ops = ops; + err =bus_iommu_probe (iommu_buses[i]); + } +if (err) +iommu_device_unregister (iommu); +return err; }EXPORT_SYMBOL_GPL (iommu_device_register);void iommu_device_unregister (struct iommu_device *iommu) { +for (int i =0 ; i <ARRAY_SIZE (iommu_buses); i++) +bus_for_each_dev (iommu_buses[i],NULL , iommu, remove_iommu_group); +spin_lock (&iommu_device_lock);list_del (&iommu->list);spin_unlock (&iommu_device_lock);
@@ -1644,13 +1671,6 @@ static int probe_iommu_group(struct device *dev, void *data)return ret; } -static int remove_iommu_group (struct device *dev,void *data) -{ -iommu_release_device (dev); - -return 0 ; -} -static int iommu_bus_notifier (struct notifier_block *nb,unsigned long action,void *data) {
@@ -1822,27 +1842,12 @@ int bus_iommu_probe(struct bus_type *bus)*/ int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops) { -int err; - -if (ops ==NULL ) { - bus->iommu_ops =NULL ; -return 0 ; - } - -if (bus->iommu_ops !=NULL ) +if (bus->iommu_ops && ops && bus->iommu_ops != ops)return -EBUSY ; bus->iommu_ops = ops; -/* Do IOMMU specific setup for this bus-type */ - err =bus_iommu_probe (bus); -if (err) { -/* Clean up */ -bus_for_each_dev (bus,NULL ,NULL , remove_iommu_group); - bus->iommu_ops =NULL ; - } - -return err; +return 0 ; }EXPORT_SYMBOL_GPL (bus_set_iommu);
Signature · 1 line
2.36.1.dirty
Jon Hunter
maintainerRe: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
Hi Robin,
Quoted 9 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Cheers
Jon
Signature · 1 line
nvpublic
Robin Murphy
reviewerRe: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
Quoted 14 lines · Jon Hunter
Hi Robin,
Quoted 9 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Quoted 6 lines
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Oh, apparently what's happened is that I've inadvertently enabled the tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu() before. Looking at the history, it appears to have been that way since c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so essentially that driver has been broken and useless for close to 8 years now :(
Given that, I'd be inclined to "fix" it as below, or just give up and delete the whole thing.
Thanks,
Robin.
----->8-----
drivers/iommu/Kconfig +1 -0
@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART bool "Tegra GART IOMMU Support" depends on ARCH_TEGRA_2x_SOC depends on TEGRA_MC + depends on BROKEN select IOMMU_API help Enables support for remapping discontiguous physical memory
Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
Quoted 24 lines · Robin Murphy
Quoted 14 lines · Jon Hunter
Hi Robin,
Quoted 9 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Quoted 8 lines
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Quoted 8 lines
Oh, apparently what's happened is that I've inadvertently enabled the tegra-gart driver, since it seems that *wasn't* calling
bus_set_iommu()before. Looking at the history, it appears to have been that way sincec7e3ca515e78("iommu/tegra: gart: Do not register with bus"), so essentially that driver has been broken and useless for close to 8 years now :(Given that, I'd be inclined to "fix" it as below, or just give up and delete the whole thing.
I'm inclined to agree. GART is severely limited: it provides a single IOMMU domain with an aperture of 32 MiB. It's close to useless for anything we would want to do and my understanding is that people have been falling back to CMA for any graphics/display stuff that the GART would've been useful for.
Given that nobody's felt the urge to fix this for the past 8 years, I don't think there's enough interest in this to keep it going.
Dmitry, any thoughts?
Thierry
Quoted 17 lines
Thanks,
Robin.----->8-----
drivers/iommu/Kconfig +1
-0@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART bool "Tegra GART IOMMU Support" depends on ARCH_TEGRA_2x_SOC depends on TEGRA_MC + depends on BROKEN select IOMMU_API help Enables support for remapping discontiguous physical memory
Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
Quoted 16 lines · Thierry Reding
Quoted 15 lines · Robin Murphy
Quoted 14 lines · Jon Hunter
Hi Robin,
Quoted 9 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
The upstream Tegra20 device-tree doesn't have IOMMU phandle for 54140000.gr2d. In this case IOMMU domain shouldn't be available for the DRM driver [1]. Sounds like IOMMU core has a bug.
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/iommu/tegra-gart.c#L243
Quoted 18 lines
Quoted 11 lines
Quoted 2 lines
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Oh, apparently what's happened is that I've inadvertently enabled the tegra-gart driver, since it seems that *wasn't* calling
bus_set_iommu()before. Looking at the history, it appears to have been that way sincec7e3ca515e78("iommu/tegra: gart: Do not register with bus"), so essentially that driver has been broken and useless for close to 8 years now :(Given that, I'd be inclined to "fix" it as below, or just give up and delete the whole thing.
I'm inclined to agree. GART is severely limited: it provides a single IOMMU domain with an aperture of 32 MiB. It's close to useless for anything we would want to do and my understanding is that people have been falling back to CMA for any graphics/display stuff that the GART would've been useful for.
Quoted 4 lines
Given that nobody's felt the urge to fix this for the past 8 years, I don't think there's enough interest in this to keep it going.
Dmitry, any thoughts?
This GART driver is used by a community kernel fork that has alternative DRM driver supporting IOMMU/GART on Tegra20. The fork is periodically synced with the latest upstream, it's used by postmarketOS. Hence it wasn't a completely dead driver.
The 32M aperture works well for 2d/3d engines because it fits multiple textures at once. Tegra DRM driver needs to remap buffers dynamically, but this is easy to implement because DRM core has nice helpers for that. We haven't got to the point where upstream DRM driver is ready to support this feature.
CMA is hard to use for anything other than display framebuffers. It's slow and fails to allocate memory if CMA area is "shared" due to fragmentation and pinned pages. Reserved CMA isn't an option for GPU because then there is no memory for the rest of system.
I don't see any problems with removing GART driver. It's not going to be used soon in upstream and only adds maintenance burden. We can always re-add it in the future.
Signature · 2 lines
Best regards, Dmitry
Jon Hunter
maintainerRe: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
In reply to Robin MurphyQuoted 35 lines · Robin Murphy
Quoted 14 lines · Jon Hunter
Hi Robin,
Quoted 9 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Quoted 6 lines
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Oh, apparently what's happened is that I've inadvertently enabled the tegra-gart driver, since it seems that *wasn't* calling
bus_set_iommu()before. Looking at the history, it appears to have been that way sincec7e3ca515e78("iommu/tegra: gart: Do not register with bus"), so essentially that driver has been broken and useless for close to 8 years now :(Given that, I'd be inclined to "fix" it as below, or just give up and delete the whole thing.
Thanks,
Robin.----->8-----
drivers/iommu/Kconfig +0
-0
drivers/iommu/Kconfig config TEGRA_IOMMU_GART · line 236
Quoted 7 lines
drivers/iommu/Kconfig +1
-0@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART bool "Tegra GART IOMMU Support" depends on ARCH_TEGRA_2x_SOC depends on TEGRA_MC + depends on BROKEN select IOMMU_API help Enables support for remapping discontiguous physical memory
Thanks Robin. This works for me.
Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?
Jon
Signature · 1 line
nvpublic
Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
18.10.2022 09:13, Jon Hunter пишет:
Quoted 58 lines · Robin Murphy
Quoted 45 lines · Jon Hunter
Quoted 16 lines
Hi Robin,
Quoted 11 lines · Robin Murphy
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances,
and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy
to clean up the exact footprint of a given instance, no more, no less.Quoted 8 lines
Since this change, I have noticed that the DRM driver on Tegra20 is failing to probe and I am seeing ...
tegra-gr2d 54140000.gr2d: failed to attach to domain: -19 drm drm: failed to initialize 54140000.gr2d: -19
Bisect points to this change and reverting it fixes it. Let me know if you have any thoughts.
Oh, apparently what's happened is that I've inadvertently enabled the tegra-gart driver, since it seems that *wasn't* calling
bus_set_iommu()before. Looking at the history, it appears to have been that way sincec7e3ca515e78("iommu/tegra: gart: Do not register with bus"), so essentially that driver has been broken and useless for close to 8 years now :(Given that, I'd be inclined to "fix" it as below, or just give up and delete the whole thing.
Thanks,
Robin.----->8-----
drivers/iommu/Kconfig +0
-0@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GARTQuoted 7 lines
bool "Tegra GART IOMMU Support"
depends onARCH_TEGRA_2x_SOC
depends onTEGRA_MC
+ depends on BROKEN
selectIOMMU_API
help
Enables support for remapping discontiguous physical memoryThanks Robin. This works for me.
Quoted 1 line
Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?
To me it is more a problem of the DRM driver that it doesn't support GART. GART will require a special handling from the DRM driver anyways [1].
[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/grate/drm.c#L460
The GART driver itself isn't broken, it's working perfectly fine. It's the DRM driver that should start caring about the GART presence, IMO.
Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
In reply to Robin MurphyQuoted 16 lines · On Mon, 15 Aug 2022 17:20:06 +0100 Robin Murphy <[email protected]> wrote:
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Tested-by Marek Szyprowski <[email protected]>Reviewed-by Krishna Reddy <[email protected]>Reviewed-by Kevin Tian <[email protected]>Signed-off-by Robin Murphy <[email protected]>
Quoted 7 lines
---
v4: Factor out the ops check in
iommu_device_register()to keep the loop even simpler, and comment the nominal change in behaviourdrivers/iommu/iommu.c | 55 +++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 25 deletions(-)
This introduces the below lockdep spat regression, bisected to commit:
57365a04c921 ("iommu: Move bus setup to IOMMU device registration")
This can be reproduced with simple vfio-pci device assignment to a VM on x86_64 with VT-d. Thanks,
Alex
======================================================
WARNING: possible circular locking dependency detected
6.0.0-rc4+ #127 Tainted: G E
------------------------------------------------------
qemu-system-x86/1726 is trying to acquire lock:ffffffffacf8a7d0 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_get_resv_regions+0x21/0x2a0
but task is already holding lock:ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (&group->mutex){+.+.}-{3:3}:
__mutex_lock+0x6d/0x8c0
iommu_group_add_device+0xfb/0x330
__iommu_probe_device+0x150/0x270
probe_iommu_group+0x31/0x50
bus_for_each_dev+0x67/0xa0
bus_iommu_probe+0x38/0x2a0
iommu_device_register+0xc1/0x130
intel_iommu_init+0xfd9/0x120d
pci_iommu_init+0xe/0x36
do_one_initcall+0x5b/0x310
kernel_init_freeable+0x275/0x2c1
kernel_init+0x16/0x140
ret_from_fork+0x22/0x30-> #0 (dmar_global_lock){++++}-{3:3}:
__lock_acquire+0x10dc/0x1da0
lock_acquire+0xc2/0x2d0
down_read+0x2d/0x40
intel_iommu_get_resv_regions+0x21/0x2a0
iommu_get_group_resv_regions+0x88/0x3b0
vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1]
vfio_fops_unl_ioctl+0x19d/0x270 [vfio]
__x64_sys_ioctl+0x8b/0xc0
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcdother info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&group->mutex);
lock(dmar_global_lock);
lock(&group->mutex); lock(dmar_global_lock);
*** DEADLOCK ***
4 locks held by qemu-system-x86/1726:
#0: ffff9811b5546c88 (&container->group_lock){++++}-{3:3}, at: vfio_fops_unl_ioctl+0xbb/0x270 [vfio] #1: ffffffffc058c720 (&vfio.iommu_drivers_lock){+.+.}-{3:3}, at: vfio_fops_unl_ioctl+0xeb/0x270 [vfio] #2: ffff9811d865ba88 (&iommu->lock#2){+.+.}-{3:3}, at: vfio_iommu_type1_attach_group+0x51/0xce1 [vfio_iommu_type1] #3: ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0
stack backtrace: CPU: 0 PID: 1726 Comm: qemu-system-x86 Tainted: G E 6.0.0-rc4+ #127 Hardware name: System manufacturer System Product Name/P8H67-M PRO, BIOS 3904 04/27/2013 Call Trace: <TASK> dump_stack_lvl+0x56/0x73 check_noncircular+0xd6/0x100 ? __lock_acquire+0x374/0x1da0 __lock_acquire+0x10dc/0x1da0 lock_acquire+0xc2/0x2d0 ? intel_iommu_get_resv_regions+0x21/0x2a0 ? trace_contention_end+0x2d/0xd0 ? __mutex_lock+0xdf/0x8c0 ? iommu_get_group_resv_regions+0x2c/0x3b0 ? lock_is_held_type+0xe2/0x140 down_read+0x2d/0x40 ? intel_iommu_get_resv_regions+0x21/0x2a0 intel_iommu_get_resv_regions+0x21/0x2a0 iommu_get_group_resv_regions+0x88/0x3b0 ? iommu_attach_group+0x76/0xa0 vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1] ? rcu_read_lock_sched_held+0x43/0x70 ? __module_address.part.0+0x2b/0xa0 ? is_module_address+0x43/0x70 ? __init_waitqueue_head+0x4a/0x60 vfio_fops_unl_ioctl+0x19d/0x270 [vfio] __x64_sys_ioctl+0x8b/0xc0 do_syscall_64+0x3b/0x90 entry_SYSCALL_64_after_hwframe+0x63/0xcd RIP: 0033:0x7f803853a17b Code: 0f 1e fa 48 8b 05 1d ad 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ed ac 0c 00 f7 d8 64 89 01 48 RSP: 002b:00007ffd8128c2e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 0000000000000020 RCX: 00007f803853a17b RDX: 0000000000000003 RSI: 0000000000003b66 RDI: 000000000000001c RBP: 00007ffd8128c320 R08: 000055f59d8ff8d0 R09: 00007f8038605a40 R10: 0000000000000008 R11: 0000000000000246 R12: 000055f599aed1d0 R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 </TASK>
Baolu Lu
maintainerRe: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration
Hi Alex,
Quoted 24 lines · Alex Williamson
Quoted 21 lines · On Mon, 15 Aug 2022 17:20:06 +0100 Robin Murphy<[email protected]> wrote:
Move the bus setup to
iommu_device_register(). This should allowbus_iommu_probe()to be correctly replayed for multiple IOMMU instances, and leavesbus_set_iommu()as a glorified no-op to be cleaned up next.At this point we can also handle cleanup better than just rolling back the most-recently-touched bus upon failure - which may release devices owned by other already-registered instances, and still leave devices on other buses with dangling pointers to the failed instance. Now it's easy to clean up the exact footprint of a given instance, no more, no less.
Tested-by Marek Szyprowski<[email protected]>Reviewed-by Krishna Reddy<[email protected]>Reviewed-by Kevin Tian<[email protected]>Tested-by Matthew Rosato<[email protected]> # s390Signed-off-by Robin Murphy<[email protected]>---
v4: Factor out the ops check in
iommu_device_register()to keep the loop even simpler, and comment the nominal change in behaviour
Quoted 8 lines
Quoted 2 lines
drivers/iommu/iommu.c | 55 +++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 25 deletions(-)This introduces the below lockdep spat regression, bisected to commit:
57365a04c921("iommu: Move bus setup to IOMMU device registration")This can be reproduced with simple vfio-pci device assignment to a VM on
x86_64with VT-d. Thanks,
Thank you for reporting this. I have proposed below fix:
https://lore.kernel.org/all/[email protected]/
Does it work for you?
Best regards,
baolu
Robin Murphy
reviewerPATCH v4 6/16 iommu/amd: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and garbage-collect the last remnants of amd_iommu_init_api().
---
v4: No change
drivers/iommu/amd/amd_iommu.h | 1 - drivers/iommu/amd/init.c | 9 +-------- drivers/iommu/amd/iommu.c | 21 --------------------- 3 files changed, 1 insertion(+), 30 deletions(-)
drivers/iommu/amd/amd_iommu.h +0 -1
@@ -18,7 +18,6 @@ extern void amd_iommu_restart_event_logging(struct amd_iommu *iommu);extern int amd_iommu_init_devices (void );extern void amd_iommu_uninit_devices (void );extern void amd_iommu_init_notifier (void ); -extern int amd_iommu_init_api (void );extern void amd_iommu_set_rlookup_table (struct amd_iommu *iommu,u16 devid);#ifdef CONFIG_AMD_IOMMU_DEBUGFS
drivers/iommu/amd/init.c +1 -8
@@ -2168,20 +2168,13 @@ static int __init amd_iommu_init_pci(void)/* * Order is important here to make sure any unity map requirements are * fulfilled. The unity mappings are created and written to the device -* table during the amd_iommu_init_api() call. +* table during the iommu_init_pci() call. * * After that we call init_device_table_dma() to make sure any * uninitialized DTE will block DMA, and in the end we flush the caches * of all IOMMUs to make sure the changes to the device table are * active. */ - ret =amd_iommu_init_api (); -if (ret) { -pr_err ("IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n" , - ret); - goto out; - } -for_each_pci_segment (pci_seg)init_device_table_dma (pci_seg);
drivers/iommu/amd/iommu.c +0 -21
@@ -11,8 +11,6 @@#include <linux/ratelimit.h> #include <linux/pci.h> #include <linux/acpi.h> -#include <linux/amba/bus.h> -#include <linux/platform_device.h> #include <linux/pci-ats.h> #include <linux/bitmap.h> #include <linux/slab.h>
@@ -1941,25 +1939,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)amd_iommu_domain_flush_complete (domain); } -int __init amd_iommu_init_api (void ) -{ -int err; - - err =bus_set_iommu (&pci_bus_type, &amd_iommu_ops); -if (err) -return err; -#ifdef CONFIG_ARM_AMBA - err =bus_set_iommu (&amba_bustype, &amd_iommu_ops); -if (err) -return err; -#endif - err =bus_set_iommu (&platform_bus_type, &amd_iommu_ops); -if (err) -return err; - -return 0 ; -} -/***************************************************************************** * * The following functions belong to the exported interface of AMD IOMMU
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 7/16 iommu/arm-smmu: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary. With device probes now replayed for every IOMMU instance registration, the whole sorry ordering workaround for legacy DT bindings goes too, hooray!
---
v4: No change
drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +-------------------------- 1 file changed, 2 insertions(+), 82 deletions(-)
drivers/iommu/arm/arm-smmu/arm-smmu.c +2 -82
@@ -37,7 +37,6 @@#include <linux/ratelimit.h> #include <linux/slab.h> -#include <linux/amba/bus.h> #include <linux/fsl/mc.h> #include "arm-smmu.h"
@@ -93,8 +92,6 @@ static struct platform_driver arm_smmu_driver;static struct iommu_ops arm_smmu_ops;#ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS -static int arm_smmu_bus_init (struct iommu_ops *ops); -static struct device_node *dev_get_dev_node (struct device *dev) {if (dev_is_pci (dev)) {
@@ -180,20 +177,6 @@ static int arm_smmu_register_legacy_master(struct device *dev,kfree (sids); return err; } - -/* -* With the legacy DT binding in play, we have no guarantees about -* probe order, but then we're also not doing default domains, so we can -* delay setting bus ops until we're sure every possible SMMU is ready, -* and that way ensure that no probe_device() calls get missed. -*/ -static int arm_smmu_legacy_bus_init (void ) -{ -if (using_legacy_binding) -return arm_smmu_bus_init (&arm_smmu_ops); -return 0 ; -} -device_initcall_sync (arm_smmu_legacy_bus_init);#else static int arm_smmu_register_legacy_master (struct device *dev,struct arm_smmu_device **smmu)
@@ -2016,52 +1999,6 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,return 0 ; } -static int arm_smmu_bus_init (struct iommu_ops *ops) -{ -int err; - -/* Oh, for a proper bus abstraction */ -if (!iommu_present (&platform_bus_type)) { - err =bus_set_iommu (&platform_bus_type, ops); -if (err) -return err; - } -#ifdef CONFIG_ARM_AMBA -if (!iommu_present (&amba_bustype)) { - err =bus_set_iommu (&amba_bustype, ops); -if (err) - goto err_reset_platform_ops; - } -#endif -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - err =bus_set_iommu (&pci_bus_type, ops); -if (err) - goto err_reset_amba_ops; - } -#endif -#ifdef CONFIG_FSL_MC_BUS -if (!iommu_present (&fsl_mc_bus_type)) { - err =bus_set_iommu (&fsl_mc_bus_type, ops); -if (err) - goto err_reset_pci_ops; - } -#endif -return 0 ; - -err_reset_pci_ops:__maybe_unused ; -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif -err_reset_amba_ops:__maybe_unused ; -#ifdef CONFIG_ARM_AMBA -bus_set_iommu (&amba_bustype,NULL ); -#endif -err_reset_platform_ops:__maybe_unused ; -bus_set_iommu (&platform_bus_type,NULL ); -return err; -} -static void arm_smmu_rmr_install_bypass_smr (struct arm_smmu_device *smmu) {struct list_head rmr_list;
@@ -2226,7 +2163,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev) err =iommu_device_register (&smmu->iommu, &arm_smmu_ops, dev);if (err) {dev_err (dev,"Failed to register iommu\n" ); - goto err_sysfs_remove; +iommu_device_sysfs_remove (&smmu->iommu); +return err; }platform_set_drvdata (pdev, smmu);
@@ -2248,24 +2186,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)pm_runtime_enable (dev); } -/* -* For ACPI and generic DT bindings, an SMMU will be probed before -* any device which might need it, so we want the bus ops in place -* ready to handle default domain setup as soon as any SMMU exists. -*/ -if (!using_legacy_binding) { - err =arm_smmu_bus_init (&arm_smmu_ops); -if (err) - goto err_unregister_device; - } -return 0 ; - -err_unregister_device: -iommu_device_unregister (&smmu->iommu); -err_sysfs_remove: -iommu_device_sysfs_remove (&smmu->iommu); -return err; }static int arm_smmu_device_remove (struct platform_device *pdev)
@@ -2278,7 +2199,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)if (!bitmap_empty (smmu->context_map,ARM_SMMU_MAX_CBS ))dev_notice (&pdev->dev,"disabling translation\n" ); -arm_smmu_bus_init (NULL );iommu_device_unregister (&smmu->iommu);iommu_device_sysfs_remove (&smmu->iommu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 8/16 iommu/arm-smmu-v3: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v4: No change
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +-------------------- 1 file changed, 2 insertions(+), 51 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -51
@@ -28,8 +28,6 @@#include <linux/pci-ats.h> #include <linux/platform_device.h> -#include <linux/amba/bus.h> -#include "arm-smmu-v3.h" #include "../../iommu-sva-lib.h"
@@ -3673,43 +3671,6 @@ static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)return SZ_128K ; } -static int arm_smmu_set_bus_ops (struct iommu_ops *ops) -{ -int err; - -#ifdef CONFIG_PCI -if (pci_bus_type.iommu_ops != ops) { - err =bus_set_iommu (&pci_bus_type, ops); -if (err) -return err; - } -#endif -#ifdef CONFIG_ARM_AMBA -if (amba_bustype.iommu_ops != ops) { - err =bus_set_iommu (&amba_bustype, ops); -if (err) - goto err_reset_pci_ops; - } -#endif -if (platform_bus_type.iommu_ops != ops) { - err =bus_set_iommu (&platform_bus_type, ops); -if (err) - goto err_reset_amba_ops; - } - -return 0 ; - -err_reset_amba_ops: -#ifdef CONFIG_ARM_AMBA -bus_set_iommu (&amba_bustype,NULL ); -#endif -err_reset_pci_ops:__maybe_unused ; -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif -return err; -} -static void __iomem *arm_smmu_ioremap (struct device *dev,resource_size_t start,resource_size_t size) {
@@ -3848,27 +3809,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev) ret =iommu_device_register (&smmu->iommu, &arm_smmu_ops, dev);if (ret) {dev_err (dev,"Failed to register iommu\n" ); - goto err_sysfs_remove; +iommu_device_sysfs_remove (&smmu->iommu); +return ret; } - ret =arm_smmu_set_bus_ops (&arm_smmu_ops); -if (ret) - goto err_unregister_device; -return 0 ; - -err_unregister_device: -iommu_device_unregister (&smmu->iommu); -err_sysfs_remove: -iommu_device_sysfs_remove (&smmu->iommu); -return ret; }static int arm_smmu_device_remove (struct platform_device *pdev) {struct arm_smmu_device *smmu =platform_get_drvdata (pdev); -arm_smmu_set_bus_ops (NULL );iommu_device_unregister (&smmu->iommu);iommu_device_sysfs_remove (&smmu->iommu);arm_smmu_device_disable (smmu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 9/16 iommu/dart: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v4: No change
drivers/iommu/apple-dart.c | 30 +----------------------------- 1 file changed, 1 insertion(+), 29 deletions(-)
drivers/iommu/apple-dart.c +1 -29
@@ -820,27 +820,6 @@ static irqreturn_t apple_dart_irq(int irq, void *dev)return IRQ_HANDLED ; } -static int apple_dart_set_bus_ops (const struct iommu_ops *ops) -{ -int ret; - -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, ops); -if (ret) -return ret; - } -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - ret =bus_set_iommu (&pci_bus_type, ops); -if (ret) { -bus_set_iommu (&platform_bus_type,NULL ); -return ret; - } - } -#endif -return 0 ; -} -static int apple_dart_probe (struct platform_device *pdev) {int ret;
@@ -895,14 +874,10 @@ static int apple_dart_probe(struct platform_device *pdev)platform_set_drvdata (pdev, dart); - ret =apple_dart_set_bus_ops (&apple_dart_iommu_ops); -if (ret) - goto err_free_irq; - ret =iommu_device_sysfs_add (&dart->iommu, dev,NULL ,"apple-dart.%s" ,dev_name (&pdev->dev));if (ret) - goto err_remove_bus_ops; + goto err_free_irq; ret =iommu_device_register (&dart->iommu, &apple_dart_iommu_ops, dev);if (ret)
@@ -916,8 +891,6 @@ static int apple_dart_probe(struct platform_device *pdev) err_sysfs_remove:iommu_device_sysfs_remove (&dart->iommu); -err_remove_bus_ops: -apple_dart_set_bus_ops (NULL ); err_free_irq:free_irq (dart->irq, dart); err_clk_disable:
@@ -932,7 +905,6 @@ static int apple_dart_remove(struct platform_device *pdev)apple_dart_hw_reset (dart);free_irq (dart->irq, dart); -apple_dart_set_bus_ops (NULL );iommu_device_unregister (&dart->iommu);iommu_device_sysfs_remove (&dart->iommu);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 10/16 iommu/exynos: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the init failure path accordingly.
---
v4: No change
drivers/iommu/exynos-iommu.c | 9 --------- 1 file changed, 9 deletions(-)
drivers/iommu/exynos-iommu.c +0 -9
@@ -1446,16 +1446,7 @@ static int __init exynos_iommu_init(void) goto err_zero_lv2; } - ret =bus_set_iommu (&platform_bus_type, &exynos_iommu_ops); -if (ret) { -pr_err ("%s: Failed to register exynos-iommu driver.\n" , - __func__); - goto err_set_iommu; - } -return 0 ; -err_set_iommu: -kmem_cache_free (lv2table_kmem_cache, zero_lv2_table); err_zero_lv2:platform_driver_unregister (&exynos_sysmmu_driver); err_reg_driver:
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 11/16 iommu/ipmmu-vmsa: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary. This also leaves the custom initcall effectively doing nothing but register the driver, which no longer needs to happen early either, so convert it to builtin_platform_driver().
---
v4: No change
drivers/iommu/ipmmu-vmsa.c | 35 +---------------------------------- 1 file changed, 1 insertion(+), 34 deletions(-)
drivers/iommu/ipmmu-vmsa.c +1 -34
@@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev) ret =iommu_device_register (&mmu->iommu, &ipmmu_ops, &pdev->dev);if (ret)return ret; - -#if defined(CONFIG_IOMMU_DMA ) -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &ipmmu_ops); -#endif }/*
@@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = { .probe = ipmmu_probe, .remove = ipmmu_remove, }; - -static int __init ipmmu_init (void ) -{ -struct device_node *np; -static bool setup_done; -int ret; - -if (setup_done) -return 0 ; - - np =of_find_matching_node (NULL , ipmmu_of_ids); -if (!np) -return 0 ; - -of_node_put (np); - - ret =platform_driver_register (&ipmmu_driver); -if (ret <0 ) -return ret; - -#if defined(CONFIG_ARM ) && !defined(CONFIG_IOMMU_DMA ) -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &ipmmu_ops); -#endif - - setup_done = true; -return 0 ; -} -subsys_initcall (ipmmu_init); +builtin_platform_driver (ipmmu_driver);
Signature · 1 line
2.36.1.dirty
kernel test robot
build botRe: [PATCH v4 11/16] iommu/ipmmu-vmsa: Clean up bus_set_iommu()
I love your patch! Perhaps something to improve:
Hi Robin,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v6.0-rc1]
[also build test WARNING on linus/master next-20220815]
[cannot apply to joro-iommu/next krzk/for-next tegra/for-next rockchip/for-next sunxi/sunxi/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommu-retire-bus_set_iommu/20220816-002348 base: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868 config: x86_64-buildonly-randconfig-r003-20220815 (https://download.01.org/0day-ci/archive/20220816/[email protected]/config) compiler: gcc-11 (Debian 11.3.0-5) 11.3.0 reproduce (this is a W=1 build): # https://github.com/intel-lab-lkp/linux/commit/72fa5405b9cf32a9814ca7ed6d5b877cabf83efd git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Robin-Murphy/iommu-retire-bus_set_iommu/20220816-002348 git checkout 72fa5405b9cf32a9814ca7ed6d5b877cabf83efd # save the config file mkdir build_dir && cp config build_dir/.config make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iommu/
If you fix the issue, kindly add following tag where applicable
All warnings (new ones prefixed by >>):
>> drivers/iommu/ipmmu-vmsa.c:946:34: warning: 'ipmmu_of_ids' defined but not used [-Wunused-const-variable=]
946 | static const struct of_device_id ipmmu_of_ids[] = {
| ^~~~~~~~~~~~vim +/ipmmu_of_ids +946 drivers/iommu/ipmmu-vmsa.c
7a62ced8ebd0e1 Yoshihiro Shimoda 2021-09-07 945
33f3ac9b511612 Magnus Damm 2017-10-16 @946 static const struct of_device_id ipmmu_of_ids[] = {
33f3ac9b511612 Magnus Damm 2017-10-16 947 {
33f3ac9b511612 Magnus Damm 2017-10-16 948 .compatible = "renesas,ipmmu-vmsa",
33f3ac9b511612 Magnus Damm 2017-10-16 949 .data = &ipmmu_features_default,
60fb0083c9d43b Fabrizio Castro 2018-08-23 950 }, {
60fb0083c9d43b Fabrizio Castro 2018-08-23 951 .compatible = "renesas,ipmmu-r8a774a1",
60fb0083c9d43b Fabrizio Castro 2018-08-23 952 .data = &ipmmu_features_rcar_gen3,
757f26a3a9ec2c Biju Das 2019-09-27 953 }, {
757f26a3a9ec2c Biju Das 2019-09-27 954 .compatible = "renesas,ipmmu-r8a774b1",
757f26a3a9ec2c Biju Das 2019-09-27 955 .data = &ipmmu_features_rcar_gen3,
b6d39cd82241bf Fabrizio Castro 2018-12-13 956 }, {
b6d39cd82241bf Fabrizio Castro 2018-12-13 957 .compatible = "renesas,ipmmu-r8a774c0",
b6d39cd82241bf Fabrizio Castro 2018-12-13 958 .data = &ipmmu_features_rcar_gen3,
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14 959 }, {
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14 960 .compatible = "renesas,ipmmu-r8a774e1",
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14 961 .data = &ipmmu_features_rcar_gen3,
58b8e8bf409236 Magnus Damm 2017-10-16 962 }, {
58b8e8bf409236 Magnus Damm 2017-10-16 963 .compatible = "renesas,ipmmu-r8a7795",
0b8ac1409641e1 Magnus Damm 2018-06-14 964 .data = &ipmmu_features_rcar_gen3,
0b8ac1409641e1 Magnus Damm 2018-06-14 965 }, {
0b8ac1409641e1 Magnus Damm 2018-06-14 966 .compatible = "renesas,ipmmu-r8a7796",
0b8ac1409641e1 Magnus Damm 2018-06-14 967 .data = &ipmmu_features_rcar_gen3,
17fe1618163980 Yoshihiro Shimoda 2020-06-11 968 }, {
17fe1618163980 Yoshihiro Shimoda 2020-06-11 969 .compatible = "renesas,ipmmu-r8a77961",
17fe1618163980 Yoshihiro Shimoda 2020-06-11 970 .data = &ipmmu_features_rcar_gen3,
98dbffd39a6513 Jacopo Mondi 2018-06-14 971 }, {
98dbffd39a6513 Jacopo Mondi 2018-06-14 972 .compatible = "renesas,ipmmu-r8a77965",
98dbffd39a6513 Jacopo Mondi 2018-06-14 973 .data = &ipmmu_features_rcar_gen3,
3701c123e1c13c Simon Horman 2018-06-14 974 }, {
3701c123e1c13c Simon Horman 2018-06-14 975 .compatible = "renesas,ipmmu-r8a77970",
3701c123e1c13c Simon Horman 2018-06-14 976 .data = &ipmmu_features_rcar_gen3,
1cdeb52e5c245b Nikita Yushchenko 2021-09-23 977 }, {
1cdeb52e5c245b Nikita Yushchenko 2021-09-23 978 .compatible = "renesas,ipmmu-r8a77980",
1cdeb52e5c245b Nikita Yushchenko 2021-09-23 979 .data = &ipmmu_features_rcar_gen3,
b0c32912150565 Hai Nguyen Pham 2018-10-17 980 }, {
b0c32912150565 Hai Nguyen Pham 2018-10-17 981 .compatible = "renesas,ipmmu-r8a77990",
b0c32912150565 Hai Nguyen Pham 2018-10-17 982 .data = &ipmmu_features_rcar_gen3,
3701c123e1c13c Simon Horman 2018-06-14 983 }, {
3701c123e1c13c Simon Horman 2018-06-14 984 .compatible = "renesas,ipmmu-r8a77995",
3701c123e1c13c Simon Horman 2018-06-14 985 .data = &ipmmu_features_rcar_gen3,
7a62ced8ebd0e1 Yoshihiro Shimoda 2021-09-07 986 }, {
7a62ced8ebd0e1 Yoshihiro Shimoda 2021-09-07 987 .compatible = "renesas,ipmmu-r8a779a0",
ae684caf465b7d Yoshihiro Shimoda 2022-02-08 988 .data = &ipmmu_features_rcar_gen4,
ae684caf465b7d Yoshihiro Shimoda 2022-02-08 989 }, {
9f7d09fe23a011 Yoshihiro Shimoda 2022-06-17 990 .compatible = "renesas,rcar-gen4-ipmmu-vmsa",
ae684caf465b7d Yoshihiro Shimoda 2022-02-08 991 .data = &ipmmu_features_rcar_gen4,
33f3ac9b511612 Magnus Damm 2017-10-16 992 }, {
33f3ac9b511612 Magnus Damm 2017-10-16 993 /* Terminator */
33f3ac9b511612 Magnus Damm 2017-10-16 994 },
33f3ac9b511612 Magnus Damm 2017-10-16 995 };
33f3ac9b511612 Magnus Damm 2017-10-16 996 Signature · 2 lines
0-DAY CI Kernel Test Service https://01.org/lkp
Robin Murphy
reviewerPATCH v4 12/16 iommu/mtk: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure paths accordingly.
---
v4: No change
drivers/iommu/mtk_iommu.c | 24 +----------------------- drivers/iommu/mtk_iommu_v1.c | 13 +------------ 2 files changed, 2 insertions(+), 35 deletions(-)
drivers/iommu/mtk_iommu.c +1 -23
@@ -1243,30 +1243,13 @@ static int mtk_iommu_probe(struct platform_device *pdev) data->hw_list = &data->hw_list_head; } -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, &mtk_iommu_ops); -if (ret) - goto out_list_del; - } -if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_MM )) { ret =component_master_add_with_match (dev, &mtk_iommu_com_ops, match);if (ret) - goto out_bus_set_null; - }else if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_INFRA ) && -MTK_IOMMU_HAS_FLAG (data->plat_data,IFA_IOMMU_PCIE_SUPPORT )) { -#ifdef CONFIG_PCI -if (!iommu_present (&pci_bus_type)) { - ret =bus_set_iommu (&pci_bus_type, &mtk_iommu_ops); -if (ret)/* PCIe fail don't affect platform_bus. */ - goto out_list_del; - } -#endif + goto out_list_del; }return ret; -out_bus_set_null: -bus_set_iommu (&platform_bus_type,NULL ); out_list_del:list_del (&data->list);iommu_device_unregister (&data->iommu);
@@ -1294,11 +1277,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_MM )) {device_link_remove (data->smicomm_dev, &pdev->dev);component_master_del (&pdev->dev, &mtk_iommu_com_ops); - }else if (MTK_IOMMU_IS_TYPE (data->plat_data,MTK_IOMMU_TYPE_INFRA ) && -MTK_IOMMU_HAS_FLAG (data->plat_data,IFA_IOMMU_PCIE_SUPPORT )) { -#ifdef CONFIG_PCI -bus_set_iommu (&pci_bus_type,NULL ); -#endif }pm_runtime_disable (&pdev->dev);for (i =0 ; i < data->plat_data->banks_num; i++) {
drivers/iommu/mtk_iommu_v1.c +1 -12
@@ -691,19 +691,11 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)if (ret) goto out_sysfs_remove; -if (!iommu_present (&platform_bus_type)) { - ret =bus_set_iommu (&platform_bus_type, &mtk_iommu_v1_ops); -if (ret) - goto out_dev_unreg; - } - ret =component_master_add_with_match (dev, &mtk_iommu_v1_com_ops, match);if (ret) - goto out_bus_set_null; + goto out_dev_unreg;return ret; -out_bus_set_null: -bus_set_iommu (&platform_bus_type,NULL ); out_dev_unreg:iommu_device_unregister (&data->iommu); out_sysfs_remove:
@@ -718,9 +710,6 @@ static int mtk_iommu_v1_remove(struct platform_device *pdev)iommu_device_sysfs_remove (&data->iommu);iommu_device_unregister (&data->iommu); -if (iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type,NULL ); -clk_disable_unprepare (data->bclk);devm_free_irq (&pdev->dev, data->irq, data);component_master_del (&pdev->dev, &mtk_iommu_v1_com_ops);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 13/16 iommu/omap: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the init failure path accordingly.
---
v4: No change
drivers/iommu/omap-iommu.c | 6 ------ 1 file changed, 6 deletions(-)
drivers/iommu/omap-iommu.c +0 -6
@@ -1776,14 +1776,8 @@ static int __init omap_iommu_init(void) goto fail_driver; } - ret =bus_set_iommu (&platform_bus_type, &omap_iommu_ops); -if (ret) - goto fail_bus; -return 0 ; -fail_bus: -platform_driver_unregister (&omap_iommu_driver); fail_driver:kmem_cache_destroy (iopte_cachep);return ret;
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 14/16 iommu/tegra-smmu: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v4: No change
drivers/iommu/tegra-smmu.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-)
drivers/iommu/tegra-smmu.c +6 -23
@@ -1083,8 +1083,8 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,/* * This is a bit of a hack. Ideally we'd want to simply return this -* value. However the IOMMU registration process will attempt to add -* all devices to the IOMMU when bus_set_iommu() is called. In order +* value. However iommu_device_register() will attempt to add +* all devices to the IOMMU before we get that far. In order * not to rely on global variables to track the IOMMU instance, we * set it here so that it can be looked up from the .probe_device() * callback via the IOMMU device's .drvdata field.
@@ -1138,32 +1138,15 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,return ERR_PTR (err); err =iommu_device_register (&smmu->iommu, &tegra_smmu_ops, dev); -if (err) - goto remove_sysfs; - - err =bus_set_iommu (&platform_bus_type, &tegra_smmu_ops); -if (err <0 ) - goto unregister; - -#ifdef CONFIG_PCI - err =bus_set_iommu (&pci_bus_type, &tegra_smmu_ops); -if (err <0 ) - goto unset_platform_bus; -#endif +if (err) { +iommu_device_sysfs_remove (&smmu->iommu); +return ERR_PTR (err); + }if (IS_ENABLED (CONFIG_DEBUG_FS ))tegra_smmu_debugfs_init (smmu);return smmu; - -unset_platform_bus:__maybe_unused ; -bus_set_iommu (&platform_bus_type,NULL ); -unregister: -iommu_device_unregister (&smmu->iommu); -remove_sysfs: -iommu_device_sysfs_remove (&smmu->iommu); - -return ERR_PTR (err); }void tegra_smmu_remove (struct tegra_smmu *smmu)
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 15/16 iommu/virtio: Clean up bus_set_iommu()
In reply to Robin MurphyStop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.
---
v4: No change
drivers/iommu/virtio-iommu.c | 25 ------------------------- 1 file changed, 25 deletions(-)
drivers/iommu/virtio-iommu.c +0 -25
@@ -7,7 +7,6 @@#define pr_fmt (fmt) KBUILD_MODNAME ": " fmt -#include <linux/amba/bus.h> #include <linux/delay.h> #include <linux/dma-iommu.h> #include <linux/dma-map-ops.h>
@@ -17,7 +16,6 @@#include <linux/module.h> #include <linux/of_platform.h> #include <linux/pci.h> -#include <linux/platform_device.h> #include <linux/virtio.h> #include <linux/virtio_config.h> #include <linux/virtio_ids.h>
@@ -1145,26 +1143,6 @@ static int viommu_probe(struct virtio_device *vdev)iommu_device_register (&viommu->iommu, &viommu_ops, parent_dev); -#ifdef CONFIG_PCI -if (pci_bus_type.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&pci_bus_type, &viommu_ops); -if (ret) - goto err_unregister; - } -#endif -#ifdef CONFIG_ARM_AMBA -if (amba_bustype.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&amba_bustype, &viommu_ops); -if (ret) - goto err_unregister; - } -#endif -if (platform_bus_type.iommu_ops != &viommu_ops) { - ret =bus_set_iommu (&platform_bus_type, &viommu_ops); -if (ret) - goto err_unregister; - } - vdev->priv = viommu; dev_info(dev, "input address: %u bits\n",
@@ -1173,9 +1151,6 @@ static int viommu_probe(struct virtio_device *vdev)return 0 ; -err_unregister: -iommu_device_sysfs_remove (&viommu->iommu); -iommu_device_unregister (&viommu->iommu); err_free_vqs: vdev->config->del_vqs (vdev);
Signature · 1 line
2.36.1.dirty
Robin Murphy
reviewerPATCH v4 16/16 iommu: Clean up bus_set_iommu()
In reply to Robin MurphyClean up the remaining trivial bus_set_iommu() callsites along with the implementation. Now drivers only have to know and care about iommu_device instances, phew!
---
v4: No change
drivers/iommu/arm/arm-smmu/qcom_iommu.c | 4 ---- drivers/iommu/fsl_pamu_domain.c | 4 ---- drivers/iommu/intel/iommu.c | 2 -- drivers/iommu/iommu.c | 24 ------------------------ drivers/iommu/msm_iommu.c | 2 -- drivers/iommu/rockchip-iommu.c | 2 -- drivers/iommu/s390-iommu.c | 6 ------ drivers/iommu/sprd-iommu.c | 5 ----- drivers/iommu/sun50i-iommu.c | 2 -- include/linux/iommu.h | 1 - 10 files changed, 52 deletions(-)
drivers/iommu/arm/arm-smmu/qcom_iommu.c +0 -4
@@ -837,8 +837,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev) goto err_pm_disable; } -bus_set_iommu (&platform_bus_type, &qcom_iommu_ops); -if (qcom_iommu->local_base) {pm_runtime_get_sync (dev);writel_relaxed (0xffffffff , qcom_iommu->local_base +SMMU_INTR_SEL_NS );
@@ -856,8 +854,6 @@ static int qcom_iommu_device_remove(struct platform_device *pdev) {struct qcom_iommu_dev *qcom_iommu =platform_get_drvdata (pdev); -bus_set_iommu (&platform_bus_type,NULL ); -pm_runtime_force_suspend (&pdev->dev);platform_set_drvdata (pdev,NULL );iommu_device_sysfs_remove (&qcom_iommu->iommu);
drivers/iommu/fsl_pamu_domain.c +0 -4
@@ -476,11 +476,7 @@ int __init pamu_domain_init(void)if (ret) {iommu_device_sysfs_remove (&pamu_iommu);pr_err ("Can't register iommu device\n" ); -return ret; } -bus_set_iommu (&platform_bus_type, &fsl_pamu_ops); -bus_set_iommu (&pci_bus_type, &fsl_pamu_ops); -return ret; }
drivers/iommu/intel/iommu.c +0 -2
@@ -3890,7 +3890,6 @@ static int __init probe_acpi_namespace_devices(void)continue ; } - pn->dev->bus->iommu_ops = &intel_iommu_ops; ret =iommu_probe_device (pn->dev);if (ret)break ;
@@ -4023,7 +4022,6 @@ int __init intel_iommu_init(void) }up_read (&dmar_global_lock); -bus_set_iommu (&pci_bus_type, &intel_iommu_ops);if (si_domain && !hw_pass_through)register_memory_notifier (&intel_iommu_memory_nb);
drivers/iommu/iommu.c +0 -24
@@ -1827,30 +1827,6 @@ int bus_iommu_probe(struct bus_type *bus)return ret; } -/** -* bus_set_iommu - set iommu-callbacks for the bus -* @bus: bus. -* @ops: the callbacks provided by the iommu-driver -* -* This function is called by an iommu driver to set the iommu methods -* used for a particular bus. Drivers for devices on that bus can use -* the iommu-api after these ops are registered. -* This special function is needed because IOMMUs are usually devices on -* the bus itself, so the iommu drivers are not initialized when the bus -* is set up. With this function the iommu-driver can set the iommu-ops -* afterwards. -*/ -int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops) -{ -if (bus->iommu_ops && ops && bus->iommu_ops != ops) -return -EBUSY ; - - bus->iommu_ops = ops; - -return 0 ; -} -EXPORT_SYMBOL_GPL (bus_set_iommu); -bool iommu_present (struct bus_type *bus) {return bus->iommu_ops !=NULL ;
drivers/iommu/msm_iommu.c +0 -2
@@ -792,8 +792,6 @@ static int msm_iommu_probe(struct platform_device *pdev) goto fail; } -bus_set_iommu (&platform_bus_type, &msm_iommu_ops); -pr_info ("device mapped at %p, irq %d with %d ctx banks\n" , iommu->base, iommu->irq, iommu->ncb);
drivers/iommu/rockchip-iommu.c +0 -2
@@ -1300,8 +1300,6 @@ static int rk_iommu_probe(struct platform_device *pdev)if (!dma_dev) dma_dev = &pdev->dev; -bus_set_iommu (&platform_bus_type, &rk_iommu_ops); -pm_runtime_enable (dev);for (i =0 ; i < iommu->num_irq; i++) {
drivers/iommu/s390-iommu.c +0 -6
@@ -390,9 +390,3 @@ static const struct iommu_ops s390_iommu_ops = { .free = s390_domain_free, } }; - -static int __init s390_iommu_init (void ) -{ -return bus_set_iommu (&pci_bus_type, &s390_iommu_ops); -} -subsys_initcall (s390_iommu_init);
drivers/iommu/sprd-iommu.c +0 -5
@@ -496,9 +496,6 @@ static int sprd_iommu_probe(struct platform_device *pdev)if (ret) goto remove_sysfs; -if (!iommu_present (&platform_bus_type)) -bus_set_iommu (&platform_bus_type, &sprd_iommu_ops); - ret =sprd_iommu_clk_enable (sdev);if (ret) goto unregister_iommu;
@@ -534,8 +531,6 @@ static int sprd_iommu_remove(struct platform_device *pdev)iommu_group_put (sdev->group); sdev->group =NULL ; -bus_set_iommu (&platform_bus_type,NULL ); -platform_set_drvdata (pdev,NULL );iommu_device_sysfs_remove (&sdev->iommu);iommu_device_unregister (&sdev->iommu);
drivers/iommu/sun50i-iommu.c +0 -2
@@ -965,8 +965,6 @@ static int sun50i_iommu_probe(struct platform_device *pdev)if (ret <0 ) goto err_unregister; -bus_set_iommu (&platform_bus_type, &sun50i_iommu_ops); -return 0 ; err_unregister:
include/linux/iommu.h +0 -1
@@ -416,7 +416,6 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)return dev->iommu->iommu_dev->ops; } -extern int bus_set_iommu (struct bus_type *bus,const struct iommu_ops *ops);extern int bus_iommu_probe (struct bus_type *bus);extern bool iommu_present (struct bus_type *bus);extern bool device_iommu_capable (struct device *dev,enum iommu_cap cap);
Signature · 1 line
2.36.1.dirty
Quoted 19 lines · Robin Murphy
Matthew Rosato (1):
iommu/s390: Fail probe for non-PCI devicesRobin Murphy (15):
iommu/vt-d: Handle race between registration and device probe iommu/amd: Handle race between registration and device probe
iommu: Always register bus notifiers
iommu: Move bus setup to IOMMU device registration iommu/amd: Clean upbus_set_iommu()
iommu/arm-smmu: Clean upbus_set_iommu()iommu/arm-smmu-v3: Clean upbus_set_iommu()iommu/dart: Clean upbus_set_iommu()
iommu/exynos: Clean upbus_set_iommu()iommu/ipmmu-vmsa: Clean upbus_set_iommu()iommu/mtk: Clean upbus_set_iommu()
iommu/omap: Clean upbus_set_iommu()iommu/tegra-smmu: Clean upbus_set_iommu()iommu/virtio: Clean upbus_set_iommu()
iommu: Clean upbus_set_iommu()
Applied, thanks Robin.