Re: [PATCH] iommu: Reorganize __iommu_attach_group()

[Jason Gunthorpe via iommu <iommu@lists.linux-foundation.org>]

On Fri, May 06, 2022 at 05:44:11PM +0100, Robin Murphy wrote:

> > > Nit: if that's true then it's equally true for iommu_attach_group() as well.
> > 
> > Is it? I didn't check this as closely..
> 
> Well, there certainly seems no obvious reason for one to WARN where the
> other fails quietly. Either it's an egregious loss of internal consistency
> to not know whether your thing is already attached or it isn't, but I don't
> see why the exact flavour of attach API called should matter.

I will try to check

> > We don't have a name for the set of domains owned by the core code vs a
> > domain set externally.
> > 
> > If you don't like core how about internal/external?
> 
> Oh no, I rather like the "core domain" nomenclature, it's the
> "iommu_group_is_..." part that then fails to parse sensibly.

Oh Ok

> > > Even getting past that, does it make sense to say NULL
> > > is a core domain? I'm not convinced.
> > 
> > NULL is not an externally imposed domain so it is definately a
> > core/internal domain in this logic.
> 
> So if it *is* a domain then I can call NULL->attach_dev() and...? ;)

You can call iommu_group_set_domain(group, NULL) and it will work.

As I said, it must have this symmetry:

 __iommu_group_attach_core_domain()
 assert(__iommu_group_is_core_domain())

This is why I choose the name, because it is a clear pairing with
__iommu_group_attach_core_domain().

How about __iommu_group_is_core_domain_attached() ? Solves the grammer
issue

> > > For the sake of future readability, I'd
> > > prefer to call this something more purpose-related like
> > > __iommu_group_can_attach()
> > 
> > That is not the case - we can always attach any domain.
> 
> No, in the context of the iommu_attach_{device,group}() APIs where this
> helper is relevant, after a successful attach, it has long been required to
> detach before another attach can be performed. That's literally the code
> you're moving around in this patch.


> This is precisely why I'm starting to think it would be beneficial to
> differentiate the internal interface now that it's firmly moving away from
> the attach/detach paradigm still exposed externally.

I'm viewing the API family of __iommu_group_attach_core_domain /
__iommu_group_set_domain / __iommu_group_is_core_domain_attached()
a layering point - the other APIs are being built on top of this
family.

It is now exposing a semantic the same as the default-domain enabled
driver ops, but is using different nomenclature.

Should it be __iommu_group_set_core_domain() ?

This naming seems way harder than it really should be.

> Heh, if we can ever get to the point where we don't have differing old and
> new semantics side-by-side at almost every level, maybe then we can find the
> right name for *everything*. Besides, it's arguably even weirder for
> attach_domain to be the only way to call detach_dev, so there's that.

It is because the ops are misnamed for what they now do.

 attach_dev is really set_domain_for_dev()
 detach_dev is really reset_dev_to_platform()

Which definitely is the root cause of the confusion that created this
bug in the first place.

Anyhow, I'll post the fixing patch again with the new comment and we
can decide how to revise the language separately, no need to delay
getting Lu's series back into the testing stream.

Thanks,
Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu