[PATCH 0/3] driver core: class: remove class_compat code

[PATCH 0/3] driver core: class: remove class_compat code

[Heiner Kallweit]

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.

Heiner Kallweit (3):
  driver core: class: add class_pseudo_register
  vfio/mdev: inline needed class_compat functionality
  driver core: class: remove class_compat code

 drivers/base/class.c          | 89 ++++-------------------------------
 drivers/vfio/mdev/mdev_core.c | 12 ++---
 include/linux/device/class.h  |  8 +---
 3 files changed, 15 insertions(+), 94 deletions(-)

-- 
2.47.1

[PATCH 1/3] driver core: class: add class_pseudo_register

[Heiner Kallweit]

In preparation of removing class_compat support, add a helper for
creating a pseudo class in sysfs. This way we can keep class_kset
private to driver core. This helper will be used by vfio/mdev,
replacing the call to class_compat_create().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/base/class.c         | 14 ++++++++++++++
 include/linux/device/class.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index 582b5a02a..f812236e2 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -578,6 +578,20 @@ struct class_compat *class_compat_register(const char *name)
 }
 EXPORT_SYMBOL_GPL(class_compat_register);
 
+/**
+ * class_pseudo_register - create a pseudo class entry in sysfs
+ * @name: the name of the child
+ *
+ * Helper for creating a pseudo class in sysfs, keeps class_kset private
+ *
+ * Returns: the created kobject
+ */
+struct kobject *class_pseudo_register(const char *name)
+{
+	return kobject_create_and_add(name, &class_kset->kobj);
+}
+EXPORT_SYMBOL_GPL(class_pseudo_register);
+
 /**
  * class_compat_unregister - unregister a compatibility class
  * @cls: the class to unregister
diff --git a/include/linux/device/class.h b/include/linux/device/class.h
index 518c9c83d..8b6e890c7 100644
--- a/include/linux/device/class.h
+++ b/include/linux/device/class.h
@@ -86,6 +86,7 @@ int class_compat_create_link(struct class_compat *cls, struct device *dev,
 			     struct device *device_link);
 void class_compat_remove_link(struct class_compat *cls, struct device *dev,
 			      struct device *device_link);
+struct kobject *class_pseudo_register(const char *name);
 
 void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
 			 const struct device *start, const struct device_type *type);
-- 
2.47.1

[PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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(-)

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);
 }
 
-- 
2.47.1

[PATCH 3/3] driver core: class: remove class_compat code

[Heiner Kallweit]

vfio/mdev as last user of class_compat has inlined the needed
functionality. So all class_compat code can be removed now.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/base/class.c         | 87 ------------------------------------
 include/linux/device/class.h |  7 ---
 2 files changed, 94 deletions(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index f812236e2..525512d05 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -551,33 +551,6 @@ ssize_t show_class_attr_string(const struct class *class,
 
 EXPORT_SYMBOL_GPL(show_class_attr_string);
 
-struct class_compat {
-	struct kobject *kobj;
-};
-
-/**
- * class_compat_register - register a compatibility class
- * @name: the name of the class
- *
- * Compatibility class are meant as a temporary user-space compatibility
- * workaround when converting a family of class devices to a bus devices.
- */
-struct class_compat *class_compat_register(const char *name)
-{
-	struct class_compat *cls;
-
-	cls = kmalloc(sizeof(struct class_compat), GFP_KERNEL);
-	if (!cls)
-		return NULL;
-	cls->kobj = kobject_create_and_add(name, &class_kset->kobj);
-	if (!cls->kobj) {
-		kfree(cls);
-		return NULL;
-	}
-	return cls;
-}
-EXPORT_SYMBOL_GPL(class_compat_register);
-
 /**
  * class_pseudo_register - create a pseudo class entry in sysfs
  * @name: the name of the child
@@ -592,66 +565,6 @@ struct kobject *class_pseudo_register(const char *name)
 }
 EXPORT_SYMBOL_GPL(class_pseudo_register);
 
-/**
- * class_compat_unregister - unregister a compatibility class
- * @cls: the class to unregister
- */
-void class_compat_unregister(struct class_compat *cls)
-{
-	kobject_put(cls->kobj);
-	kfree(cls);
-}
-EXPORT_SYMBOL_GPL(class_compat_unregister);
-
-/**
- * class_compat_create_link - create a compatibility class device link to
- *			      a bus device
- * @cls: the compatibility class
- * @dev: the target bus device
- * @device_link: an optional device to which a "device" link should be created
- */
-int class_compat_create_link(struct class_compat *cls, struct device *dev,
-			     struct device *device_link)
-{
-	int error;
-
-	error = sysfs_create_link(cls->kobj, &dev->kobj, dev_name(dev));
-	if (error)
-		return error;
-
-	/*
-	 * Optionally add a "device" link (typically to the parent), as a
-	 * class device would have one and we want to provide as much
-	 * backwards compatibility as possible.
-	 */
-	if (device_link) {
-		error = sysfs_create_link(&dev->kobj, &device_link->kobj,
-					  "device");
-		if (error)
-			sysfs_remove_link(cls->kobj, dev_name(dev));
-	}
-
-	return error;
-}
-EXPORT_SYMBOL_GPL(class_compat_create_link);
-
-/**
- * class_compat_remove_link - remove a compatibility class device link to
- *			      a bus device
- * @cls: the compatibility class
- * @dev: the target bus device
- * @device_link: an optional device to which a "device" link was previously
- * 		 created
- */
-void class_compat_remove_link(struct class_compat *cls, struct device *dev,
-			      struct device *device_link)
-{
-	if (device_link)
-		sysfs_remove_link(&dev->kobj, "device");
-	sysfs_remove_link(cls->kobj, dev_name(dev));
-}
-EXPORT_SYMBOL_GPL(class_compat_remove_link);
-
 /**
  * class_is_registered - determine if at this moment in time, a class is
  *			 registered in the driver core or not.
diff --git a/include/linux/device/class.h b/include/linux/device/class.h
index 8b6e890c7..85b036d0a 100644
--- a/include/linux/device/class.h
+++ b/include/linux/device/class.h
@@ -79,13 +79,6 @@ int __must_check class_register(const struct class *class);
 void class_unregister(const struct class *class);
 bool class_is_registered(const struct class *class);
 
-struct class_compat;
-struct class_compat *class_compat_register(const char *name);
-void class_compat_unregister(struct class_compat *cls);
-int class_compat_create_link(struct class_compat *cls, struct device *dev,
-			     struct device *device_link);
-void class_compat_remove_link(struct class_compat *cls, struct device *dev,
-			      struct device *device_link);
 struct kobject *class_pseudo_register(const char *name);
 
 void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
-- 
2.47.1

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 1/3] driver core: class: add class_pseudo_register

[Greg Kroah-Hartman]

On Tue, Dec 03, 2024 at 09:10:05PM +0100, Heiner Kallweit wrote:
> In preparation of removing class_compat support, add a helper for
> creating a pseudo class in sysfs. This way we can keep class_kset
> private to driver core. This helper will be used by vfio/mdev,
> replacing the call to class_compat_create().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/base/class.c         | 14 ++++++++++++++
>  include/linux/device/class.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index 582b5a02a..f812236e2 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -578,6 +578,20 @@ struct class_compat *class_compat_register(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(class_compat_register);
>  
> +/**
> + * class_pseudo_register - create a pseudo class entry in sysfs
> + * @name: the name of the child
> + *
> + * Helper for creating a pseudo class in sysfs, keeps class_kset private
> + *
> + * Returns: the created kobject
> + */
> +struct kobject *class_pseudo_register(const char *name)
> +{
> +	return kobject_create_and_add(name, &class_kset->kobj);
> +}
> +EXPORT_SYMBOL_GPL(class_pseudo_register);

I see the goal here, but let's not continue on and create fake kobjects
in places where there should NOT be any kobjects.  Also, you might get
in trouble when removing this kobject as it thinks it is a 'struct
class' but yet it isn't, right?  Did you test that?

thanks,

greg k-h

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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

Re: [PATCH 1/3] driver core: class: add class_pseudo_register

[Heiner Kallweit]

On 04.12.2024 10:33, Greg Kroah-Hartman wrote:
> On Tue, Dec 03, 2024 at 09:10:05PM +0100, Heiner Kallweit wrote:
>> In preparation of removing class_compat support, add a helper for
>> creating a pseudo class in sysfs. This way we can keep class_kset
>> private to driver core. This helper will be used by vfio/mdev,
>> replacing the call to class_compat_create().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/base/class.c         | 14 ++++++++++++++
>>  include/linux/device/class.h |  1 +
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/base/class.c b/drivers/base/class.c
>> index 582b5a02a..f812236e2 100644
>> --- a/drivers/base/class.c
>> +++ b/drivers/base/class.c
>> @@ -578,6 +578,20 @@ struct class_compat *class_compat_register(const char *name)
>>  }
>>  EXPORT_SYMBOL_GPL(class_compat_register);
>>  
>> +/**
>> + * class_pseudo_register - create a pseudo class entry in sysfs
>> + * @name: the name of the child
>> + *
>> + * Helper for creating a pseudo class in sysfs, keeps class_kset private
>> + *
>> + * Returns: the created kobject
>> + */
>> +struct kobject *class_pseudo_register(const char *name)
>> +{
>> +	return kobject_create_and_add(name, &class_kset->kobj);
>> +}
>> +EXPORT_SYMBOL_GPL(class_pseudo_register);
> 
> I see the goal here, but let's not continue on and create fake kobjects
> in places where there should NOT be any kobjects.  Also, you might get
> in trouble when removing this kobject as it thinks it is a 'struct
> class' but yet it isn't, right?  Did you test that?
> 

It's removed using kobject_put(), same as what class_compat_unregister() does.
I only compile-tested the changes.

> thanks,
> 
> greg k-h

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 1/3] driver core: class: add class_pseudo_register

[Greg Kroah-Hartman]

On Wed, Dec 04, 2024 at 06:04:32PM +0100, Heiner Kallweit wrote:
> On 04.12.2024 10:33, Greg Kroah-Hartman wrote:
> > On Tue, Dec 03, 2024 at 09:10:05PM +0100, Heiner Kallweit wrote:
> >> In preparation of removing class_compat support, add a helper for
> >> creating a pseudo class in sysfs. This way we can keep class_kset
> >> private to driver core. This helper will be used by vfio/mdev,
> >> replacing the call to class_compat_create().
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/base/class.c         | 14 ++++++++++++++
> >>  include/linux/device/class.h |  1 +
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/base/class.c b/drivers/base/class.c
> >> index 582b5a02a..f812236e2 100644
> >> --- a/drivers/base/class.c
> >> +++ b/drivers/base/class.c
> >> @@ -578,6 +578,20 @@ struct class_compat *class_compat_register(const char *name)
> >>  }
> >>  EXPORT_SYMBOL_GPL(class_compat_register);
> >>  
> >> +/**
> >> + * class_pseudo_register - create a pseudo class entry in sysfs
> >> + * @name: the name of the child
> >> + *
> >> + * Helper for creating a pseudo class in sysfs, keeps class_kset private
> >> + *
> >> + * Returns: the created kobject
> >> + */
> >> +struct kobject *class_pseudo_register(const char *name)
> >> +{
> >> +	return kobject_create_and_add(name, &class_kset->kobj);
> >> +}
> >> +EXPORT_SYMBOL_GPL(class_pseudo_register);
> > 
> > I see the goal here, but let's not continue on and create fake kobjects
> > in places where there should NOT be any kobjects.  Also, you might get
> > in trouble when removing this kobject as it thinks it is a 'struct
> > class' but yet it isn't, right?  Did you test that?
> > 
> 
> It's removed using kobject_put(), same as what class_compat_unregister() does.
> I only compile-tested the changes.

I would not be able to take these unless someone actually runs them as
the kobject removal here might be getting confused a bit.

thanks,

greg k-h

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Alex Williamson]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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

Re: [PATCH 1/3] driver core: class: add class_pseudo_register

[Heiner Kallweit]

On 04.12.2024 19:17, Greg Kroah-Hartman wrote:
> On Wed, Dec 04, 2024 at 06:04:32PM +0100, Heiner Kallweit wrote:
>> On 04.12.2024 10:33, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 03, 2024 at 09:10:05PM +0100, Heiner Kallweit wrote:
>>>> In preparation of removing class_compat support, add a helper for
>>>> creating a pseudo class in sysfs. This way we can keep class_kset
>>>> private to driver core. This helper will be used by vfio/mdev,
>>>> replacing the call to class_compat_create().
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/base/class.c         | 14 ++++++++++++++
>>>>  include/linux/device/class.h |  1 +
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/base/class.c b/drivers/base/class.c
>>>> index 582b5a02a..f812236e2 100644
>>>> --- a/drivers/base/class.c
>>>> +++ b/drivers/base/class.c
>>>> @@ -578,6 +578,20 @@ struct class_compat *class_compat_register(const char *name)
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(class_compat_register);
>>>>  
>>>> +/**
>>>> + * class_pseudo_register - create a pseudo class entry in sysfs
>>>> + * @name: the name of the child
>>>> + *
>>>> + * Helper for creating a pseudo class in sysfs, keeps class_kset private
>>>> + *
>>>> + * Returns: the created kobject
>>>> + */
>>>> +struct kobject *class_pseudo_register(const char *name)
>>>> +{
>>>> +	return kobject_create_and_add(name, &class_kset->kobj);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(class_pseudo_register);
>>>
>>> I see the goal here, but let's not continue on and create fake kobjects
>>> in places where there should NOT be any kobjects.  Also, you might get
>>> in trouble when removing this kobject as it thinks it is a 'struct
>>> class' but yet it isn't, right?  Did you test that?
>>>
>>
>> It's removed using kobject_put(), same as what class_compat_unregister() does.
>> I only compile-tested the changes.
> 
> I would not be able to take these unless someone actually runs them as
> the kobject removal here might be getting confused a bit.
> 
Understood. Maybe Alex/Kirti can test.

> thanks,
> 
> greg k-h

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Alex Williamson]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Heiner Kallweit]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 0/3] driver core: class: remove class_compat code

[Christoph Hellwig]

On Tue, Dec 03, 2024 at 09:08:55PM +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.
> 
> 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.

Is there any reason it can't just be removed entirely?

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Christoph Hellwig]

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)

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Alex Williamson]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Alex Williamson]

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*

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Christoph Hellwig]

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. 

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Greg Kroah-Hartman]

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.
> 
> 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.

Did this ever get tested?

> 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;

If you want to resubmit this, after testing, you need some BIG comments
here as to what you are doing and why, and that no one else should EVER
do this again so they don't cut/paste from this code to create the same
mess.

thanks,

greg k-h

Re: [PATCH 2/3] vfio/mdev: inline needed class_compat functionality

[Alex Williamson]

On Fri, 10 Jan 2025 15:35:30 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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.
> > 
> > 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.  
> 
> Did this ever get tested?

I wasn't sure where we stand between this and

https://lore.kernel.org/all/db49131d-fd79-4f23-93f2-0ab541a345fa@gmail.com/

I've tested them both separately.

Tested-by: Alex Williamson <alex.williamson@redhat.com>
Acked-by: Alex Williamson <alex.williamson@redhat.com>
 
> > 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;  
> 
> If you want to resubmit this, after testing, you need some BIG comments
> here as to what you are doing and why, and that no one else should EVER
> do this again so they don't cut/paste from this code to create the same
> mess.

WFM.  Thanks,

Alex