Re: [PATCH 10/18] vfio/mdev: Remove duplicate storage of parent in mdev_device

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Mar 26, 2021 at 03:53:10AM +0000, Tian, Kevin wrote:

> > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref)
> >  /* Caller must hold parent unreg_sem read or write lock */
> >  static void mdev_device_remove_common(struct mdev_device *mdev)
> >  {
> > -	struct mdev_parent *parent;
> > +	struct mdev_parent *parent = mdev->type->parent;
> 
> What about having a wrapper here, like mdev_parent_dev? For
> readability it's not necessary to show that the parent is indirectly
> retrieved through mdev_type.

I think that is too much wrappering, we only have three usages of the
mdev->type->parent sequence and two are already single line inlines.

> >  	int ret;
> > 
> >  	mdev_remove_sysfs_files(mdev);
> >  	device_del(&mdev->dev);
> > -	parent = mdev->parent;
> >  	lockdep_assert_held(&parent->unreg_sem);
> >  	ret = parent->ops->remove(mdev);
> >  	if (ret)
> > @@ -212,7 +211,7 @@ static void mdev_device_release(struct device *dev)
> >  	struct mdev_device *mdev = to_mdev_device(dev);
> > 
> >  	/* Pairs with the get in mdev_device_create() */
> > -	mdev_put_parent(mdev->parent);
> > +	kobject_put(&mdev->type->kobj);
> 
> Maybe keep mdev_get/put_parent and change them to accept "struct
> mdev_device *" parameter like other places.

We do keep mdev_get/put_parent() they manipulate the refcount inside
the parent and this is only done in side the type logic now.

This is get/put of the refcount on the type, and this is the only
place that does it, so I don't see a reason for more wrappers.

Jason