On Fri, Mar 22, 2024 at 06:39:11PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> The sysfs_create_link() return code is marked as __must_check, but the
> module_add_driver() function tries hard to not care, by assigning the
> return code to a variable. When building with 'make W=1', gcc still
> warns because this variable is only assigned but not used:
>
> drivers/base/module.c: In function 'module_add_driver':
> drivers/base/module.c:36:6: warning: variable 'no_warn' set but not used [-Wunused-but-set-variable]
>
> Rework the code to properly unwind and return the error code to the
> caller. My reading of the original code was that it tries to
> not fail when the links already exist, so keep ignoring -EEXIST
> errors.
>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> Cc: linux-modules@xxxxxxxxxxxxxxx
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Fixes: e17e0f51aeea ("Driver core: show drivers in /sys/module/")
> See-also: 4a7fb6363f2d ("add __must_check to device management code")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> v2: rework to actually handle the error. I have not tested the
> error handling beyond build testing, so please review carefully.
> ---
> drivers/base/base.h | 2 +-
> drivers/base/bus.c | 7 ++++++-
> drivers/base/module.c | 42 +++++++++++++++++++++++++++++++-----------
> 3 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 0738ccad08b2..0e04bfe02943 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -192,7 +192,7 @@ extern struct kset *devices_kset;
> void devices_kset_move_last(struct device *dev);
>
> #if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS)
> -void module_add_driver(struct module *mod, struct device_driver *drv);
> +int module_add_driver(struct module *mod, struct device_driver *drv);
> void module_remove_driver(struct device_driver *drv);
> #else
> static inline void module_add_driver(struct module *mod,
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index daee55c9b2d9..7ef75b60d331 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -674,7 +674,12 @@ int bus_add_driver(struct device_driver *drv)
> if (error)
> goto out_del_list;
> }
> - module_add_driver(drv->owner, drv);
> + error = module_add_driver(drv->owner, drv);
> + if (error) {
> + printk(KERN_ERR "%s: failed to create module links for %s\n",
> + __func__, drv->name);
> + goto out_del_list;
Don't we need to walk back the driver_attach() call here if this fails?
> + }
>
> error = driver_create_file(drv, &driver_attr_uevent);
> if (error) {
> diff --git a/drivers/base/module.c b/drivers/base/module.c
> index 46ad4d636731..61282eaed670 100644
> --- a/drivers/base/module.c
> +++ b/drivers/base/module.c
> @@ -30,14 +30,14 @@ static void module_create_drivers_dir(struct module_kobject *mk)
> mutex_unlock(&drivers_dir_mutex);
> }
>
> -void module_add_driver(struct module *mod, struct device_driver *drv)
> +int module_add_driver(struct module *mod, struct device_driver *drv)
> {
> char *driver_name;
> - int no_warn;
> + int ret;
> struct module_kobject *mk = NULL;
>
> if (!drv)
> - return;
> + return 0;
>
> if (mod)
> mk = &mod->mkobj;
> @@ -56,17 +56,37 @@ void module_add_driver(struct module *mod, struct device_driver *drv)
> }
>
> if (!mk)
> - return;
> + return 0;
> +
> + ret = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module");
> + if (ret && ret != -EEXIST)
Why would EEXIST happen here? How can this be called twice?
> + return ret;
>
> - /* Don't check return codes; these calls are idempotent */
> - no_warn = sysfs_create_link(&drv->p->kobj, &mk->kobj, "module");
> driver_name = make_driver_name(drv);
> - if (driver_name) {
> - module_create_drivers_dir(mk);
> - no_warn = sysfs_create_link(mk->drivers_dir, &drv->p->kobj,
> - driver_name);
> - kfree(driver_name);
> + if (!driver_name) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + module_create_drivers_dir(mk);
> + if (!mk->drivers_dir) {
> + ret = -EINVAL;
> + goto out;
> }
> +
> + ret = sysfs_create_link(mk->drivers_dir, &drv->p->kobj, driver_name);
> + if (ret && ret != -EEXIST)
> + goto out;
Same EEXIST question here.
thanks,
greg k-h
