[PATCH v3] drivers/base: return bus_add_driver() errors

[PATCH v3] drivers/base: return bus_add_driver() errors

[Yemingxuan]

bus_add_driver() creates the driver's uevent attribute, adds the bus
driver groups and the bind/unbind files after the driver has already
been inserted into the bus and its module links have been made.  When
any of these fail it logs the failure and still returns 0.

The driver is then registered but incomplete: a driver without
bind/unbind cannot be bound or unbound from userspace, and a driver
without its uevent attribute can no longer emit uevent notifications.
driver_register() reports success, so neither the driver's init routine
nor its caller retries, and nothing else ever creates the missing
files.

The device side of the same driver model already treats the equivalent
steps this way: device_add() returns failures of the equivalent sysfs
operations to its caller, and device_del() undoes each one.
driver_register() already calls bus_remove_driver() when the group
registration after bus_add_driver() fails, and that branch is only
unreachable for failures inside bus_add_driver() because of the
unconditional successful return.

Propagate the errors so the existing unwind runs.  The new
out_remove_module label calls module_remove_driver() before falling
through to out_detach: bus_remove_driver() pairs every operation
bus_add_driver() performs in reverse order, but the symlinks
module_add_driver() creates live in the module's sysfs directory, not
under drv->p->kobj, so the kobject teardown does not remove them.
The attribute and the groups need no explicit removal because they live
under drv->p->kobj, and kobject_cleanup() drops the sysfs directory for
the caller.

The error messages are kept.  The return value of an initcall is not
printed by the initcall machinery, so dropping them would turn a
reported failure into a silent one.

These failures are not expected on a working system: driver_ktype
always supplies sysfs_ops, so -EINVAL is unreachable, no in-tree bus
defines a drv_group that collides with the built-in uevent, bind or
unbind attributes, and -ENOMEM needs memory pressure.  The point is not
a user-visible bug but keeping the driver model consistent with the
device side and leaving the failure reported.

Signed-off-by: Yemingxuan <level_zero@qq.com>
---
 drivers/base/bus.c | 26 +
 1 file changed, 5 insertions(+), 2 deletions(-)

--- a/drivers/base/bus.c	2026-10-20 14:29:46.248067968 +0800
+++ b/drivers/base/bus.c	2026-10-20 14:29:46.266034395 +0800
@@ -775,25 +775,28 @@
 	if (error) {
 		printk(KERN_ERR "%s: uevent attr (%s) failed\n",
 			__func__, drv->name);
+		goto out_remove_module;
 	}
 	error = driver_add_groups(drv, sp->bus->drv_groups);
 	if (error) {
-		/* How the hell do we get out of this pickle? Give up */
 		printk(KERN_ERR "%s: driver_add_groups(%s) failed\n",
 			__func__, drv->name);
+		goto out_remove_module;
 	}
 
 	if (!drv->suppress_bind_attrs) {
 		error = add_bind_files(drv);
 		if (error) {
-			/* Ditto */
 			printk(KERN_ERR "%s: add_bind_files(%s) failed\n",
 				__func__, drv->name);
+			goto out_remove_module;
 		}
 	}
 
 	return 0;
 
+out_remove_module:
+	module_remove_driver(drv);
 out_detach:
 	driver_detach(drv);
 out_del_list:
-- 
2.34.1

Re: [PATCH v3] drivers/base: return bus_add_driver() errors

[Greg Kroah-Hartman]

On Tue, Oct 20, 2026 at 02:31:06PM +0800, Yemingxuan wrote:
> bus_add_driver() creates the driver's uevent attribute, adds the bus
> driver groups and the bind/unbind files after the driver has already
> been inserted into the bus and its module links have been made.  When
> any of these fail it logs the failure and still returns 0.
> 
> The driver is then registered but incomplete: a driver without
> bind/unbind cannot be bound or unbound from userspace, and a driver
> without its uevent attribute can no longer emit uevent notifications.
> driver_register() reports success, so neither the driver's init routine
> nor its caller retries, and nothing else ever creates the missing
> files.
> 
> The device side of the same driver model already treats the equivalent
> steps this way: device_add() returns failures of the equivalent sysfs
> operations to its caller, and device_del() undoes each one.
> driver_register() already calls bus_remove_driver() when the group
> registration after bus_add_driver() fails, and that branch is only
> unreachable for failures inside bus_add_driver() because of the
> unconditional successful return.
> 
> Propagate the errors so the existing unwind runs.  The new
> out_remove_module label calls module_remove_driver() before falling
> through to out_detach: bus_remove_driver() pairs every operation
> bus_add_driver() performs in reverse order, but the symlinks
> module_add_driver() creates live in the module's sysfs directory, not
> under drv->p->kobj, so the kobject teardown does not remove them.
> The attribute and the groups need no explicit removal because they live
> under drv->p->kobj, and kobject_cleanup() drops the sysfs directory for
> the caller.
> 
> The error messages are kept.  The return value of an initcall is not
> printed by the initcall machinery, so dropping them would turn a
> reported failure into a silent one.
> 
> These failures are not expected on a working system: driver_ktype
> always supplies sysfs_ops, so -EINVAL is unreachable, no in-tree bus
> defines a drv_group that collides with the built-in uevent, bind or
> unbind attributes, and -ENOMEM needs memory pressure.  The point is not
> a user-visible bug but keeping the driver model consistent with the
> device side and leaving the failure reported.
> 
> Signed-off-by: Yemingxuan <level_zero@qq.com>

Full name?

> ---
>  drivers/base/bus.c | 26 +
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> --- a/drivers/base/bus.c	2026-10-20 14:29:46.248067968 +0800
> +++ b/drivers/base/bus.c	2026-10-20 14:29:46.266034395 +0800
> @@ -775,25 +775,28 @@
>  	if (error) {
>  		printk(KERN_ERR "%s: uevent attr (%s) failed\n",
>  			__func__, drv->name);
> +		goto out_remove_module;
>  	}
>  	error = driver_add_groups(drv, sp->bus->drv_groups);
>  	if (error) {
> -		/* How the hell do we get out of this pickle? Give up */
>  		printk(KERN_ERR "%s: driver_add_groups(%s) failed\n",
>  			__func__, drv->name);
> +		goto out_remove_module;

Are you sure that this is going to work properly?  How was this tested?
That comment is a good indication that no one will hit this, so why even
worry about any of this?

thanks,

greg k-h

Re: [PATCH v3] drivers/base: return bus_add_driver() errors

[Yemingxuan]

Hi Greg,

Thanks for the review.

> Full name?

My full name is Ye Mingxuan; I had it in the From: line as a single
latinised word.  I will use the full form from now on.

> Are you sure that this is going to work properly?  How was this tested?

You're right to ask.  It has not been tested at runtime, only built and
checked with checkpatch.  I looked at the three failure modes again and
that is where the real problem is:

  - -EINVAL is unreachable: driver_ktype always supplies sysfs_ops, so
    sysfs_add_file_mode_ns() can never reject the attribute on that
    ground.
  - -EEXIST needs an in-tree bus whose drv_groups collides with the
    built-in uevent, bind or unbind attribute.  None of the eight buses
    that define drv_groups do.
  - -ENOMEM would need an allocation failure at exactly that point in
    boot.

So there is no way for me to construct the situation this patch is
supposed to handle, which also means I cannot show that the unwind is
correct.

> That comment is a good indication that no one will hit this, so why
> even worry about any of this?

Agreed.  I read that comment as "the author gave up on the error path",
but you are right that it says more about how reachable the path is than
about whether it is handled.  With -EINVAL and -EEXIST both ruled out and
-ENOMEM being a boot-time allocation failure, changing the error handling
here buys nothing that anyone can observe, and it does add a return path
that was never exercised.

Please drop this patch.  Sorry for the noise.

Ye Mingxuan