Message-Id: <DBOI4VYHAJFB.1T4FSZ00HSUI8@kernel.org>
Date: Tue, 29 Jul 2025 13:28:14 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Leon Romanovsky" <leon@...nel.org>
Cc: "Sean Anderson" <sean.anderson@...ux.dev>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Ira Weiny" <ira.weiny@...el.com>,
 <linux-kernel@...r.kernel.org>, "Rafael J . Wysocki" <rafael@...nel.org>,
 "Dave Ertman" <david.m.ertman@...el.com>
Subject: Re: [PATCH] auxiliary: Automatically generate id

On Tue Jul 29, 2025 at 1:11 PM CEST, Leon Romanovsky wrote:
> On Tue, Jul 29, 2025 at 12:51:42PM +0200, Danilo Krummrich wrote:
>> On Tue Jul 29, 2025 at 12:01 PM CEST, Leon Romanovsky wrote:
>> > On Tue, Jul 29, 2025 at 11:36:27AM +0200, Danilo Krummrich wrote:
>> >> On Mon Jul 28, 2025 at 11:10 PM CEST, Sean Anderson wrote:
>> >> > As it turns out, ids are not allowed to have semantic meaning. Their
>> >> > only purpose is to prevent sysfs collisions. To simplify things, just
>> >> > generate a unique id for each auxiliary device. Remove all references to
>> >> > filling in the id member of the device.
>> >> >
>> >> > Signed-off-by: Sean Anderson <sean.anderson@...ux.dev>
>> >> > ---
>> >> >
>> >> >  drivers/base/auxiliary.c      | 32 +++++++++++++++++++++++---------
>> >> >  include/linux/auxiliary_bus.h | 26 ++++++++------------------
>> >> >  2 files changed, 31 insertions(+), 27 deletions(-)
>> >> >
>> >> > diff --git a/drivers/base/auxiliary.c b/drivers/base/auxiliary.c
>> >> > index dba7c8e13a53..f66067df03ad 100644
>> >> > --- a/drivers/base/auxiliary.c
>> >> > +++ b/drivers/base/auxiliary.c
>> >> > @@ -264,6 +264,8 @@ static const struct bus_type auxiliary_bus_type = {
>> >> >  	.pm = &auxiliary_dev_pm_ops,
>> >> >  };
>> >> >  
>> >> > +static DEFINE_IDA(auxiliary_id);
>> >> 
>> >> I think this is the correct thing to do, even though the per device IDA drivers
>> >> typically went for so far produces IDs that are easier to handle when debugging
>> >> things.
>> >> 
>> >> > +
>> >> >  /**
>> >> >   * auxiliary_device_init - check auxiliary_device and initialize
>> >> >   * @auxdev: auxiliary device struct
>> >> > @@ -331,20 +333,37 @@ int __auxiliary_device_add(struct auxiliary_device *auxdev, const char *modname)
>> >> >  		return -EINVAL;
>> >> >  	}
>> >> >  
>> >> > +	ret = ida_alloc(&auxiliary_id, GFP_KERNEL);
>> >> > +	if (ret < 0) {
>> >> > +		dev_err(dev, "auxiliary device id_alloc fauiled: %d\n", ret);
>> >> > +		return ret;
>> >> > +	}
>> >> > +	auxdev->id = ret;
>> >> 
>> >> This overwrites the ID number set by various drivers that (still) use the
>> >> auxiliary_device_init() and auxiliary_device_add() pair.
>> >> 
>> >> While I agree with the general intent, I think it's a very bad idea to just
>> >> perform this change silently leaving drivers with their IDA instances not
>> >> knowing that the set ID numbers do not have an effect anymore.
>> >> 
>> >> I think this should be multiple steps:
>> >> 
>> >>   (1) Remove the id parameter and force an internal ID only for
>> >>       auxiliary_device_create().
>> >> 
>> >>   (2) Convert applicable drivers (and the Rust abstraction) to use
>> >>       auxiliary_device_create() rather than auxiliary_device_init() and
>> >>       auxiliary_device_add().
>> >> 
>> >>   (3) Treewide change to force an internal ID for all auxiliary devices
>> >>       considering this change in all affected drivers.
>> >
>> > I would suggest easier approach.
>> > 1. Add to the proposal patch, the sed generated line which removes auxdev->id
>> > assignment in the drivers.
>> > Something like this from mlx5:
>> >  - sf_dev->adev.id = id;
>> >
>> > 2. Add standalone patches to remove not used ida_alloc/ida_free calls
>> > from the drivers.
>> 
>> I assume you suggest this as an alternative to (3) above? If so, that's what I
>> meant in (3), I should have written "treewide series" instead of "treewide
>> change".
>
> I would say for all steps. Very important reason to use
> auxiliary_device_init() and not auxiliary_device_create() is to bind
> custom release callback, which is needed to release private data.
>
> In addition, complex devices embed struct auxiliary_device in their
> internal struct to rely on container_of to access the data.
> See mlx5_sf_dev_add() in drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.c
> as an example.

That's why I said "*applicable* drivers" everywhere. :)

The examples you mention don't fall under this category, but in general drivers
that *can* use auxiliary_device_create() should do it.

>> Technically (2) is orthogonal, yet I think it's a bit better to do the desired
>> change right away. Otherwise we end up converting all applicable drivers to
>> implement the auxiliary device release callback (which we need for a common
>> ida_free()) first, just to remove it later on when we convert to
>> auxiliary_device_create().
>
> My expectation is to see extension of driver/base/core.c. Something like that:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index cbc0099d8ef24..63847c84dbdc0 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2560,8 +2560,10 @@ static void device_release(struct kobject *kobj)
>
>         kfree(dev->dma_range_map);
>
> -       if (dev->release)
> +       if (dev->release) {
> +               dev->bus_specific_cleanup(dev);
>                 dev->release(dev);
> +       }
>         else if (dev->type && dev->type->release)
>                 dev->type->release(dev);
>         else if (dev->class && dev->class->dev_release)

The common pattern is to have custom release callbacks for class or bus specific
device types.

In this case drivers would set struct auxiliary_device::release. And the
auxiliary bus would implement the underlying struct device::release to call the
driver provided struct auxiliary_device::release plus the additional cleanup.

What you propose works as well, but it moves bus or class device specifics into
the generic struct device, where the normal inheritance pattern already solves
this.

Powered by blists - more mailing lists