On Fri, 2 Sep 2022 03:51:01 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> > 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,
I don't get it, if a group is detaching itself from a container, why
isn't it vfio_group_detach_container(). Given our naming scheme,
vfio_container_detach_group() absolutely implies the args Kevin
suggests, even if they're redundant. vfio_foo_* functions should
operate on a a vfio_foo object. Thanks,
Alex
