iommu: Retire bus_set_iommu()

drivers/iommu/iommu.c +0 -2

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus)
   */
  int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
  {
-	int err;
-

Quoted 9 lines · Robin Murphy

drivers/iommu/iommu.c +0 -5

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(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;

drivers/iommu/iommu.c static int iommu_bus_init(struct bus_type *bus) · line 1906

Quoted 6 lines

drivers/iommu/iommu.c +1 -3

-    }
-
-    if (bus->iommu_ops != NULL)
+    if (bus->iommu_ops && ops && bus->iommu_ops != ops)
           return -EBUSY;
       bus->iommu_ops = ops;

Do we still need to keep above lines in bus_set_iommu()?

Quoted 10 lines · Lu Baolu

Quoted 9 lines · Robin Murphy

drivers/iommu/iommu.c +0 -5

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(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;

Quoted 8 lines

drivers/iommu/iommu.c static int iommu_bus_init(struct bus_type *bus) · line 1906

Quoted 6 lines

drivers/iommu/iommu.c +1 -3

-    }
-
-    if (bus->iommu_ops != NULL)
+    if (bus->iommu_ops && ops && bus->iommu_ops != ops)
           return -EBUSY;
       bus->iommu_ops = ops;

Do we still need to keep above lines in bus_set_iommu()?

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Quoted 20 lines · Robin Murphy

Quoted 18 lines · Lu Baolu

Quoted 5 lines · Robin Murphy

drivers/iommu/iommu.c +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

drivers/iommu/iommu.c static int iommu_bus_init(struct bus_type *bus) · line 1905

Quoted 10 lines

drivers/iommu/iommu.c +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

drivers/iommu/iommu.c +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type 
 *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

drivers/iommu/iommu.c static int iommu_bus_init(struct bus_type · line 1906

Quoted 10 lines

drivers/iommu/iommu.c +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

drivers/iommu/iommu.c +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type 
 *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

drivers/iommu/iommu.c static int iommu_bus_init(struct bus_type · line 1906

Quoted 10 lines

drivers/iommu/iommu.c +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

Hi Robin,

Quoted 31 lines · Robin Murphy

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

(quoted diff) +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type 
 *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

Review static int iommu_bus_init(struct bus_type · line 1906

Quoted 10 lines

(quoted diff) +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

Quoted 5 lines

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

I wound the head back to "iommu: Clean up bus_set_iommu" and tested it on an Intel machine. It got stuck during boot. This test was on a remote machine and I have no means to access it physically. So I can't get any kernel debugging messages. (I have to work from home these days. :-()

I guess it's due to the fact that intel_iommu_probe_device() callback only works for the pci devices. The issue occurs when probing a device other than a PCI one.

Quoted 40 lines · Lu Baolu

Hi Robin,

Quoted 31 lines · Robin Murphy

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

(quoted diff) +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct bus_type 
 *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

Review static int iommu_bus_init(struct bus_type · line 1906

Quoted 10 lines

(quoted diff) +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

Quoted 5 lines

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

Quoted 8 lines

I wound the head back to "iommu: Clean up bus_set_iommu" and tested it on an Intel machine. It got stuck during boot. This test was on a remote machine and I have no means to access it physically. So I can't get any kernel debugging messages. (I have to work from home these days. :-()

I guess it's due to the fact that intel_iommu_probe_device() callback only works for the pci devices. The issue occurs when probing a device other than a PCI one.

Yeah, I was wondering if that would be significant, since it's the only driver that never registered itself for platform_bus_type so won't have actually seen those calls before. I'm inclined to bodge that as below for now, as long as it then works OK in terms of the rest of the changes.

Thanks,
Robin.

----->8-----

drivers/iommu/intel/iommu.c +4 -0

index 9fa1b98186a3..6e359f92ec00 100644
@@ -4565,6 +4565,10 @@ static struct iommu_device 
 *intel_iommu_probe_device(struct device *dev)
      unsigned long flags;
      u8 bus, devfn;
 
+    /* ANDD platform device support needs fixing */
+    if (!pdev)
+        return ERR_PTR(-ENODEV);
+
      iommu = device_to_iommu(dev, &bus, &devfn);
      if (!iommu)

         return ERR_PTR(-ENODEV);

Quoted 65 lines · Robin Murphy

Quoted 40 lines · Lu Baolu

Hi Robin,

Quoted 31 lines · Robin Murphy

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

(quoted diff) +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct 
 bus_type *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

Review static int iommu_bus_init(struct · line 1906

Quoted 10 lines

(quoted diff) +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

Quoted 5 lines

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

Quoted 8 lines

I wound the head back to "iommu: Clean up bus_set_iommu" and tested it on an Intel machine. It got stuck during boot. This test was on a remote machine and I have no means to access it physically. So I can't get any kernel debugging messages. (I have to work from home these days. :-()

I guess it's due to the fact that intel_iommu_probe_device() callback only works for the pci devices. The issue occurs when probing a device other than a PCI one.

Yeah, I was wondering if that would be significant, since it's the only driver that never registered itself for platform_bus_type so won't have actually seen those calls before. I'm inclined to bodge that as below for now, as long as it then works OK in terms of the rest of the changes.

Thanks,
Robin.

----->8-----

drivers/iommu/intel/iommu.c +0 -0

index 9fa1b98186a3..6e359f92ec00 100644
@@ -4565,6 +4565,10 @@ static struct iommu_device 
 *intel_iommu_probe_device(struct device *dev)
       unsigned long flags;
       u8 bus, devfn;
 

Quoted 7 lines

drivers/iommu/intel/iommu.c +4 -0

+    /* ANDD platform device support needs fixing */
+    if (!pdev)
+        return ERR_PTR(-ENODEV);
+
       iommu = device_to_iommu(dev, &bus, &devfn);
       if (!iommu)

          return ERR_PTR(-ENODEV);

I haven't seen any real ANDD platform devices, hence this works for me.

Quoted 66 lines · Lu Baolu

Quoted 65 lines · Robin Murphy

Quoted 40 lines · Lu Baolu

Hi Robin,

Quoted 31 lines · Robin Murphy

Quoted 22 lines · Lu Baolu

Quoted 21 lines · Robin Murphy

Quoted 19 lines · Lu Baolu

Quoted 6 lines · Robin Murphy

(quoted diff) +0 -1

@@ -1883,27 +1900,12 @@ static int iommu_bus_init(struct 
 bus_type *bus)
    */
   int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
   {
-    int err;

Review static int iommu_bus_init(struct · line 1906

Quoted 10 lines

(quoted diff) +1 -7

-
-    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 we still need to keep above lines in bus_set_iommu()?

Quoted 7 lines

Quoted 1 line

It preserves the existing behaviour until each callsite and its associated error handling are removed later on, which seems like as good a thing to do as any. Since I'm already relaxing iommu_device_register() to a warn-but-continue behaviour while it keeps the bus ops on life-support internally, I figured not changing too much at once would make it easier to bisect any potential issues arising from this first step.

Fair enough. Thank you for the explanation.

Do you have a public tree that I could pull these patches and try them on an Intel hardware? Or perhaps you have done this? I like the whole idea of this series, but it's better to try it with a real hardware.

Quoted 5 lines

I haven't bothered with separate branches since there's so many different pieces in-flight, but my complete (unstable) development branch can be found here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/iommu/bus

For now I'd recommend winding the head back to "iommu: Clean up bus_set_iommu()" for testing - some of the patches above that have already been posted and picked up by their respective subsystems, but others are incomplete and barely compile-tested. I'll probably rearrange it later this week to better reflect what's happened so far.

Quoted 8 lines

I wound the head back to "iommu: Clean up bus_set_iommu" and tested it on an Intel machine. It got stuck during boot. This test was on a remote machine and I have no means to access it physically. So I can't get any kernel debugging messages. (I have to work from home these days. :-()

I guess it's due to the fact that intel_iommu_probe_device() callback only works for the pci devices. The issue occurs when probing a device other than a PCI one.

Yeah, I was wondering if that would be significant, since it's the only driver that never registered itself for platform_bus_type so won't have actually seen those calls before. I'm inclined to bodge that as below for now, as long as it then works OK in terms of the rest of the changes.

Thanks,
Robin.

----->8-----

drivers/iommu/intel/iommu.c +0 -0

index 9fa1b98186a3..6e359f92ec00 100644
@@ -4565,6 +4565,10 @@ static struct iommu_device 
 *intel_iommu_probe_device(struct device *dev)
       unsigned long flags;
       u8 bus, devfn;
 

Quoted 9 lines

Quoted 7 lines

drivers/iommu/intel/iommu.c +4 -0

+    /* ANDD platform device support needs fixing */
+    if (!pdev)
+        return ERR_PTR(-ENODEV);
+
       iommu = device_to_iommu(dev, &bus, &devfn);
       if (!iommu)

          return ERR_PTR(-ENODEV);

I haven't seen any real ANDD platform devices, hence this works for me.

Or more precisely, return NULL when @dev goes through device_to_iommu()?

drivers/iommu/intel/iommu.c +4 -1

index df5c62ecf942..0d447739e076 100644
@@ -797,8 +797,11 @@ struct intel_iommu *device_to_iommu(struct device 
 *dev, u8 *bus, u8 *devfn)
                 pf_pdev = pci_physfn(pdev);
                 dev = &pf_pdev->dev;
                 segment = pci_domain_nr(pdev->bus);
-       } else if (has_acpi_companion(dev))
+       } else if (has_acpi_companion(dev)) {
                 dev = &ACPI_COMPANION(dev)->dev;
+       } else {
+               return NULL;
+       }
 
         rcu_read_lock();

        for_each_iommu(iommu, drhd) {

drivers/iommu/iommu.c +10 -0

+       for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+               struct bus_type *bus = iommu_buses[i];
+               const struct iommu_ops *bus_ops = bus->iommu_ops;
+               int err;
+
+               WARN_ON(bus_ops && bus_ops != ops);
+               bus->iommu_ops = ops;
+               err = bus_iommu_probe(bus);
+               if (err) {
+                       bus_for_each_dev(bus, NULL, iommu,

remove_iommu_group);

Good effort to isolate bus config from smmu drivers.

Reviewed-by Krishna Reddy <[email protected]>

I have an orthogonal question here.
Can the following code handle the case, where different buses have different type of SMMU instances(like one bus has SMMUv2 and another bus has SMMUv3)? If it need to handle the above case, can the smmu device bus be matched with specific bus here and ops set only for that bus?

Quoted 13 lines

drivers/iommu/iommu.c +10 -0

+       for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+               struct bus_type *bus = iommu_buses[i];
+               const struct iommu_ops *bus_ops = bus->iommu_ops;
+               int err;
+
+               WARN_ON(bus_ops && bus_ops != ops);
+               bus->iommu_ops = ops;
+               err = bus_iommu_probe(bus);
+               if (err) {
+                       bus_for_each_dev(bus, NULL, iommu,

remove_iommu_group);

drivers/iommu/iommu.c int iommu_device_register(struct iommu_device *iommu, · line 222

Quoted 4 lines

drivers/iommu/iommu.c +4 -0

+                       bus->iommu_ops = bus_ops;
+                       return err;
+               }
+       }

-KR

Stop 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!

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +--------------------------
 1 file changed, 2 insertions(+), 82 deletions(-)

Quoted 3 lines · Robin Murphy

Stop 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!

Ha, I hope you tested this!

Quoted 4 lines

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +--------------------------
  1 file changed, 2 insertions(+), 82 deletions(-)

Assuming it works,

Acked-by Will Deacon <[email protected]>

Will

Quoted 6 lines · Will Deacon

Quoted 3 lines · Robin Murphy

Stop 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!

Ha, I hope you tested this!

Oh alright then, since it's you... :)

I've hacked up a Juno DT with the old bindings, and (after needing a while to remember that they're fundamentally incompatible with disable_bypass), can confirm that with my whole dev branch including this series applied, it boots and creates IOMMU groups as expected. I then made the mistake of trying without the branch to check whether the squawks from iommu_setup_dma_ops() were new or not, and... well... plain rc3 doesn't even boot on the same setup - it's somehow blowing up in the failure cleanup path of iommu_bus_init(), apparently calling iommu_release_device() on something where dev->iommu->iommu_dev is NULL, for reasons that are far from clear and I'm not sure I can really be bothered to debug further... :/

Stop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +--------------------
 1 file changed, 2 insertions(+), 51 deletions(-)

Stop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.

Signed-off-by Robin Murphy <[email protected]>

Stop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/virtio-iommu.c | 24 ------------------------
 1 file changed, 24 deletions(-)

drivers/iommu/virtio-iommu.c +0 -0

index 25be4b822aa0..371f8657c0ce 100644

drivers/iommu/virtio-iommu.c +0 -23

@@ -1146,26 +1145,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",

@@ -1174,9 +1153,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.28.0.dirty

Quoted 16 lines · Robin Murphy

Stop calling bus_set_iommu() since it's now unnecessary, and simplify the probe failure path accordingly.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/virtio-iommu.c | 24 ------------------------
  1 file changed, 24 deletions(-)

drivers/iommu/virtio-iommu.c +0 -0

index 25be4b822aa0..371f8657c0ce 100644
@@ -7,7 +7,6 @@
 
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 

drivers/iommu/virtio-iommu.c line 12

Quoted 4 lines

drivers/iommu/virtio-iommu.c +0 -1

-#include <linux/amba/bus.h>
  #include <linux/delay.h>
  #include <linux/dma-iommu.h>
  #include <linux/dma-map-ops.h>

<linux/platform_device.h> isn't needed anymore either. In any case it looks great, thanks

Reviewed-by Jean-Philippe Brucker <[email protected]>

and tested on QEMU (so only PCI for now)

Quoted 37 lines

drivers/iommu/virtio-iommu.c +0 -23

@@ -1146,26 +1145,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",

@@ -1174,9 +1153,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);
 

Quoted 3 lines

Signature · 1 line

2.28.0.dirty

Hi all,

Here's another chapter in my saga of moving to per-instance IOMMU ops - iommu_present() and iommu_capable() cleanups will be ongoing for another cycle or two, while this one is at least self-contained within the subsystem. The next steps after this are making iommu_domain_alloc() instance-aware - which should finish the public API - and pulling the fwnode/of_xlate bits into __iommu_probe_device(). And then making sense of whatever's left :)

For ease of review here I split out individual driver patches based on whether there was any non-trivial change or affect on control flow; the straightforward deletions are all lumped together since the whole series needs applying together either way, but I'm happy to split the final patch up further if anyone would like.

Patch #3 for AMD is based on Mario's SWIOTLB patch here:

https://lore.kernel.org/linux-iommu/[email protected]/

since that wants merging first as fix material. The series is also based contextually (but not functionally) on my device_iommu_capable() patches here:

https://lore.kernel.org/linux-iommu/[email protected]/

since those are pretty much good to go now (I'll send a slightly-tweaked final version once the iommu/core branch is open).

Thanks,
Robin.

Robin Murphy (13):
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                   |  21 ----
  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                 |   1 -
  drivers/iommu/iommu.c                       | 109 +++++++++-----------
  drivers/iommu/ipmmu-vmsa.c                  |  35 +------
  drivers/iommu/msm_iommu.c                   |   2 -
  drivers/iommu/mtk_iommu.c                   |  13 +--
  drivers/iommu/mtk_iommu_v1.c                |  13 +--
  drivers/iommu/omap-iommu.c                  |   6 --

  drivers/iommu/rockchip-iommu.c              |   2 -
  drivers/iommu/s390-iommu.c                  |   6 --
  drivers/iommu/sprd-iommu.c                  |   5 -
  drivers/iommu/sun50i-iommu.c                |   2 -
  drivers/iommu/tegra-smmu.c                  |  29 ++----
  drivers/iommu/virtio-iommu.c                |  24 -----
  include/linux/iommu.h                       |   1 -
  23 files changed, 62 insertions(+), 401 deletions(-)