> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Friday, September 2, 2022 8:29 AM
>
> On Wed, Aug 31, 2022 at 08:46:30AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Sent: Wednesday, August 31, 2022 9:02 AM
> > >
> > > To vfio_container_detatch_group(). This function is really a container
> > > function.
> > >
> > > Fold the WARN_ON() into it as a precondition assertion.
> > >
> > > A following patch will move the vfio_container functions to their own .c
> > > file.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > ---
> > > drivers/vfio/vfio_main.c | 11 +++++------
> > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > > index bfa6119ba47337..e145c87f208f3a 100644
> > > --- a/drivers/vfio/vfio_main.c
> > > +++ b/drivers/vfio/vfio_main.c
> > > @@ -928,12 +928,13 @@ static const struct file_operations vfio_fops = {
> > > /*
> > > * VFIO Group fd, /dev/vfio/$GROUP
> > > */
> > > -static void __vfio_group_unset_container(struct vfio_group *group)
> > > +static void vfio_container_detatch_group(struct vfio_group *group)
> >
> > s/detatch/detach/
>
> Oops
>
> > Given it's a vfio_container function is it better to have a container pointer
> > as the first parameter, i.e.:
> >
> > static void vfio_container_detatch_group(struct vfio_container *container,
> > struct vfio_group *group)
>
> Ah, I'm not so sure, it seems weird to make the caller do
> group->container then pass the group in as well.
>
> This call assumes the container is the container of the group, it
> doesn't work in situations where that isn't true.
Yes, this is a valid interpretation on doing this way.
Other places e.g. iommu_detach_group(domain, group) go the other way
even if internally domain is not used at all. I kind of leave that pattern
in mind thus raised this comment. But not a strong opinion.
>
> It is kind of weird layering in a way, arguably we should have the
> current group stored in the container if we want things to work that
> way, but that is a big change and not that wortwhile I think.
>
> Patch 7 is pretty much the same, it doesn't work right unless the
> container and device are already matched
>
If Alex won't have a different preference and with the typo fixed,
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
