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

[Baolu Lu]

On 4/23/26 23:47, Nicolin Chen wrote:
> On Thu, Apr 23, 2026 at 03:55:02PM +0800, Baolu Lu wrote:
> > On 4/17/26 07:28, Nicolin Chen wrote:
> > > +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?
>
> This is the original patch from Jason:
> https://lore.kernel.org/r/4-v3-328044aa278c+45e49-iommu_probe_jgg@xxxxxxxxxx
>
> I kept the comments as-is, though It might be slightly confusing?
>
> I think it means:
> If group->default_domain == NULL, it does check "set back to NULL".
> If group->default_domain != NULL, it then checks "default domain".
>
> Maybe it could be "must be set back to the default domain (which
> itself can be NULL"?

This is clearer. As I understand it, when the last device leaves the
iommu_group, the group->domain should be one of the static system
domains, either the default domain or the blocking domain.

> > 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 a device is quarantined, its group->domain is retained to the
> previously attached domain. Its blocking state is logged in the
> gdev->blocked flag. So, I think it can pass the test.

Oh, my mistake. I thought group->domain would point to the blocking
domain when a device is quarantined. It's fine if group->domain remains
set to the previous domain.

> Thanks
> Nicolin

Thanks,
baolu