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