Re: [PATCH v3 07/17] iommu: Make iommu_group_do_dma_first_attach() simpler

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 06, 2023 at 04:44:57PM +0100, Robin Murphy wrote:
> On 06/04/2023 12:38 am, Jason Gunthorpe wrote:
> > It should always attach to the current group->domain, so don't take in a
> > domain parameter. Use the __iommu_device_set_domain() common code to
> > handle the attach.
> 
> Hmm, why do we still end up with this in two places, and potentially called
> twice in the same path?
> 
> iommu_probe_device()
>   __iommu_probe_device()
>     iommu_group_get_for_dev()
>       iommu_group_add_device()
>         iommu_group_do_dma_first_attach()
>   iommu_create_device_direct_mappings()
>   iommu_group_do_dma_first_attach()
> 
> Seems off to me... at best it's confusing, but at worst it's plain wrong
> (it's the one in iommu_group_add_device() that I'm most suspicious of)

Lu and Kevin pointed to this as well, it started out that way.

I made a whole other series tidying the probe related flows just based
on this observation.. 

I can pull one patch into here since this seems to catch everyone.

AFIACT it is harmless at this point since we can safely call
first_attach twice?

> Not to mention the slightly different open-coded variant we still seem to
> have in iommu_setup_default_domain() via the "else if
> (!group->default_domain)" path :/

Yes.. Humm. More parts could be moved into a function but I think it
would obfuscates what iommu_setup_default_domain() does.

Oh, I see another mistake:

	if (group->domain) {
		iommu_create_device_direct_mappings(group->domain, dev);
                                                  ^^^^^^^^^^^^^^^

iommu_create_device_direct_mappings should only be called on the
default domain.

> (BTW if any comments seem misplaced, it's because I'm really looking at the
> end result at the cover-letter commit in your branch, rather than trying to
> review each patch independently)

That is probably best. The patches are OK but it is hard to see the
final arrangment

Thanks,
Jason