Re: [PATCH] auxiliary: Automatically generate id

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

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.

> +
>  	ret = dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
>  	if (ret) {
>  		dev_err(dev, "auxiliary device dev_set_name failed: %d\n", ret);
> +		ida_free(&auxiliary_id, auxdev->id);
>  		return ret;
>  	}
>  
>  	ret = device_add(dev);
> -	if (ret)
> +	if (ret) {
>  		dev_err(dev, "adding auxiliary device failed!: %d\n", ret);
> +		ida_free(&auxiliary_id, auxdev->id);
> +	}
>  
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__auxiliary_device_add);
>  
> +void auxiliary_device_delete(struct auxiliary_device *auxdev)
> +{
> +	ida_free(&auxiliary_id, auxdev->id);

Hm...I wonder if we should call this from the device's release callback instead.

The above ensures that we never have duplicate IDs in sysfs, but we can still
end up with two auxiliary devices that have the same ID.

If we want the ID to be *always* unique, we need to introduce our own auxiliary
device release callback, such that we have a hook for the
auxiliary_device_init() and auxiliary_device_add() pair. Since so far drivers
are controlling the underlying device's release callback.

> +	device_del(&auxdev->dev);
> +}
> +EXPORT_SYMBOL_GPL(auxiliary_device_delete);