On Wed, 4 Dec 2024 19:16:03 +0100
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Dec 04, 2024 at 06:01:36PM +0100, Heiner Kallweit wrote:
> > On 04.12.2024 10:32, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 03, 2024 at 09:11:47PM +0100, Heiner Kallweit wrote:
> > >> vfio/mdev is the last user of class_compat, and it doesn't use it for
> > >> the intended purpose. See kdoc of class_compat_register():
> > >> Compatibility class are meant as a temporary user-space compatibility
> > >> workaround when converting a family of class devices to a bus devices.
> > >
> > > True, so waht is mdev doing here?
> > >
> > >> In addition it uses only a part of the class_compat functionality.
> > >> So inline the needed functionality, and afterwards all class_compat
> > >> code can be removed.
> > >>
> > >> No functional change intended. Compile-tested only.
> > >>
> > >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > >> ---
> > >> drivers/vfio/mdev/mdev_core.c | 12 ++++++------
> > >> 1 file changed, 6 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > >> index ed4737de4..a22c49804 100644
> > >> --- a/drivers/vfio/mdev/mdev_core.c
> > >> +++ b/drivers/vfio/mdev/mdev_core.c
> > >> @@ -18,7 +18,7 @@
> > >> #define DRIVER_AUTHOR "NVIDIA Corporation"
> > >> #define DRIVER_DESC "Mediated device Core Driver"
> > >>
> > >> -static struct class_compat *mdev_bus_compat_class;
> > >> +static struct kobject *mdev_bus_kobj;
> > >
> > >
> > >
> > >>
> > >> static LIST_HEAD(mdev_list);
> > >> static DEFINE_MUTEX(mdev_list_lock);
> > >> @@ -76,7 +76,7 @@ int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
> > >> if (ret)
> > >> return ret;
> > >>
> > >> - ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL);
> > >> + ret = sysfs_create_link(mdev_bus_kobj, &dev->kobj, dev_name(dev));
> > >
> > > This feels really wrong, why create a link to a random kobject? Who is
> > > using this kobject link?
> > >
> > >> if (ret)
> > >> dev_warn(dev, "Failed to create compatibility class link\n");
> > >>
> > >> @@ -98,7 +98,7 @@ void mdev_unregister_parent(struct mdev_parent *parent)
> > >> dev_info(parent->dev, "MDEV: Unregistering\n");
> > >>
> > >> down_write(&parent->unreg_sem);
> > >> - class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL);
> > >> + sysfs_remove_link(mdev_bus_kobj, dev_name(parent->dev));
> > >> device_for_each_child(parent->dev, NULL, mdev_device_remove_cb);
> > >> parent_remove_sysfs_files(parent);
> > >> up_write(&parent->unreg_sem);
> > >> @@ -251,8 +251,8 @@ static int __init mdev_init(void)
> > >> if (ret)
> > >> return ret;
> > >>
> > >> - mdev_bus_compat_class = class_compat_register("mdev_bus");
> > >> - if (!mdev_bus_compat_class) {
> > >> + mdev_bus_kobj = class_pseudo_register("mdev_bus");
> > >
> > > But this isn't a class, so let's not fake it please. Let's fix this
> > > properly, odds are all of this code can just be removed entirely, right?
> > >
> >
> > After I removed class_compat from i2c core, I asked Alex basically the
> > same thing: whether class_compat support can be removed from vfio/mdev too.
> >
> > His reply:
> > I'm afraid we have active userspace tools dependent on
> > /sys/class/mdev_bus currently, libvirt for one. We link mdev parent
> > devices here and I believe it's the only way for userspace to find
> > those parent devices registered for creating mdev devices. If there's a
> > desire to remove class_compat, we might need to add some mdev
> > infrastructure to register the class ourselves to maintain the parent
> > links.
> >
> >
> > It's my understanding that /sys/class/mdev_bus has nothing in common
> > with an actual class, it's just a container for devices which at least
> > partially belong to other classes. And there's user space tools depending
> > on this structure.
>
> That's odd, when this was added, why was it added this way? The
> class_compat stuff is for when classes move around, yet this was always
> done in this way?
>
> And what tools use this symlink today that can't be updated?
It's been this way for 8 years, so it's hard to remember exact
motivation for using this mechanism, whether we just didn't look hard
enough at the class_compat or it didn't pass by the right set of eyes.
Yes, it's always been this way for the mdev_bus class.
The intention here is much like other classes, that we have a node in
sysfs where we can find devices that provide a common feature, in this
case providing support for creating and managing vfio mediated devices
(mdevs). The perhaps unique part of this use case is that these devices
aren't strictly mdev providers, they might also belong to another class
and the mdev aspect of their behavior might be dynamically added after
the device itself is added.
I've done some testing with this series and it does indeed seem to
maintain compatibility with existing userspace tools, mdevctl and
libvirt. We can update these tools, but then we get into the breaking
userspace and deprecation period questions, where we may further delay
removal of class_compat. Also if we were to re-implement this, is there
a better mechanism than proposed here within the class hierarchy, or
would you recommend a non-class approach? Thanks,
Alex
