[2/6] parport: Remove register_sysctl_table from parport_proc_register
Commit Message
This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. Register dev/parport/PORTNAME and
dev/parport/PORTNAME/devices. Temporary allocation for name is freed at
the end of the function. Remove all the struct elements that are no
longer used in the parport_device_sysctl_template struct. Add parport
specific defines that hide the base path sizes.
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
drivers/parport/procfs.c | 89 +++++++++++++++++++++++++---------------
1 file changed, 57 insertions(+), 32 deletions(-)
Comments
Awesome!
On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> +
> + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> + /*
> + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> + * We calculate for the second as that will give us enough for the first.
> + */
> + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
Any reason why not kzalloc()?
> + if (!tmp_dir_path) {
> + err = -ENOMEM;
> + goto exit_free_t;
> + }
>
> - t->port_dir[0].procname = port->name;
> + if (tmp_path_len
> + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
Since we are clearing up obfuscation code, it would be nicer to
make this easier to read and split the snprintf() into one line, capture
the error there. And then in a new line do the check. Even if we have to
add a new int value here.
Other than this I'd just ask to extend the commit log to use
the before and after of vmlinux (when this module is compiled in with all
the bells and whistles) with ./scripts/bloat-o-meter.
Ie build before the patch and cp vmlinux to vmlinux.old and then compare
with:
./scripts/bloat-o-meter vmlinux.old vmlinux
Can you also describe any testing if any.
With the above changes, feel free to add to all these patches:
Reviewed-by: Luis Chamberlain
> + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
BTW, we should be able to remove now replace register_sysctl_base() with a simple
register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
on kernel/sysctl.c and just remove:
* DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
* and then after all this register_sysctl_table() completely
Let me know if you'd like a stab at it, or if you prefer me to do that.
Luis
On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> Awesome!
>
> On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > +
> > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > + /*
> > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > + * We calculate for the second as that will give us enough for the first.
> > + */
> > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
>
> Any reason why not kzalloc()?
nope. Will zero it out.
>
> > + if (!tmp_dir_path) {
> > + err = -ENOMEM;
> > + goto exit_free_t;
> > + }
> >
> > - t->port_dir[0].procname = port->name;
> > + if (tmp_path_len
> > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
>
> Since we are clearing up obfuscation code, it would be nicer to
> make this easier to read and split the snprintf() into one line, capture
> the error there. And then in a new line do the check. Even if we have to
> add a new int value here.
np. Will do something like this:
num_chars_sprinted = snprintf(....
if(tmp_path_len <= num_chars_sprinted) {
err = -ENOENT;
...
}
>
> Other than this I'd just ask to extend the commit log to use
> the before and after of vmlinux (when this module is compiled in with all
> the bells and whistles) with ./scripts/bloat-o-meter.
>
> Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> with:
>
> ./scripts/bloat-o-meter vmlinux.old vmlinux
>
> Can you also describe any testing if any.
Sure thing. Will add the bloat-o-meter output on the last patch so as to
gather the results for all the patches.
I'll write some testing info on the patches.
>
> With the above changes, feel free to add to all these patches:
>
> Reviewed-by: Luis Chamberlain
Ack
>
> > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
>
> BTW, we should be able to remove now replace register_sysctl_base() with a simple
> register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> on kernel/sysctl.c and just remove:
>
> * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> * and then after all this register_sysctl_table() completely
>
> Let me know if you'd like a stab at it, or if you prefer me to do that.
I think I can give it a go. Should I just add that to these set of
patches? or should we create a new patch set?
>
> Luis
>
>
best
On Tue, May 16, 2023 at 04:31:16PM +0200, Joel Granados wrote:
> On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> > Awesome!
> >
> > On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > > +
> > > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > > + /*
> > > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > > + * We calculate for the second as that will give us enough for the first.
> > > + */
> > > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> >
> > Any reason why not kzalloc()?
> nope. Will zero it out.
>
> >
> > > + if (!tmp_dir_path) {
> > > + err = -ENOMEM;
> > > + goto exit_free_t;
> > > + }
> > >
> > > - t->port_dir[0].procname = port->name;
> > > + if (tmp_path_len
> > > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> >
> > Since we are clearing up obfuscation code, it would be nicer to
> > make this easier to read and split the snprintf() into one line, capture
> > the error there. And then in a new line do the check. Even if we have to
> > add a new int value here.
> np. Will do something like this:
>
> num_chars_sprinted = snprintf(....
> if(tmp_path_len <= num_chars_sprinted) {
> err = -ENOENT;
> ...
> }
>
> >
> > Other than this I'd just ask to extend the commit log to use
> > the before and after of vmlinux (when this module is compiled in with all
> > the bells and whistles) with ./scripts/bloat-o-meter.
> >
> > Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> > with:
> >
> > ./scripts/bloat-o-meter vmlinux.old vmlinux
> >
> > Can you also describe any testing if any.
> Sure thing. Will add the bloat-o-meter output on the last patch so as to
> gather the results for all the patches.
>
> I'll write some testing info on the patches.
>
>
> >
> > With the above changes, feel free to add to all these patches:
> >
> > Reviewed-by: Luis Chamberlain
> Ack
>
> >
> > > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> >
> > BTW, we should be able to remove now replace register_sysctl_base() with a simple
> > register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> > on kernel/sysctl.c and just remove:
> >
> > * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> > * and then after all this register_sysctl_table() completely
> >
> > Let me know if you'd like a stab at it, or if you prefer me to do that.
> I think I can give it a go. Should I just add that to these set of
> patches? or should we create a new patch set?
I'll send the V2 of this patch set without this. Will add it to the
patch set when I finish if it makes sense. Else I'll just create a new
series.
Best
On Tue, May 16, 2023 at 06:12:21PM +0200, Joel Granados wrote:
> On Tue, May 16, 2023 at 04:31:16PM +0200, Joel Granados wrote:
> > On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> > > Awesome!
> > >
> > > On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > > > +
> > > > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > > > + /*
> > > > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > > > + * We calculate for the second as that will give us enough for the first.
> > > > + */
> > > > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > > > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> > >
> > > Any reason why not kzalloc()?
> > nope. Will zero it out.
> >
> > >
> > > > + if (!tmp_dir_path) {
> > > > + err = -ENOMEM;
> > > > + goto exit_free_t;
> > > > + }
> > > >
> > > > - t->port_dir[0].procname = port->name;
> > > > + if (tmp_path_len
> > > > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> > >
> > > Since we are clearing up obfuscation code, it would be nicer to
> > > make this easier to read and split the snprintf() into one line, capture
> > > the error there. And then in a new line do the check. Even if we have to
> > > add a new int value here.
> > np. Will do something like this:
> >
> > num_chars_sprinted = snprintf(....
> > if(tmp_path_len <= num_chars_sprinted) {
> > err = -ENOENT;
> > ...
> > }
> >
> > >
> > > Other than this I'd just ask to extend the commit log to use
> > > the before and after of vmlinux (when this module is compiled in with all
> > > the bells and whistles) with ./scripts/bloat-o-meter.
> > >
> > > Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> > > with:
> > >
> > > ./scripts/bloat-o-meter vmlinux.old vmlinux
> > >
> > > Can you also describe any testing if any.
> > Sure thing. Will add the bloat-o-meter output on the last patch so as to
> > gather the results for all the patches.
> >
> > I'll write some testing info on the patches.
> >
> >
> > >
> > > With the above changes, feel free to add to all these patches:
> > >
> > > Reviewed-by: Luis Chamberlain
> > Ack
> >
> > >
> > > > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> > >
> > > BTW, we should be able to remove now replace register_sysctl_base() with a simple
> > > register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> > > on kernel/sysctl.c and just remove:
> > >
> > > * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> > > * and then after all this register_sysctl_table() completely
> > >
> > > Let me know if you'd like a stab at it, or if you prefer me to do that.
> > I think I can give it a go. Should I just add that to these set of
> > patches? or should we create a new patch set?
> I'll send the V2 of this patch set without this. Will add it to the
> patch set when I finish if it makes sense. Else I'll just create a new
> series.
New series is good.
Luis
@@ -32,6 +32,13 @@
#define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ)
#define PARPORT_MIN_SPINTIME_VALUE 1
#define PARPORT_MAX_SPINTIME_VALUE 1000
+/*
+ * PARPORT_BASE_* is the size of the known parts of the sysctl path
+ * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9
+ * and null char(1).
+ */
+#define PARPORT_BASE_PATH_SIZE 13
+#define PARPORT_BASE_DEVICES_PATH_SIZE 22
static int do_active_device(struct ctl_table *table, int write,
void *result, size_t *lenp, loff_t *ppos)
@@ -260,9 +267,6 @@ struct parport_sysctl_table {
struct ctl_table_header *sysctl_header;
struct ctl_table vars[12];
struct ctl_table device_dir[2];
- struct ctl_table port_dir[2];
- struct ctl_table parport_dir[2];
- struct ctl_table dev_dir[2];
};
static const struct parport_sysctl_table parport_sysctl_template = {
@@ -305,7 +309,6 @@ static const struct parport_sysctl_table parport_sysctl_template = {
.mode = 0444,
.proc_handler = do_hardware_modes
},
- PARPORT_DEVICES_ROOT_DIR,
#ifdef CONFIG_PARPORT_1284
{
.procname = "autoprobe",
@@ -355,18 +358,6 @@ static const struct parport_sysctl_table parport_sysctl_template = {
},
{}
},
- {
- PARPORT_PORT_DIR(NULL),
- {}
- },
- {
- PARPORT_PARPORT_DIR(NULL),
- {}
- },
- {
- PARPORT_DEV_DIR(NULL),
- {}
- }
};
struct parport_device_sysctl_table
@@ -477,7 +468,9 @@ parport_default_sysctl_table = {
int parport_proc_register(struct parport *port)
{
struct parport_sysctl_table *t;
- int i;
+ char *tmp_dir_path;
+ size_t tmp_path_len, port_name_len;
+ int i, err = 0;
t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
if (t == NULL)
@@ -485,28 +478,60 @@ int parport_proc_register(struct parport *port)
t->device_dir[0].extra1 = port;
- for (i = 0; i < 5; i++)
- t->vars[i].extra1 = port;
-
t->vars[0].data = &port->spintime;
- t->vars[5].child = t->device_dir;
-
- for (i = 0; i < 5; i++)
- t->vars[6 + i].extra2 = &port->probe_info[i];
+ for (i = 0; i < 5; i++) {
+ t->vars[i].extra1 = port;
+ t->vars[5 + i].extra2 = &port->probe_info[i];
+ }
+
+ port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
+ /*
+ * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
+ * We calculate for the second as that will give us enough for the first.
+ */
+ tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
+ tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
+ if (!tmp_dir_path) {
+ err = -ENOMEM;
+ goto exit_free_t;
+ }
- t->port_dir[0].procname = port->name;
+ if (tmp_path_len
+ <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
+ kfree(tmp_dir_path);
+ return -ENOENT;
+ }
+ if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
+ /*
+ * We keep the original behavior of parport where failure to register
+ * does not return error.
+ */
+ goto exit_free_tmp_dir_path;
- t->port_dir[0].child = t->vars;
- t->parport_dir[0].child = t->port_dir;
- t->dev_dir[0].child = t->parport_dir;
- t->sysctl_header = register_sysctl_table(t->dev_dir);
- if (t->sysctl_header == NULL) {
- kfree(t);
- t = NULL;
+ tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len;
+ if (tmp_path_len
+ <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s", port->name)) {
+ err = -ENOENT;
+ goto exit_free_tmp_dir_path;
}
+
+ t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
+ if (t->sysctl_header == NULL)
+ goto exit_free_tmp_dir_path;
+
port->sysctl_table = t;
+
+ kfree(tmp_dir_path);
return 0;
+
+exit_free_tmp_dir_path:
+ kfree(tmp_dir_path);
+exit_free_t:
+ kfree(t);
+ t = NULL;
+ return err;
+
}
int parport_proc_unregister(struct parport *port)