Re: [PATCH v3 01/13] iommu/arm-smmu-v3: Add arm_smmu_attach_release()

[Nicolin Chen <nicolinc@nvidia.com>]

Thanks for the reviews.

On Thu, Sep 03, 2026 at 12:18:33PM -0700, Jonathan Cameron wrote:
> > The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
> > the master_domain on detach, under the global arm_smmu_asid_lock mutex. But
> > the teardown must drain any in-flight IOPF (for the old domain), before the
> > master_domain is freed via iopf_queue_flush_dev() calling flush_workqueue()
> > that can block on user-faulting page-fault handlers. Doing so while holding
> > the arm_smmu_asid_lock would stall any unrelated attachment in the system.
> > 
> > Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
> > arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.
> > 
> > Since no other device would use the old master_domain that is being freed,
> > it's safe to move out of arm_smmu_asid_lock (still under the protection of
> > iommu_group->mutex).
> 
> This is a lot of text if the next bit about being a refactor only
> is accurate.  Seems that not blocking attachments is the issue and
> to me that is a functional and useful change. However, is that
> in this patch?  Anyhow to me this needs a rewrite to focus on just
> what is actually changing here rather than the eventual picture.

I rewrote it:

The IOPF teardown is done in arm_smmu_remove_master_domain() when releasing
the master_domain on detach, under the global arm_smmu_asid_lock mutex.

A later change will add an IOPF workqueue flush to that teardown, which can
block on a user-faulting page-fault handler. Holding the arm_smmu_asid_lock
across it would stall every unrelated attachment in the system.

Split the teardown out of arm_smmu_remove_master_domain(), to a new helper
arm_smmu_attach_release() that runs after arm_smmu_asid_lock is released.

No functional change: the old master_domain belongs to no other device, so
freeing it outside the lock stays safe, still under iommu_group->mutex.

> > +/* Release the old master_domain detached by arm_smmu_remove_master_domain() */
> > +void arm_smmu_attach_release(struct arm_smmu_attach_state *state)
> > +{
> > +	struct arm_smmu_master_domain *master_domain = state->old_master_domain;
> > +	struct arm_smmu_master *master = state->master;
> > +
> > +	iommu_group_mutex_assert(master->dev);
> > +
> > +	if (!master_domain)
> 
> I guess this makes sense in later patches, but for now the local
> variable seems more confusing than anything.

I'd like to keeping this: this is prep patch anyway, so pre-adding
the local variable here can make later patches slightly cleaner.

> > +		return;
> 
> I'd add a blank line here to separate the sanity checks from bulk
> code.

Done.

> >  	arm_smmu_disable_iopf(master, master_domain);
> >  	kfree(master_domain);
> > +	state->old_master_domain = NULL;
> >  }
> >  
> 
> > @@ -3784,6 +3801,7 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
> >
> This path is hit from a failure of arm_smmu_attach_prepare()
> At that point the old domain hasn't been detached.
> 
> Now it doesn't matter because of what is currently done in release,
> but from a code flow / what that function is documented to be for
> this seems wrong to me.  I'd separate the good and the bad
> paths in the function.

I cleaned that up -- once prepare() is done, it is in a no-fail path:

@@ -3783,8 +3784,10 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 
 	mutex_lock(&arm_smmu_asid_lock);
 	ret = arm_smmu_attach_prepare(&state, &smmu_domain->domain);
-	if (ret)
-		goto out_unlock;
+	if (ret) {
+		mutex_unlock(&arm_smmu_asid_lock);
+		return ret;
+	}
 
 	/*
 	 * We don't want to obtain to the asid_lock too early, so fix up the
@@ -3798,11 +3801,9 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 	arm_smmu_update_ste(master, sid_domain, state.ats_enabled);
 
 	arm_smmu_attach_commit(&state);
-
-out_unlock:
 	mutex_unlock(&arm_smmu_asid_lock);
 	arm_smmu_attach_release(&state);
-	return ret;
+	return 0;
 }
 
 static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,

Thanks
Nicolin