drivers/base/auxiliary.c | 32 +++++++++++++++++++++++--------- include/linux/auxiliary_bus.h | 26 ++++++++------------------ 2 files changed, 31 insertions(+), 27 deletions(-)
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);
+
/**
* 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;
+
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);
+ device_del(&auxdev->dev);
+}
+EXPORT_SYMBOL_GPL(auxiliary_device_delete);
+
/**
* __auxiliary_driver_register - register a driver for auxiliary bus devices
* @auxdrv: auxiliary_driver structure
@@ -408,7 +427,6 @@ static void auxiliary_device_release(struct device *dev)
* @modname: module name used to create the auxiliary driver name.
* @devname: auxiliary bus device name
* @platform_data: auxiliary bus device platform data
- * @id: auxiliary bus device id
*
* Helper to create an auxiliary bus device.
* The device created matches driver 'modname.devname' on the auxiliary bus.
@@ -416,8 +434,7 @@ static void auxiliary_device_release(struct device *dev)
struct auxiliary_device *auxiliary_device_create(struct device *dev,
const char *modname,
const char *devname,
- void *platform_data,
- int id)
+ void *platform_data)
{
struct auxiliary_device *auxdev;
int ret;
@@ -426,7 +443,6 @@ struct auxiliary_device *auxiliary_device_create(struct device *dev,
if (!auxdev)
return NULL;
- auxdev->id = id;
auxdev->name = devname;
auxdev->dev.parent = dev;
auxdev->dev.platform_data = platform_data;
@@ -476,7 +492,6 @@ EXPORT_SYMBOL_GPL(auxiliary_device_destroy);
* @modname: module name used to create the auxiliary driver name.
* @devname: auxiliary bus device name
* @platform_data: auxiliary bus device platform data
- * @id: auxiliary bus device id
*
* Device managed helper to create an auxiliary bus device.
* The device created matches driver 'modname.devname' on the auxiliary bus.
@@ -484,13 +499,12 @@ EXPORT_SYMBOL_GPL(auxiliary_device_destroy);
struct auxiliary_device *__devm_auxiliary_device_create(struct device *dev,
const char *modname,
const char *devname,
- void *platform_data,
- int id)
+ void *platform_data)
{
struct auxiliary_device *auxdev;
int ret;
- auxdev = auxiliary_device_create(dev, modname, devname, platform_data, id);
+ auxdev = auxiliary_device_create(dev, modname, devname, platform_data);
if (!auxdev)
return NULL;
diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index 4086afd0cc6b..c972b5a3c2c4 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -57,7 +57,7 @@
* The release and parent fields of the device structure must be filled
* in
* @name: Match name found by the auxiliary device driver,
- * @id: unique identitier if multiple devices of the same name are exported,
+ * @id: automatically-generated unique identitier,
* @sysfs: embedded struct which hold all sysfs related fields,
* @sysfs.irqs: irqs xarray contains irq indices which are used by the device,
* @sysfs.lock: Synchronize irq sysfs creation,
@@ -74,15 +74,11 @@
* Registering an auxiliary_device is a three-step process.
*
* First, a 'struct auxiliary_device' needs to be defined or allocated for each
- * sub-device desired. The name, id, dev.release, and dev.parent fields of
- * this structure must be filled in as follows.
+ * sub-device desired. The name, dev.release, and dev.parent fields of this
+ * structure must be filled in as follows.
*
* The 'name' field is to be given a name that is recognized by the auxiliary
- * driver. If two auxiliary_devices with the same match_name, eg
- * "foo_mod.foo_dev", are registered onto the bus, they must have unique id
- * values (e.g. "x" and "y") so that the registered devices names are
- * "foo_mod.foo_dev.x" and "foo_mod.foo_dev.y". If match_name + id are not
- * unique, then the device_add fails and generates an error message.
+ * driver.
*
* The auxiliary_device.dev.type.release or auxiliary_device.dev.release must
* be populated with a non-NULL pointer to successfully register the
@@ -113,7 +109,6 @@
*
* // Step 1:
* my_aux_dev->name = MY_DEVICE_NAME;
- * my_aux_dev->id = my_unique_id_alloc(xxx);
* my_aux_dev->dev.release = my_aux_dev_release;
* my_aux_dev->dev.parent = my_dev;
*
@@ -242,10 +237,7 @@ static inline void auxiliary_device_uninit(struct auxiliary_device *auxdev)
put_device(&auxdev->dev);
}
-static inline void auxiliary_device_delete(struct auxiliary_device *auxdev)
-{
- device_del(&auxdev->dev);
-}
+void auxiliary_device_delete(struct auxiliary_device *auxdev);
int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner,
const char *modname);
@@ -257,19 +249,17 @@ void auxiliary_driver_unregister(struct auxiliary_driver *auxdrv);
struct auxiliary_device *auxiliary_device_create(struct device *dev,
const char *modname,
const char *devname,
- void *platform_data,
- int id);
+ void *platform_data);
void auxiliary_device_destroy(void *auxdev);
struct auxiliary_device *__devm_auxiliary_device_create(struct device *dev,
const char *modname,
const char *devname,
- void *platform_data,
- int id);
+ void *platform_data);
#define devm_auxiliary_device_create(dev, devname, platform_data) \
__devm_auxiliary_device_create(dev, KBUILD_MODNAME, devname, \
- platform_data, 0)
+ platform_data)
/**
* module_auxiliary_driver() - Helper macro for registering an auxiliary driver
--
2.35.1.1320.gc452695387.dirtyOn 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);
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.
>
> > +
> > 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.
Yes, you are right.
Thanks
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().
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@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".
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.
>
> 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)
(END)
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@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".
>
> 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.
On Tue, Jul 29, 2025 at 01:28:14PM +0200, Danilo Krummrich wrote:
> 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@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".
> >
> > 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.
Of course, the thing is that even drivers with auxiliary_device_init()
shouldn't set "id" and because they need to be updated.
The auxiliary_device_create() relies on auxiliary_device_init() under the hood,
so most likely the change should be there.
>
> >> 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.
It was just a sketch, everything that allows to set custom release
callback is fine by me.
Thanks
On Tue Jul 29, 2025 at 1:49 PM CEST, Leon Romanovsky wrote:
> On Tue, Jul 29, 2025 at 01:28:14PM +0200, Danilo Krummrich wrote:
>> 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@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".
>> >
>> > 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.
>
> Of course, the thing is that even drivers with auxiliary_device_init()
> shouldn't set "id" and because they need to be updated.
I think we agree here. :) It's only about the ordering between "convert to
auxiliary_device_create()" and "use internal IDA".
I think it'd be a good synergy to convert applicable drivers to
auxiliary_device_create() first, but I'm not insisting on it.
> The auxiliary_device_create() relies on auxiliary_device_init() under the hood,
> so most likely the change should be there.
>
>>
>> >> 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.
>
> It was just a sketch, everything that allows to set custom release
> callback is fine by me.
Yeah, what I meant is that we shouldn't add an additional release callback to
struct device for such things in general.
device::release() is for the user of the struct device, in this case this is
struct auxiliary_device.
auxiliary_device::release() will be for the user of struct auxiliary_device,
which could be some generic driver specific device structure, let's say
struct foo_device.
Now, struct foo_device may have another release callback for its specific
cleanup.
So, the callchain would look like this:
device::release {
auxiliary_device::release {
foo_device::release {
// clean up struct foo_device
}
// clean up struct auxiliary_device
}
// clean up struct device
}
Having additional release callbacks on struct device does not scale.
On Tue, Jul 29, 2025 at 02:31:43PM +0200, Danilo Krummrich wrote:
> On Tue Jul 29, 2025 at 1:49 PM CEST, Leon Romanovsky wrote:
> > On Tue, Jul 29, 2025 at 01:28:14PM +0200, Danilo Krummrich wrote:
> >> 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@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".
> >> >
> >> > 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.
> >
> > Of course, the thing is that even drivers with auxiliary_device_init()
> > shouldn't set "id" and because they need to be updated.
>
> I think we agree here. :) It's only about the ordering between "convert to
> auxiliary_device_create()" and "use internal IDA".
>
> I think it'd be a good synergy to convert applicable drivers to
> auxiliary_device_create() first, but I'm not insisting on it.
>
> > The auxiliary_device_create() relies on auxiliary_device_init() under the hood,
> > so most likely the change should be there.
> >
> >>
> >> >> 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.
> >
> > It was just a sketch, everything that allows to set custom release
> > callback is fine by me.
>
> Yeah, what I meant is that we shouldn't add an additional release callback to
> struct device for such things in general.
>
> device::release() is for the user of the struct device, in this case this is
> struct auxiliary_device.
>
> auxiliary_device::release() will be for the user of struct auxiliary_device,
> which could be some generic driver specific device structure, let's say
> struct foo_device.
>
> Now, struct foo_device may have another release callback for its specific
> cleanup.
>
> So, the callchain would look like this:
>
> device::release {
> auxiliary_device::release {
> foo_device::release {
> // clean up struct foo_device
> }
>
> // clean up struct auxiliary_device
> }
>
> // clean up struct device
> }
>
> Having additional release callbacks on struct device does not scale.
Yes, we both agree that more work on this "auxiliary: Automatically
generate id" patch is needed. :)
Thanks
© 2016 - 2025 Red Hat, Inc.