| Message ID | 20200529074108.16928-10-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sysctl: spring cleaning | expand |
On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote: > From: Xiaoming Ni <nixiaoming@huawei.com> > > Move the firmware config sysctl table to fallback_table.c and use the > new register_sysctl_subdir() helper. This removes the clutter from > kernel/sysctl.c. > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > --- > drivers/base/firmware_loader/fallback.c | 4 ++++ > drivers/base/firmware_loader/fallback.h | 11 ++++++++++ > drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++-- > include/linux/sysctl.h | 1 - > kernel/sysctl.c | 7 ------ > 5 files changed, 35 insertions(+), 10 deletions(-) So it now takes more lines than the old stuff? :( > > diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c > index d9ac7296205e..8190653ae9a3 100644 > --- a/drivers/base/firmware_loader/fallback.c > +++ b/drivers/base/firmware_loader/fallback.c > @@ -200,12 +200,16 @@ static struct class firmware_class = { > > int register_sysfs_loader(void) > { > + int ret = register_firmware_config_sysctl(); > + if (ret != 0) > + return ret; checkpatch :( > return class_register(&firmware_class); And if that fails? > } > > void unregister_sysfs_loader(void) > { > class_unregister(&firmware_class); > + unregister_firmware_config_sysctl(); > } > > static ssize_t firmware_loading_show(struct device *dev, > diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h > index 06f4577733a8..7d2cb5f6ceb8 100644 > --- a/drivers/base/firmware_loader/fallback.h > +++ b/drivers/base/firmware_loader/fallback.h > @@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void); > > int register_sysfs_loader(void); > void unregister_sysfs_loader(void); > +#ifdef CONFIG_SYSCTL > +extern int register_firmware_config_sysctl(void); > +extern void unregister_firmware_config_sysctl(void); > +#else > +static inline int register_firmware_config_sysctl(void) > +{ > + return 0; > +} > +static inline void unregister_firmware_config_sysctl(void) { } > +#endif /* CONFIG_SYSCTL */ > + > #else /* CONFIG_FW_LOADER_USER_HELPER */ > static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name, > struct device *device, > diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c > index 46a731dede6f..4234aa5ee5df 100644 > --- a/drivers/base/firmware_loader/fallback_table.c > +++ b/drivers/base/firmware_loader/fallback_table.c > @@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = { > EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE); > > #ifdef CONFIG_SYSCTL > -struct ctl_table firmware_config_table[] = { > +static struct ctl_table firmware_config_table[] = { > { > .procname = "force_sysfs_fallback", > .data = &fw_fallback_config.force_sysfs_fallback, > @@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = { > }, > { } > }; > -#endif > + > +static struct ctl_table_header *hdr; > +int register_firmware_config_sysctl(void) > +{ > + if (hdr) > + return -EEXIST; How can hdr be set? > + hdr = register_sysctl_subdir("kernel", "firmware_config", > + firmware_config_table); > + if (!hdr) > + return -ENOMEM; > + return 0; > +} > + > +void unregister_firmware_config_sysctl(void) > +{ > + if (hdr) > + unregister_sysctl_table(hdr); Why can't unregister_sysctl_table() take a null pointer value? And what sets 'hdr' (worst name for a static variable) to NULL so that it knows not to be unregistered again as it looks like register_firmware_config_sysctl() could be called multiple times. thanks, greg k-h
On 2020/5/29 18:26, Greg KH wrote: > On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote: >> From: Xiaoming Ni <nixiaoming@huawei.com> >> >> Move the firmware config sysctl table to fallback_table.c and use the >> new register_sysctl_subdir() helper. This removes the clutter from >> kernel/sysctl.c. >> >> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> >> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> >> --- >> drivers/base/firmware_loader/fallback.c | 4 ++++ >> drivers/base/firmware_loader/fallback.h | 11 ++++++++++ >> drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++-- >> include/linux/sysctl.h | 1 - >> kernel/sysctl.c | 7 ------ >> 5 files changed, 35 insertions(+), 10 deletions(-) > > So it now takes more lines than the old stuff? :( > CONFIG_FW_LOADER = m Before cleaning, no matter whether ko is loaded or not, the sysctl interface will be created, but now we need to add register and unregister interfaces, so the number of lines of code has increased >> >> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c >> index d9ac7296205e..8190653ae9a3 100644 >> --- a/drivers/base/firmware_loader/fallback.c >> +++ b/drivers/base/firmware_loader/fallback.c >> @@ -200,12 +200,16 @@ static struct class firmware_class = { >> >> int register_sysfs_loader(void) >> { >> + int ret = register_firmware_config_sysctl(); >> + if (ret != 0) >> + return ret; > > checkpatch :( This is my fault, thanks for your guidance > >> return class_register(&firmware_class); > > And if that fails? > Yes, it is better to call register_firmware_config_sysctl() after class_register(). thanks for your guidance. >> } >> >> void unregister_sysfs_loader(void) >> { >> class_unregister(&firmware_class); >> + unregister_firmware_config_sysctl(); >> } >> >> static ssize_t firmware_loading_show(struct device *dev, >> diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h >> index 06f4577733a8..7d2cb5f6ceb8 100644 >> --- a/drivers/base/firmware_loader/fallback.h >> +++ b/drivers/base/firmware_loader/fallback.h >> @@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void); >> >> int register_sysfs_loader(void); >> void unregister_sysfs_loader(void); >> +#ifdef CONFIG_SYSCTL >> +extern int register_firmware_config_sysctl(void); >> +extern void unregister_firmware_config_sysctl(void); >> +#else >> +static inline int register_firmware_config_sysctl(void) >> +{ >> + return 0; >> +} >> +static inline void unregister_firmware_config_sysctl(void) { } >> +#endif /* CONFIG_SYSCTL */ >> + >> #else /* CONFIG_FW_LOADER_USER_HELPER */ >> static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name, >> struct device *device, >> diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c >> index 46a731dede6f..4234aa5ee5df 100644 >> --- a/drivers/base/firmware_loader/fallback_table.c >> +++ b/drivers/base/firmware_loader/fallback_table.c >> @@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = { >> EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE); >> >> #ifdef CONFIG_SYSCTL >> -struct ctl_table firmware_config_table[] = { >> +static struct ctl_table firmware_config_table[] = { >> { >> .procname = "force_sysfs_fallback", >> .data = &fw_fallback_config.force_sysfs_fallback, >> @@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = { >> }, >> { } >> }; >> -#endif >> + >> +static struct ctl_table_header *hdr; >> +int register_firmware_config_sysctl(void) >> +{ >> + if (hdr) >> + return -EEXIST; > > How can hdr be set? > It's my mistake, register_firmware_config_sysctl() is not exported, there will be no repeated calls. thanks for your guidance. >> + hdr = register_sysctl_subdir("kernel", "firmware_config", >> + firmware_config_table); >> + if (!hdr) >> + return -ENOMEM; >> + return 0; >> +} >> + >> +void unregister_firmware_config_sysctl(void) >> +{ >> + if (hdr) >> + unregister_sysctl_table(hdr); > > Why can't unregister_sysctl_table() take a null pointer value? Sorry, I didn't notice that the unregister_sysctl_table() already checks the input parameters. thanks for your guidance. > And what sets 'hdr' (worst name for a static variable) to NULL so that > it knows not to be unregistered again as it looks like > register_firmware_config_sysctl() could be called multiple times. How about renaming hdr to firmware_config_sysct_table_header? + if (hdr) + return -EEXIST; After deleting this code in register_firmware_config_sysctl(), and considering register_firmware_config_sysctl() and unregister_firmware_config_sysctl() are not exported, whether there is no need to add "hdr = NULL;" ? Thanks Xiaoming Ni
On Fri, May 29, 2020 at 12:26:13PM +0200, Greg KH wrote: > On Fri, May 29, 2020 at 07:41:04AM +0000, Luis Chamberlain wrote: > > From: Xiaoming Ni <nixiaoming@huawei.com> > > > > Move the firmware config sysctl table to fallback_table.c and use the > > new register_sysctl_subdir() helper. This removes the clutter from > > kernel/sysctl.c. > > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com> > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > --- > > drivers/base/firmware_loader/fallback.c | 4 ++++ > > drivers/base/firmware_loader/fallback.h | 11 ++++++++++ > > drivers/base/firmware_loader/fallback_table.c | 22 +++++++++++++++++-- > > include/linux/sysctl.h | 1 - > > kernel/sysctl.c | 7 ------ > > 5 files changed, 35 insertions(+), 10 deletions(-) > > So it now takes more lines than the old stuff? :( Pretty much agreed with the other changes, thanks for the review! But this diff-stat change, indeed, it is unfortunate that we end up with more code here than before. We'll try to reduce it instead somehow, however in some cases during this spring-cleaning, since the goal is to move code from one file to another, it *may* require more code. So it won't always be negative. But we'll try! Luis
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c index d9ac7296205e..8190653ae9a3 100644 --- a/drivers/base/firmware_loader/fallback.c +++ b/drivers/base/firmware_loader/fallback.c @@ -200,12 +200,16 @@ static struct class firmware_class = { int register_sysfs_loader(void) { + int ret = register_firmware_config_sysctl(); + if (ret != 0) + return ret; return class_register(&firmware_class); } void unregister_sysfs_loader(void) { class_unregister(&firmware_class); + unregister_firmware_config_sysctl(); } static ssize_t firmware_loading_show(struct device *dev, diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h index 06f4577733a8..7d2cb5f6ceb8 100644 --- a/drivers/base/firmware_loader/fallback.h +++ b/drivers/base/firmware_loader/fallback.h @@ -42,6 +42,17 @@ void fw_fallback_set_default_timeout(void); int register_sysfs_loader(void); void unregister_sysfs_loader(void); +#ifdef CONFIG_SYSCTL +extern int register_firmware_config_sysctl(void); +extern void unregister_firmware_config_sysctl(void); +#else +static inline int register_firmware_config_sysctl(void) +{ + return 0; +} +static inline void unregister_firmware_config_sysctl(void) { } +#endif /* CONFIG_SYSCTL */ + #else /* CONFIG_FW_LOADER_USER_HELPER */ static inline int firmware_fallback_sysfs(struct firmware *fw, const char *name, struct device *device, diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c index 46a731dede6f..4234aa5ee5df 100644 --- a/drivers/base/firmware_loader/fallback_table.c +++ b/drivers/base/firmware_loader/fallback_table.c @@ -24,7 +24,7 @@ struct firmware_fallback_config fw_fallback_config = { EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE); #ifdef CONFIG_SYSCTL -struct ctl_table firmware_config_table[] = { +static struct ctl_table firmware_config_table[] = { { .procname = "force_sysfs_fallback", .data = &fw_fallback_config.force_sysfs_fallback, @@ -45,4 +45,22 @@ struct ctl_table firmware_config_table[] = { }, { } }; -#endif + +static struct ctl_table_header *hdr; +int register_firmware_config_sysctl(void) +{ + if (hdr) + return -EEXIST; + hdr = register_sysctl_subdir("kernel", "firmware_config", + firmware_config_table); + if (!hdr) + return -ENOMEM; + return 0; +} + +void unregister_firmware_config_sysctl(void) +{ + if (hdr) + unregister_sysctl_table(hdr); +} +#endif /* CONFIG_SYSCTL */ diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 58bc978d4f03..aa01f54d0442 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -217,7 +217,6 @@ extern int no_unaligned_warning; extern struct ctl_table sysctl_mount_point[]; extern struct ctl_table random_table[]; -extern struct ctl_table firmware_config_table[]; extern struct ctl_table epoll_table[]; #else /* CONFIG_SYSCTL */ diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 30c2d521502a..e007375c8a11 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2088,13 +2088,6 @@ static struct ctl_table kern_table[] = { .mode = 0555, .child = usermodehelper_table, }, -#ifdef CONFIG_FW_LOADER_USER_HELPER - { - .procname = "firmware_config", - .mode = 0555, - .child = firmware_config_table, - }, -#endif { .procname = "overflowuid", .data = &overflowuid,