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
