Re: [PATCH] [v3] module: don't ignore sysfs_create_link() failures
On Tue, Mar 26, 2024, at 16:29, Andy Shevchenko wrote: > On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: >> From: Arnd Bergmann >> >> 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 >> Cc: linux-modu...@vger.kernel.org >> Cc: Greg Kroah-Hartman >> Cc: "Rafael J. Wysocki" > > Wondering if you can move these to be after --- to avoid polluting commit > message. This will have the same effect and be archived on lore. But on > pros side it will unload the commit message(s) from unneeded noise. Done > >> +error = module_add_driver(drv->owner, drv); >> +if (error) { >> +printk(KERN_ERR "%s: failed to create module links for %s\n", >> +__func__, drv->name); > > What's wrong with pr_err()? Even if it's not a style used, in a new pieces of > code this can be improved beforehand. So, we will reduce a technical debt, and > not adding to it. I think that would be more confusing, and would rather keep the style consistent. There is no practical difference here. >> +int module_add_driver(struct module *mod, struct device_driver *drv) >> { >> char *driver_name; >> -int no_warn; >> +int ret; > > I would move it... > >> struct module_kobject *mk = NULL; > > ...to be here. Done Arnd
Re: [PATCH] [v3] module: don't ignore sysfs_create_link() failures
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > 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 > Cc: linux-modu...@vger.kernel.org > Cc: Greg Kroah-Hartman > Cc: "Rafael J. Wysocki" > Fixes: e17e0f51aeea ("Driver core: show drivers in /sys/module/") > See-also: 4a7fb6363f2d ("add __must_check to device management code") > Signed-off-by: Arnd Bergmann > --- > v3: make error handling stricter, add unwinding, > fix build fail with CONFIG_MODULES=n > 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 | 9 ++--- > drivers/base/bus.c| 9 - > drivers/base/module.c | 42 +++--- > 3 files changed, 45 insertions(+), 15 deletions(-) Reviewed-by: Greg Kroah-Hartman
Re: [PATCH] [v3] module: don't ignore sysfs_create_link() failures
On Tue, Mar 26, 2024 at 03:57:18PM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > 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 > Cc: linux-modu...@vger.kernel.org > Cc: Greg Kroah-Hartman > Cc: "Rafael J. Wysocki" Wondering if you can move these to be after --- to avoid polluting commit message. This will have the same effect and be archived on lore. But on pros side it will unload the commit message(s) from unneeded noise. ... > + error = module_add_driver(drv->owner, drv); > + if (error) { > + printk(KERN_ERR "%s: failed to create module links for %s\n", > + __func__, drv->name); What's wrong with pr_err()? Even if it's not a style used, in a new pieces of code this can be improved beforehand. So, we will reduce a technical debt, and not adding to it. > + goto out_detach; > + } ... > +int module_add_driver(struct module *mod, struct device_driver *drv) > { > char *driver_name; > - int no_warn; > + int ret; I would move it... > struct module_kobject *mk = NULL; ...to be here. -- With Best Regards, Andy Shevchenko