Messages in this thread | | | Date | Sun, 25 Apr 2021 01:06:06 +0000 | From | Barnabás Pőcze <> | Subject | Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices |
| |
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
| |