On 2022-07-07 07:51, Tian, Kevin wrote:
From: Robin Murphy <robin.murphy@xxxxxxx> Sent: Wednesday, July 6, 2022 1:08 AM @@ -202,12 +210,32 @@ 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); + + 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?
Apart from that: Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Thanks! (and for the others as well) Robin.
