Re: [PATCH v2 03/14] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

Hi Robin,

On 2022/4/28 21:18, Robin Murphy wrote:
> Move 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.

I re-fetched the latest patches on

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

and rolled back the head to "iommu: Cleanup bus_set_iommu".

The test machine still hangs during boot.

I went through the code. It seems that the .probe_device for Intel IOMMU
driver can't handle the probe replay well. It always assumes that the
device has never been probed.

Best regards,
baolu

> 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 <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
>   drivers/iommu/iommu.c | 51 +++++++++++++++++++++++--------------------
>   1 file changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c4621afc8cf..c89af4dc54c2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -175,6 +175,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
> @@ -197,12 +205,29 @@ 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;
> +
> +		WARN_ON(bus->iommu_ops && bus->iommu_ops != ops);
> +		bus->iommu_ops = ops;
> +		err = bus_iommu_probe(bus);
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +
>   	return 0;
>   }
>   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);
> @@ -1655,13 +1680,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)
>   {
> @@ -1884,27 +1902,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;
> -	}
> -
> -	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);
>