Re: [RFC] driver core: add a virtual bus for use when a simple device/bus is needed

Danilo Krummrich
2025-02-03T21:13:15Z

On Mon, Feb 03, 2025 at 12:25:23PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Feb 03, 2025 at 12:01:04PM +0100, Danilo Krummrich wrote:
> > On Mon, Feb 03, 2025 at 10:39:58AM +0100, Greg Kroah-Hartman wrote:
> > > From 4c7aa0f9f0f7d25c962b70a11bad48d418b9490a Mon Sep 17 00:00:00 2001
> > > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Date: Fri, 31 Jan 2025 15:01:32 +0100
> > > Subject: [PATCH] driver core: add a virtual bus for use when a simple
> > > device/bus is needed
> > >
> > > Many drivers abuse the platform driver/bus system as it provides a
> > > simple way to create and bind a device to a driver-specific set of
> > > probe/release functions. Instead of doing that, and wasting all of the
> > > memory associated with a platform device, here is a "virtual" bus that
> > > can be used instead.
> > >
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > I think it turned out pretty nice combining the driver and device creation for
> > convenience.
> >
> > But I think we may still need the option to create multiple devices for the same
> > driver, as mentioned by Sima.
>
> That will work just fine, the api will allow that, just give each device
> a unique name and you are good to go.
>
> > @Sima: I wonder if the number of devices could just be an argument?
>
> Then the virtual bus logic will have to create some sort of number/name
> system and I don't want to do that. It's a "first caller gets the name"
> type thing here. You can easily in a driver do this:
>
> my_dev_1 = virtual_device_create(&my_virt_ops, "my_dev_1");
> my_dev_2 = virtual_device_create(&my_virt_ops, "my_dev_2");
> my_dev_3 = virtual_device_create(&my_virt_ops, "my_dev_3");
> ...
>
> You share the same callbacks, and that's all you really care about. If
> you want to hang sysfs files off of these things, I can make them take a
> device_groups pointer as well, but let's keep it simple first.

Sure, that works perfectly. Just thought, we might not want to also create a new
struct driver for each device.

>
> > > +/*
> > > + * Internal rapper structure so we can hold the memory
> >
> > I guess having an internal "rapper" does make the interface even cooler! :-)
>
> Hah, I totally missed that. Language is fun...
>
> > > +static void virtual_device_release(struct device *dev)
> > > +{
> > > + struct virtual_object *virt_obj = to_virtual_object(dev);
> > > + struct device_driver *drv = &virt_obj->driver;
> > > +
> > > + /*
> > > + * Now that the device is going away, it has been unbound from the
> > > + * driver we created for it, so it is safe to unregister the driver from
> > > + * the system.
> > > + */
> > > + driver_unregister(drv);
> >
> > This is probably becoming non-trivial if we allow multiple devices to be created
> > for the driver.
>
> Nope, see above, the driver is created dynamically per device created,
> but that has NOTHING to do with the caller of this api, this is all
> internal housekeeping.
>
> You will note that the caller knows nothing about a driver or anything
> like that, all it does is provide some callbacks.

Should have said in case we allow multiple devices per driver, but as long as we
already create the full "virtual_object", that's fine for sure.

>
> > > +/**
> > > + * __virtual_device_create - create and register a virtual device and driver
> > > + * @virt_ops: struct virtual_driver_ops that the new device will call back into
> > > + * @name: name of the device and driver we are adding
> > > + * @owner: module owner of the device/driver
> > > + *
> > > + * Create a new virtual device and driver, both with the same name, and register
> > > + * them in the driver core properly. The probe() callback of @virt_ops will be
> > > + * called with the new device that is created for the caller to do something
> > > + * with.
> > > + */
> > > +struct virtual_device *__virtual_device_create(struct virtual_driver_ops *virt_ops,
> > > + const char *name, struct module *owner)
> > > +{
> > > + struct device_driver *drv;
> > > + struct device *dev;
> > > + struct virtual_object *virt_obj;
> > > + struct virtual_device *virt_dev;
> > > + int ret;
> > > +
> > > + pr_info("%s: %s\n", __func__, name);
> > > +
> > > + virt_obj = kzalloc(sizeof(*virt_obj) + strlen(name) + 1, GFP_KERNEL);
> > > + if (!virt_obj)
> > > + return NULL;
> > > +
> > > + /* Save off the name of the object into local memory */
> > > + strcpy(virt_obj->name, name);
> > > +
> > > + /* Initialize the driver portion and register it with the driver core */
> > > + virt_obj->virt_ops = virt_ops;
> >
> > I wonder if it would make sense to allow NULL for virt_ops and use default ops
> > in this case.
>
> What would be a "default"? If you don't care/want to do anything with
> probe/remove, then yes, we can allow it to be set to NULL.

Exactly that, no probe, no remove. With that we can avoid the full bus
abstraction in Rust.

>
> Actually looking at some of the places this can be replaced with, that
> does make sense, I'll go make that change.
>
> > This could be useful for the Rust side of things, since then we could probably
> > avoid having a virtual bus abstraction and instead would only need an
> > abstraction of __virtual_device_create() itself.
>
> Ok.
>
> > However, this is probalby only convenient for when we have a single device /
> > driver, but not multiple devices for a single driver.
>
> Again, see above, and stop worrying about the traditional "driver" model
> here, I took that away from you :)
>
> > The more I think about it, the less I think it's a good idea, since it'd
> > probably trick people into coming up with questionable constructs...
>
> No, I think it will work, let me do some replacements later today after
> I get some other work done, I think it does make sense, don't doubt
> yourself :)
>
> thanks,
>
> greg k-h