RE: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain

From: "Tian, Kevin" <kevin.tian@intel.com>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Qian Cai <quic_qiancai@quicinc.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"Rodel, Jorg" <jroedel@suse.de>,
	Robin Murphy <robin.murphy@arm.com>,
	Will Deacon <will@kernel.org>
Subject: RE: [PATCH v2] iommu: iommu_group_claim_dma_owner() must always assign a domain
Date: Thu, 5 May 2022 23:28:58 +0000	[thread overview]
Message-ID: <BN9PR11MB5276476BB203D8D61CA8849C8CC29@BN9PR11MB5276.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20220505153320.GS49344@nvidia.com>

> From: Jason Gunthorpe
> Sent: Thursday, May 5, 2022 11:33 PM
> > >  	/*
> > > -	 * If the group has been claimed already, do not re-attach the default
> > > -	 * domain.
> > > +	 * New drivers should support default domains and so the
> > > detach_dev() op
> > > +	 * will never be called. Otherwise the NULL domain indicates the
> > > +	 * translation for the group should be set so it will work with the
> >
> > translation should be 'blocked'?
> 
> No, not blocked.
> 
> > > +	 * platform DMA ops.
> >
> > I didn't get the meaning of the last sentence.
> 
> It is as discussed with Robin, NULL means to return the group back to
> some platform defined translation, and in some cases this means
> returning back to work with the platform's DMA ops - presumably if it
> isn't using the dma api.

This is clearer than the original text.

> 
> > > +	/*
> > > +	 * Changing the domain is done by calling attach on the new domain.
> > > +	 * Drivers should implement this so that DMA is always translated by
> >
> > what does 'this' mean?
> 
> The code below - attach_dev called in a loop for each dev in the group.

Yes.

> 
> > > +	 * either the new, old, or a blocking domain. DMA should never
> >
> > isn't the blocking domain passed in as the new domain?
> 
> Soemtimes. This is a statement about the required
> atomicity. New/old/block are all valid translations during the
> operation. Identity is not.

but new/old/block are not the same type of classifications. A group
has an old domain and a new domain at this transition point, and
both old/new domains have a domain type (dma, unmanged, block,
identity, etc.). Mixing them together only increases confusion here.

> 
> So, I'm going to leave this patch as-is since it has been tested now
> and we need to get the series back into iommu-next ASAP.
> 

Agree. Just hope some improvements on the code comment.

But either way given no more code change:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu