From: "Barnabás Pőcze" <pobrn@protonmail.com> To: Anupama K Patil <anupamakpatil123@gmail.com> Cc: Jaroslav Kysela <perex@perex.cz>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "skhan@linuxfoundation.org" <skhan@linuxfoundation.org>, "bkkarthik@pesu.pes.edu" <bkkarthik@pesu.pes.edu>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "kernelnewbies@kernelnewbies.org" <kernelnewbies@kernelnewbies.org> Subject: Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices Date: Sun, 25 Apr 2021 01:06:06 +0000 [thread overview] Message-ID: <CSBw5RbPfjHIC-pzsVOv-S6X9ir8mhLYp3cP3P6UizGPb-wc-OUBfXFXvAPr7fISCC2Bo_r1hHsoltEEEK7VXi8UyTNHc51lH_att8eNZqk=@protonmail.com> (raw) In-Reply-To: <20210424194301.jmsqpycvsm7izbk3@ubuntu> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Barnabás Pőcze" <pobrn@protonmail.com> To: Anupama K Patil <anupamakpatil123@gmail.com> Cc: "kernelnewbies@kernelnewbies.org" <kernelnewbies@kernelnewbies.org>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>, "bkkarthik@pesu.pes.edu" <bkkarthik@pesu.pes.edu>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Jaroslav Kysela <perex@perex.cz>, "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, "skhan@linuxfoundation.org" <skhan@linuxfoundation.org> Subject: Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices Date: Sun, 25 Apr 2021 01:06:06 +0000 [thread overview] Message-ID: <CSBw5RbPfjHIC-pzsVOv-S6X9ir8mhLYp3cP3P6UizGPb-wc-OUBfXFXvAPr7fISCC2Bo_r1hHsoltEEEK7VXi8UyTNHc51lH_att8eNZqk=@protonmail.com> (raw) In-Reply-To: <20210424194301.jmsqpycvsm7izbk3@ubuntu> 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 _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
next prev parent reply other threads:[~2021-04-25 1:06 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-24 19:43 [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices Anupama K Patil 2021-04-24 19:43 ` Anupama K Patil 2021-04-24 20:37 ` Valdis Klētnieks 2021-04-24 20:37 ` Valdis Klētnieks 2021-04-25 1:06 ` Barnabás Pőcze [this message] 2021-04-25 1:06 ` Barnabás Pőcze 2021-04-26 5:04 ` Leon Romanovsky 2021-04-26 5:04 ` Leon Romanovsky 2021-04-26 17:50 ` bkkarthik 2021-04-26 17:50 ` bkkarthik 2021-04-27 4:26 ` Leon Romanovsky 2021-04-27 4:26 ` Leon Romanovsky 2021-04-29 4:31 ` Valdis Klētnieks 2021-04-29 4:31 ` Valdis Klētnieks 2021-04-29 7:05 ` Leon Romanovsky 2021-04-29 7:05 ` Leon Romanovsky 2021-04-28 12:04 ` Jaroslav Kysela 2021-04-28 12:04 ` Jaroslav Kysela 2021-04-28 12:21 ` Leon Romanovsky 2021-04-28 12:21 ` Leon Romanovsky 2021-04-28 12:26 ` bkkarthik 2021-04-28 12:26 ` bkkarthik 2021-04-28 12:30 ` Jaroslav Kysela 2021-04-28 12:30 ` Jaroslav Kysela 2021-04-28 12:37 ` bkkarthik 2021-04-28 12:37 ` bkkarthik
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CSBw5RbPfjHIC-pzsVOv-S6X9ir8mhLYp3cP3P6UizGPb-wc-OUBfXFXvAPr7fISCC2Bo_r1hHsoltEEEK7VXi8UyTNHc51lH_att8eNZqk=@protonmail.com' \ --to=pobrn@protonmail.com \ --cc=anupamakpatil123@gmail.com \ --cc=bkkarthik@pesu.pes.edu \ --cc=gregkh@linuxfoundation.org \ --cc=kernelnewbies@kernelnewbies.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=perex@perex.cz \ --cc=rafael.j.wysocki@intel.com \ --cc=skhan@linuxfoundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.