Re: [PATCH v3 06/11] iommu: Defer __iommu_group_free_device() to be outside group->mutex

[Baolu Lu]

On 4/17/26 07:28, Nicolin Chen wrote:
> __iommu_group_remove_device() holds group->mutex across the entire call to
> __iommu_group_free_device() that performs sysfs removals, tracing, and the
> final kfree_rcu(). But in fact, most of these operations don't really need
> the group->mutex.
>
> The group_device structure will support a work_struct to quarantine broken
> devices asynchronously. The work function must hold group->mutex to safely
> update group state. cancel_work_sync() must be called, to cancel that work
> before freeing the device. But doing so under group->mutex would deadlock
> if the worker is already running and waiting to acquire the same lock.
>
> Separate the assertion from __iommu_group_free_device() to another helper
> __iommu_group_empty_assert_owner_cnt().
>
> Defer the __iommu_group_free_device() until the mutex is released.
>
> This is a preparatory refactor with no functional change.
>
> Signed-off-by: Nicolin Chen<nicolinc@xxxxxxxxxx>
> ---
>   drivers/iommu/iommu.c | 35 +++++++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d1be62a07904a..810e7b94a1ae2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -627,6 +627,19 @@ static struct iommu_domain *pasid_array_entry_to_domain(void *entry)
>   
>   DEFINE_MUTEX(iommu_probe_device_lock);
>   
> +static void __iommu_group_empty_assert_owner_cnt(struct iommu_group *group)
> +{
> +	lockdep_assert_held(&group->mutex);
> +	/*
> +	 * If the group has become empty then ownership must have been
> +	 * released, and the current domain must be set back to NULL or
> +	 * the default domain.
> +	 */

Nit: this comment doesn't quite match the following code. The code
doesn't check "group->domain != NULL". Or perhaps in that case,
group->default_domain must be NULL?

Furthermore, if a device is currently quarantined, group->domain will be
the blocking_domain. If that quarantined device is then hot-removed and
happens to be the last device in the group, will this WARN_ON trigger
unnecessarily?

> +	if (list_empty(&group->devices))
> +		WARN_ON(group->owner_cnt ||
> +			group->domain != group->default_domain);
> +}
> +
>   static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
>   {
>   	struct iommu_group *group;

Thanks,
baolu