Hi 2021. április 24., szombat 21:43 keltezéssel, Anupama K Patil írta:
isapnp_proc_init() does not look at the return value from isapnp_proc_attach_device(). Check for this return value in isapnp_proc_detach_device().
Cleanup in isapnp_proc_detach_device and isapnp_proc_detach_bus() for cleanup.
Changed sprintf() to the kernel-space function scnprintf() as it returns the actual number of bytes written.
Removed unnecessary variables de, e of type 'struct proc_dir_entry' to save memory.
Suggested-by: Shuah Khan <skhan@linuxfoundation.org> Co-developed-by: B K Karthik <bkkarthik@pesu.pes.edu> Signed-off-by: B K Karthik <bkkarthik@pesu.pes.edu> Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com> --- drivers/pnp/isapnp/proc.c | 40 +++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c index 785a796430fa..46ebc24175b7 100644 --- a/drivers/pnp/isapnp/proc.c +++ b/drivers/pnp/isapnp/proc.c @@ -54,34 +54,54 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = { .proc_read = isapnp_proc_bus_read, };
+static int isapnp_proc_detach_device(struct pnp_dev *dev) +{ + proc_remove(dev->procent); + dev->procent = NULL; + return 0; +} + +static int isapnp_proc_detach_bus(struct pnp_card *bus) +{ + proc_remove(bus->procdir);
Is there any reason for not setting `bus->procdir` to `NULL` similarly to the previous function?
+ return 0; +} +
Is there any reason why the previous two functions return something? It doesn't seem to be necessary.
static int isapnp_proc_attach_device(struct pnp_dev *dev) { struct pnp_card *bus = dev->card; - struct proc_dir_entry *de, *e; char name[16];
- if (!(de = bus->procdir)) { - sprintf(name, "%02x", bus->number); - de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir); - if (!de) + if (!bus->procdir) { + scnprintf(name, 16, "%02x", bus->number);
I think `sizeof(name)` would be preferable to hard-coding 16.
+ bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir); + if (!bus->procdir) return -ENOMEM; } - sprintf(name, "%02x", dev->number); - e = dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, de, + scnprintf(name, 16, "%02x", dev->number);
Here as well.
+ dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir, &isapnp_proc_bus_proc_ops, dev);
Please align the continuation properly.
- if (!e) + if (!dev->procent) { + isapnp_proc_detach_bus(bus);
I'm not sure if this should be here. If I'm not mistaken, the code creates a procfs directory for a bus when it first sees a `pnp_dev` from that bus. This call removes the whole directory for the bus, and with that, the files of those `pnp_dev`s which were successfully created earlier.
return -ENOMEM; - proc_set_size(e, 256); + } + proc_set_size(dev->procent, 256); return 0; }
int __init isapnp_proc_init(void) { struct pnp_dev *dev; + int dev_attach;
isapnp_proc_bus_dir = proc_mkdir("bus/isapnp", NULL);
You could add a check to see if this `proc_mkdir()` call succeeds, and possibly return early if it does not.
protocol_for_each_dev(&isapnp_protocol, dev) { - isapnp_proc_attach_device(dev); + dev_attach = isapnp_proc_attach_device(dev); + if (!dev_attach) {
`isapnp_proc_attach_device()` returns 0 on success, so the condition should be inverted. And maybe `err` or something like that would be a better name than `dev_attach`.
+ pr_info("procfs: pnp: Unable to attach the device, not enough memory");
If I'm not mistaken, allocation failures are logged, so this is probably not needed.
+ isapnp_proc_detach_device(dev);
I'm also not sure if this is needed here. If `isapnp_proc_attach_device()` returns an error, then `dev->procdir` could not have been "created". In other words, if the execution reaches this point, `proc_create_data()` could not have succeeded because either it had not yet been called or it had failed.
+ return -ENOMEM;
It is usually preferable to return the error code you receive. E.g.: err = isapnp_proc_attach_device(...); if (err) { ... return err; }
+ } } return 0; } -- 2.25.1
Regards, Barnabás Pőcze