| 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
On 04.12.2024 20:30, Alex Williamson wrote: > 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 Greg, is this testing, done by Alex, sufficient for you to take the series? > 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, > You have /sys/bus/mdev. Couldn't we create a directory here which holds the links to the devices in question? Then user space would simply have to switch from /sys/class/mdev_bus to /sys/bus/mdev/<new_dir>. > Alex > Heiner
On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: > On 04.12.2024 20:30, Alex Williamson wrote: > > 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 > > Greg, is this testing, done by Alex, sufficient for you to take the series? Were devices actually removed from the system and all worked well? > > 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, > > > > You have /sys/bus/mdev. Couldn't we create a directory here which holds > the links to the devices in question? Links to devices is not what class links are for, so why is this in /sys/class/ at all? > Then user space would simply have to switch from /sys/class/mdev_bus > to /sys/bus/mdev/<new_dir>. I think you are confusing what /sys/class/ is for here, if you have devices on a "bus" then they need to be in /sys/bus/ class has nothing to do with that. So can we just drop the /sys/class/ mistake entirely? thanks, greg k-h
On Fri, 6 Dec 2024 08:42:02 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: > > On 04.12.2024 20:30, Alex Williamson wrote: > > > 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 > > > > Greg, is this testing, done by Alex, sufficient for you to take the series? > > Were devices actually removed from the system and all worked well? Creating and removing virtual mdev devices as well as unloading and re-loading modules for the parent device providing the mdev support. > > > 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, > > > > > > > You have /sys/bus/mdev. Couldn't we create a directory here which holds > > the links to the devices in question? > > Links to devices is not what class links are for, so why is this in > /sys/class/ at all? Sorry, I'm confused. I look in /sys/class/block and /sys/class/net and I only see links to devices. /sys/class/mdev_bus has links to devices that have registered as supporting the mdev interface for creating devices in /sys/bus/mdev. We could link these devices somewhere else, but there are existing projects, userspace scripts, and documentation that references and relies on this layout. Whatever we decide it should have been 8 years ago is going to need yet another compatibility interface/link to avoid breaking userspace. > > Then user space would simply have to switch from /sys/class/mdev_bus > > to /sys/bus/mdev/<new_dir>. > > I think you are confusing what /sys/class/ is for here, if you have > devices on a "bus" then they need to be in /sys/bus/ class has nothing > to do with that. > > So can we just drop the /sys/class/ mistake entirely? Not without breaking userspace. /sys/bus/mdev is used for enumerating the virtual mdev devics that are created by devices supporting the mdev interface, where the latter are enumerated in /sys/class/mdev_bus. Thanks, Alex
On 06.12.2024 08:42, Greg Kroah-Hartman wrote: > On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: >> On 04.12.2024 20:30, Alex Williamson wrote: >>> 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 >> >> Greg, is this testing, done by Alex, sufficient for you to take the series? > > Were devices actually removed from the system and all worked well? > >>> 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, >>> >> >> You have /sys/bus/mdev. Couldn't we create a directory here which holds >> the links to the devices in question? > > Links to devices is not what class links are for, so why is this in > /sys/class/ at all? > Complementing what Alex just wrote: It's my understanding that it's in /sys/class only, because back then, when the mdev driver was written, class_compat seemed to be a convenient way to achieve what was required: providing a generic container in sysfs for arbitrary device links. So it wasn't an informed decision to use /sys/class. >> Then user space would simply have to switch from /sys/class/mdev_bus >> to /sys/bus/mdev/<new_dir>. > > I think you are confusing what /sys/class/ is for here, if you have > devices on a "bus" then they need to be in /sys/bus/ class has nothing > to do with that. > > So can we just drop the /sys/class/ mistake entirely? > > thanks, > > greg k-h
On Fri, Dec 06, 2024 at 09:37:33AM -0700, Alex Williamson wrote: > On Fri, 6 Dec 2024 08:42:02 +0100 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: > > > On 04.12.2024 20:30, Alex Williamson wrote: > > > > 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 > > > > > > Greg, is this testing, done by Alex, sufficient for you to take the series? > > > > Were devices actually removed from the system and all worked well? > > Creating and removing virtual mdev devices as well as unloading and > re-loading modules for the parent device providing the mdev support. > > > > > 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, > > > > > > > > > > You have /sys/bus/mdev. Couldn't we create a directory here which holds > > > the links to the devices in question? > > > > Links to devices is not what class links are for, so why is this in > > /sys/class/ at all? > > Sorry, I'm confused. I look in /sys/class/block and /sys/class/net and > I only see links to devices. Yes, they are linking to "class devices", i.e. things controlled by that class, NOT just a driver bound to a device on a bus. > /sys/class/mdev_bus has links to devices > that have registered as supporting the mdev interface for creating > devices in /sys/bus/mdev. And that's the issue here, drivers binding to a device on a bus are NOT class devices, they are bus devices (i.e. on the bus.) This used to be much "clearer" when we had "struct class_device" but we unified that a long time ago and now only have "struct device". In short, a bus device is the thing that is on a "bus" that a driver binds to (i.e. controls the hardware). A class device is the thing that a user talks to in a standardized way (input, block, tty, network, etc.) that is INDEPENDENT of the hardware bus the device happens to be attached to. > We could link these devices somewhere else, > but there are existing projects, userspace scripts, and documentation > that references and relies on this layout. Whatever we decide it > should have been 8 years ago is going to need yet another compatibility > interface/link to avoid breaking userspace. What you did here is say "mdev is both a standard interface to talk to userspace AND a standard bus", when really you should have made a mdev class if you really wanted one. Why not just do that now instead? Nothing should be preventing that and then your bus code can be the same too. > > > Then user space would simply have to switch from /sys/class/mdev_bus > > > to /sys/bus/mdev/<new_dir>. > > > > I think you are confusing what /sys/class/ is for here, if you have > > devices on a "bus" then they need to be in /sys/bus/ class has nothing > > to do with that. > > > > So can we just drop the /sys/class/ mistake entirely? > > Not without breaking userspace. /sys/bus/mdev is used for enumerating > the virtual mdev devics that are created by devices supporting the mdev > interface, where the latter are enumerated in /sys/class/mdev_bus. Great, how about creating a mdev_bus "struct class" then and doing this properly? That would be the correct solution overall, not this overloading of a symlink that causes confusion. thanks, greg k-h
On 07.12.2024 09:38, Greg Kroah-Hartman wrote: > On Fri, Dec 06, 2024 at 09:37:33AM -0700, Alex Williamson wrote: >> On Fri, 6 Dec 2024 08:42:02 +0100 >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: >> >>> On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: >>>> On 04.12.2024 20:30, Alex Williamson wrote: >>>>> 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 >>>> >>>> Greg, is this testing, done by Alex, sufficient for you to take the series? >>> >>> Were devices actually removed from the system and all worked well? >> >> Creating and removing virtual mdev devices as well as unloading and >> re-loading modules for the parent device providing the mdev support. >> >>>>> 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, >>>>> >>>> >>>> You have /sys/bus/mdev. Couldn't we create a directory here which holds >>>> the links to the devices in question? >>> >>> Links to devices is not what class links are for, so why is this in >>> /sys/class/ at all? >> >> Sorry, I'm confused. I look in /sys/class/block and /sys/class/net and >> I only see links to devices. > > Yes, they are linking to "class devices", i.e. things controlled by that > class, NOT just a driver bound to a device on a bus. > >> /sys/class/mdev_bus has links to devices >> that have registered as supporting the mdev interface for creating >> devices in /sys/bus/mdev. > > And that's the issue here, drivers binding to a device on a bus are NOT > class devices, they are bus devices (i.e. on the bus.) > > This used to be much "clearer" when we had "struct class_device" but we > unified that a long time ago and now only have "struct device". > > In short, a bus device is the thing that is on a "bus" that a driver > binds to (i.e. controls the hardware). A class device is the thing that > a user talks to in a standardized way (input, block, tty, network, etc.) > that is INDEPENDENT of the hardware bus the device happens to be > attached to. > >> We could link these devices somewhere else, >> but there are existing projects, userspace scripts, and documentation >> that references and relies on this layout. Whatever we decide it >> should have been 8 years ago is going to need yet another compatibility >> interface/link to avoid breaking userspace. > > What you did here is say "mdev is both a standard interface to talk to > userspace AND a standard bus", when really you should have made a mdev > class if you really wanted one. Why not just do that now instead? > Nothing should be preventing that and then your bus code can be the same > too. > >>>> Then user space would simply have to switch from /sys/class/mdev_bus >>>> to /sys/bus/mdev/<new_dir>. >>> >>> I think you are confusing what /sys/class/ is for here, if you have >>> devices on a "bus" then they need to be in /sys/bus/ class has nothing >>> to do with that. >>> >>> So can we just drop the /sys/class/ mistake entirely? >> >> Not without breaking userspace. /sys/bus/mdev is used for enumerating >> the virtual mdev devics that are created by devices supporting the mdev >> interface, where the latter are enumerated in /sys/class/mdev_bus. > > Great, how about creating a mdev_bus "struct class" then and doing this > properly? That would be the correct solution overall, not this > overloading of a symlink that causes confusion. > Issue with this approach is that these "mdev parent" devices are partially class devices belonging to other classes. See for example mtty_dev_init(), there the device passed to us belongs to class mtty. Here in mdev two types of devices are handled: 1. The "mdev parent" devices, which are linked to /sys/class/mdev_bus. These devices can be class devices of other classes. 2. The mdev devices, these are normal bus devices residing under /sys/bus/mdev. > thanks, > > greg k-h
On Sat, Dec 07, 2024 at 11:06:15AM +0100, Heiner Kallweit wrote: > On 07.12.2024 09:38, Greg Kroah-Hartman wrote: > > On Fri, Dec 06, 2024 at 09:37:33AM -0700, Alex Williamson wrote: > >> On Fri, 6 Dec 2024 08:42:02 +0100 > >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > >> > >>> On Fri, Dec 06, 2024 at 08:35:47AM +0100, Heiner Kallweit wrote: > >>>> On 04.12.2024 20:30, Alex Williamson wrote: > >>>>> 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 > >>>> > >>>> Greg, is this testing, done by Alex, sufficient for you to take the series? > >>> > >>> Were devices actually removed from the system and all worked well? > >> > >> Creating and removing virtual mdev devices as well as unloading and > >> re-loading modules for the parent device providing the mdev support. > >> > >>>>> 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, > >>>>> > >>>> > >>>> You have /sys/bus/mdev. Couldn't we create a directory here which holds > >>>> the links to the devices in question? > >>> > >>> Links to devices is not what class links are for, so why is this in > >>> /sys/class/ at all? > >> > >> Sorry, I'm confused. I look in /sys/class/block and /sys/class/net and > >> I only see links to devices. > > > > Yes, they are linking to "class devices", i.e. things controlled by that > > class, NOT just a driver bound to a device on a bus. > > > >> /sys/class/mdev_bus has links to devices > >> that have registered as supporting the mdev interface for creating > >> devices in /sys/bus/mdev. > > > > And that's the issue here, drivers binding to a device on a bus are NOT > > class devices, they are bus devices (i.e. on the bus.) > > > > This used to be much "clearer" when we had "struct class_device" but we > > unified that a long time ago and now only have "struct device". > > > > In short, a bus device is the thing that is on a "bus" that a driver > > binds to (i.e. controls the hardware). A class device is the thing that > > a user talks to in a standardized way (input, block, tty, network, etc.) > > that is INDEPENDENT of the hardware bus the device happens to be > > attached to. > > > >> We could link these devices somewhere else, > >> but there are existing projects, userspace scripts, and documentation > >> that references and relies on this layout. Whatever we decide it > >> should have been 8 years ago is going to need yet another compatibility > >> interface/link to avoid breaking userspace. > > > > What you did here is say "mdev is both a standard interface to talk to > > userspace AND a standard bus", when really you should have made a mdev > > class if you really wanted one. Why not just do that now instead? > > Nothing should be preventing that and then your bus code can be the same > > too. > > > >>>> Then user space would simply have to switch from /sys/class/mdev_bus > >>>> to /sys/bus/mdev/<new_dir>. > >>> > >>> I think you are confusing what /sys/class/ is for here, if you have > >>> devices on a "bus" then they need to be in /sys/bus/ class has nothing > >>> to do with that. > >>> > >>> So can we just drop the /sys/class/ mistake entirely? > >> > >> Not without breaking userspace. /sys/bus/mdev is used for enumerating > >> the virtual mdev devics that are created by devices supporting the mdev > >> interface, where the latter are enumerated in /sys/class/mdev_bus. > > > > Great, how about creating a mdev_bus "struct class" then and doing this > > properly? That would be the correct solution overall, not this > > overloading of a symlink that causes confusion. > > > Issue with this approach is that these "mdev parent" devices are partially > class devices belonging to other classes. See for example mtty_dev_init(), > there the device passed to us belongs to class mtty. Then fix that please. And there's no issue with multiple "class devices" being associated with one "bus device", as long as they aren't named the same, right? And that's a sample, surely it's not "real" code there :) > Here in mdev two types of devices are handled: > 1. The "mdev parent" devices, which are linked to /sys/class/mdev_bus. > These devices can be class devices of other classes. But that's not how a class works. Again, a class is a bunch of devices that all interact with userspace with the same user/kernel api. You can put them whereever you want in the tree but a "class device" does not have children in the same class, as classes are "flat". Only the real device tree (i.e. bus devices) have a heirachy. > 2. The mdev devices, these are normal bus devices residing under /sys/bus/mdev. That's fine. Again, I think you all need to sort this out properly. Classes are for user/kernel apis. Devices are for interactions on a hardware bus. Keep them separate as they are logically totally different things (note, you can have a class that talks on a bus, like i2c devices, but let's not go there...) thanks, greg k-h
On Sat, Dec 07, 2024 at 11:06:15AM +0100, Heiner Kallweit wrote: [..a lot of full quotes.. Guys, can you please fix your email to actually be readable. Having to delete dozens of pages of crap is really infuriating] > Issue with this approach is that these "mdev parent" devices are partially > class devices belonging to other classes. See for example mtty_dev_init(), > there the device passed to us belongs to class mtty. The samples don't matter and can be fixed any time. Or even better deleted. The real question is if the i915, ccw and ap devices are class devices. From a quick unscientific grep they appear not to, but we'd need to double check that. (btw, drm_class_device_register in drm is entirely unused)
On Wed, 11 Dec 2024 20:42:06 -0800 Christoph Hellwig <hch@infradead.org> wrote: > On Sat, Dec 07, 2024 at 11:06:15AM +0100, Heiner Kallweit wrote: > > Issue with this approach is that these "mdev parent" devices are partially > > class devices belonging to other classes. See for example mtty_dev_init(), > > there the device passed to us belongs to class mtty. > > The samples don't matter and can be fixed any time. Or even better > deleted. There is value to these. In particular mtty exposes a dummy PCI serial device with two different flavors (single/dual port) that's useful for not only testing the mdev lifecycle behavior, but also implements the vfio migration API. Otherwise testing any of this requires specific hardware. I'd agree that breaking userspace API for a sample device is less of a blocking issue, but then we have these... > The real question is if the i915, ccw and ap devices are > class devices. From a quick unscientific grep they appear not to, > but we'd need to double check that. And I'd expect that these are all linking bus devices under the mdev_bus class. I understand the issue now, that from the start we should have been creating class devices, but it seems that resolving that is going to introduce a level of indirection between the new class device and the bus device which is likely going to have dependencies in the existing userspace tools. We'll need to work through the primary ones and figure out contingencies for the others to avoid breaking userspace. The "just remove it anyway" stance seems to be in conflict with the "don't break userspace" policy and I don't think we can instantly fix this. Thanks, Alex
On Thu, Dec 12, 2024 at 11:12:00AM -0700, Alex Williamson wrote: > On Wed, 11 Dec 2024 20:42:06 -0800 > Christoph Hellwig <hch@infradead.org> wrote: > > On Sat, Dec 07, 2024 at 11:06:15AM +0100, Heiner Kallweit wrote: > > > Issue with this approach is that these "mdev parent" devices are partially > > > class devices belonging to other classes. See for example mtty_dev_init(), > > > there the device passed to us belongs to class mtty. > > > > The samples don't matter and can be fixed any time. Or even better > > deleted. > > There is value to these. In particular mtty exposes a dummy PCI serial > device with two different flavors (single/dual port) that's useful for > not only testing the mdev lifecycle behavior, but also implements the > vfio migration API. Otherwise testing any of this requires specific > hardware. I'd agree that breaking userspace API for a sample device is > less of a blocking issue, but then we have these... > > > The real question is if the i915, ccw and ap devices are > > class devices. From a quick unscientific grep they appear not to, > > but we'd need to double check that. > > And I'd expect that these are all linking bus devices under the > mdev_bus class. I understand the issue now, that from the start we > should have been creating class devices, but it seems that resolving > that is going to introduce a level of indirection between the new class > device and the bus device which is likely going to have dependencies in > the existing userspace tools. We'll need to work through the primary > ones and figure out contingencies for the others to avoid breaking > userspace. The "just remove it anyway" stance seems to be in conflict > with the "don't break userspace" policy and I don't think we can > instantly fix this. Thanks, But samples are not "real" and are not actually used. If they were, then shouldn't they be in the real part of the kernel? So what are we "breaking" here? confused, greg k-h
On Thu, 12 Dec 2024 19:21:34 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > But samples are not "real" and are not actually used. If they were, > then shouldn't they be in the real part of the kernel? > > So what are we "breaking" here? We'd be breaking the management of and use of Intel vGPU support on older integrated graphics (GVT-g/KVMgt), as well as the ccw and ap drivers on s390, which are mdev devices that expose disk and crypto interfaces respectively, aiui. These are drivers that exist in the real part of the kernel[1][2][3]. Thanks, Alex [1] drivers/gpu/drm/i915/gvt/kvmgt.c [2] drivers/s390/cio/vfio_ccw* [3] drivers/s390/crypto/vfio_ap*
On Thu, Dec 12, 2024 at 11:12:00AM -0700, Alex Williamson wrote: > userspace. The "just remove it anyway" stance seems to be in conflict > with the "don't break userspace" policy and I don't think we can > instantly fix this. Thanks, The just remove was about the sample, which are explicitly samples and not something that is part of the kernel ABI gurantee.
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(-)