| 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 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,