| Message ID | 0a14a4df-fbb5-4613-837f-f8025dc73380@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | driver core: class: remove class_compat code | expand |
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@gmail.com> > --- > 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? thanks, greg k-h
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@gmail.com> >> --- >> 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. > thanks, > > greg k-h
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@gmail.com> > >> --- > >> 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? thanks, greg k-h
On Wed, 4 Dec 2024 19:16:03 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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@gmail.com> > > >> --- > > >> 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
On 04.12.2024 19:16, Greg Kroah-Hartman 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@gmail.com> >>>> --- >>>> 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? > I can only answer the when: in 2016, with the initial version of vfio/mdev Kirti is the author, maybe she can provide some background. > And what tools use this symlink today that can't be updated? > According to Alex: libvirt, not clear whether there are more users of the current sysfs structure I'm just the messenger here and can't comment on whether/how/who updating user space. > thanks, > > greg k-h
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)); 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"); + if (!mdev_bus_kobj) { bus_unregister(&mdev_bus_type); return -ENOMEM; } @@ -262,7 +262,7 @@ static int __init mdev_init(void) static void __exit mdev_exit(void) { - class_compat_unregister(mdev_bus_compat_class); + kobject_put(mdev_bus_kobj); bus_unregister(&mdev_bus_type); }
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. 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@gmail.com> --- drivers/vfio/mdev/mdev_core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)