Re: [PATCH] auxiliary: Automatically generate id

["Danilo Krummrich" <dakr@kernel.org>]

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@linux.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".

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