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

[Danilo Krummrich <dakr@kernel.org>]

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