From: Robin Murphy <robin.murphy@arm.com>
Sent: Thursday, July 7, 2022 6:58 PM

On 2022-07-07 07:51, Tian, Kevin wrote:

quoted

quoted

From: Robin Murphy <robin.murphy@arm.com>
Sent: Wednesday, July 6, 2022 1:08 AM

@@ -202,12 +210,32 @@ int iommu_device_register(struct

iommu_device

quoted

quoted

*iommu,
  	spin_lock(&iommu_device_lock);
  	list_add_tail(&iommu->list, &iommu_device_list);
  	spin_unlock(&iommu_device_lock);
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		struct bus_type *bus = iommu_buses[i];
+		int err;
+
+		if (bus->iommu_ops && bus->iommu_ops != ops) {
+			err = -EBUSY;
+		} else {
+			bus->iommu_ops = ops;
+			err = bus_iommu_probe(bus);
+		}
+		if (err) {
+			iommu_device_unregister(iommu);
+			return err;
+		}
+	}
+

Probably move above into a new function bus_iommu_probe_all():

	/* probe all buses for devices associated with this iommu */
	err = bus_iommu_probe_all();
	if (err) {
		iommu_device_unregister(iommu);
		return err;
	}

Just  my personal preference on leaving logic in iommu_device_register()
more relevant to the iommu instance itself.

On reflection I think it makes sense to pull the
iommu_device_unregister() out of the loop anyway - I think that's really
a left-over from between v1 and v2 when that error case briefly jumped
to another cleanup loop, before I realised it was actually trivial for
iommu_device_unregister() to clean up for itself.

However I now see I've also missed another opportunity, and the -EBUSY
case should be hoisted out of the loop as well, since checking
iommu_buses[0] is sufficient. Then it's hopefully much clearer that once
the bus ops go away we'll be left with just a single extra line for the
loop, as in iommu_device_unregister(). Does that sound reasonable?

Yes, sounds good to me.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel