| Message ID | 20210424194301.jmsqpycvsm7izbk3@ubuntu |
|---|---|
| State | New |
| Headers | show |
| Series | drivers: pnp: proc.c: Handle errors while attaching devices | expand |
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
On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > 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. What exactly do you fix for such an old code? > > 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); > + return 0; > +} Please don't add one line functions that are called only once and have return value that no one care about it. Thanks > + > 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); > + 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); > + dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir, > &isapnp_proc_bus_proc_ops, dev); > - if (!e) > + if (!dev->procent) { > + isapnp_proc_detach_bus(bus); > 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); > protocol_for_each_dev(&isapnp_protocol, dev) { > - isapnp_proc_attach_device(dev); > + dev_attach = isapnp_proc_attach_device(dev); > + if (!dev_attach) { > + pr_info("procfs: pnp: Unable to attach the device, not enough memory"); > + isapnp_proc_detach_device(dev); > + return -ENOMEM; > + } > } > return 0; > } > -- > 2.25.1 > > _______________________________________________ > Kernelnewbies mailing list > Kernelnewbies@kernelnewbies.org > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On 21/04/26 08:04AM, Leon Romanovsky wrote: > On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > > 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. > > What exactly do you fix for such an old code? I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? > > > > > 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); > > + return 0; > > +} > > Please don't add one line functions that are called only once and have > return value that no one care about it. These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. Maybe those should be changed? thanks, karthik > > Thanks > > > + > > 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); > > + 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); > > + dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir, > > &isapnp_proc_bus_proc_ops, dev); > > - if (!e) > > + if (!dev->procent) { > > + isapnp_proc_detach_bus(bus); > > 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); > > protocol_for_each_dev(&isapnp_protocol, dev) { > > - isapnp_proc_attach_device(dev); > > + dev_attach = isapnp_proc_attach_device(dev); > > + if (!dev_attach) { > > + pr_info("procfs: pnp: Unable to attach the device, not enough memory"); > > + isapnp_proc_detach_device(dev); > > + return -ENOMEM; > > + } > > } > > return 0; > > } > > -- > > 2.25.1 > > > > > > > _______________________________________________ > > Kernelnewbies mailing list > > Kernelnewbies@kernelnewbies.org > > https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies >
On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote: > On 21/04/26 08:04AM, Leon Romanovsky wrote: > > On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > > > 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. <...> > > > +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); > > > + return 0; > > > +} > > > > Please don't add one line functions that are called only once and have > > return value that no one care about it. > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > Maybe those should be changed? Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too. Thanks
Dne 26. 04. 21 v 19:50 bkkarthik napsal(a): > On 21/04/26 08:04AM, Leon Romanovsky wrote: >> On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: >>> 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. >> >> What exactly do you fix for such an old code? > > I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. > Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) > > Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? > >> >>> >>> 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); >>> + return 0; >>> +} >> >> Please don't add one line functions that are called only once and have >> return value that no one care about it. > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > Maybe those should be changed? Which code you refer? I see: for_each_pci_dev(dev) pci_proc_attach_device(dev); The error codes are ignored, too. It does not harm, if proc entries are not created (in this case - the system is unstable anyway). We should concentrate only to the wrong pointers usage. Jaroslav -- Jaroslav Kysela <perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Wed, Apr 28, 2021 at 02:04:49PM +0200, Jaroslav Kysela wrote: > Dne 26. 04. 21 v 19:50 bkkarthik napsal(a): > > On 21/04/26 08:04AM, Leon Romanovsky wrote: > >> On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > >>> 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. > >> > >> What exactly do you fix for such an old code? > > > > I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. > > Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) > > > > Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? > > > >> > >>> > >>> 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); > >>> + return 0; > >>> +} > >> > >> Please don't add one line functions that are called only once and have > >> return value that no one care about it. > > > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > > Maybe those should be changed? > > Which code you refer? I see: > > for_each_pci_dev(dev) > pci_proc_attach_device(dev); He talks about isapnp_proc_detach_*() functions. > > > The error codes are ignored, too. It does not harm, if proc entries are not > created (in this case - the system is unstable anyway). We should concentrate > only to the wrong pointers usage. > > Jaroslav > > -- > Jaroslav Kysela <perex@perex.cz> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On 21/04/28 03:21PM, Leon Romanovsky wrote: > On Wed, Apr 28, 2021 at 02:04:49PM +0200, Jaroslav Kysela wrote: > > Dne 26. 04. 21 v 19:50 bkkarthik napsal(a): > > > On 21/04/26 08:04AM, Leon Romanovsky wrote: > > >> On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > > >>> 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. > > >> > > >> What exactly do you fix for such an old code? > > > > > > I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. > > > Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) > > > > > > Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? > > > > > >> > > >>> > > >>> 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); > > >>> + return 0; > > >>> +} > > >> > > >> Please don't add one line functions that are called only once and have > > >> return value that no one care about it. > > > > > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > > > Maybe those should be changed? > > > > Which code you refer? I see: > > > > for_each_pci_dev(dev) > > pci_proc_attach_device(dev); > > He talks about isapnp_proc_detach_*() functions. Yes, pci_proc_detach_device() and pci_proc_detach_bus() are both one-line functions as well. I don't mean to question working code, we only tried to do something similar here for ISA. thanks, karthik > > > > > > > The error codes are ignored, too. It does not harm, if proc entries are not > > created (in this case - the system is unstable anyway). We should concentrate > > only to the wrong pointers usage. > > > > Jaroslav > > > > -- > > Jaroslav Kysela <perex@perex.cz> > > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
Dne 28. 04. 21 v 14:21 Leon Romanovsky napsal(a): > On Wed, Apr 28, 2021 at 02:04:49PM +0200, Jaroslav Kysela wrote: >> Dne 26. 04. 21 v 19:50 bkkarthik napsal(a): >>> On 21/04/26 08:04AM, Leon Romanovsky wrote: >>>> On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: >>>>> 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. >>>> >>>> What exactly do you fix for such an old code? >>> >>> I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. >>> Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) >>> >>> Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? >>> >>>> >>>>> >>>>> 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); >>>>> + return 0; >>>>> +} >>>> >>>> Please don't add one line functions that are called only once and have >>>> return value that no one care about it. >>> >>> These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. >>> Maybe those should be changed? >> >> Which code you refer? I see: >> >> for_each_pci_dev(dev) >> pci_proc_attach_device(dev); > > He talks about isapnp_proc_detach_*() functions. But only this patch introduced those functions. The pci_proc_init() code does not call pci_proc_detach_*() functions and ignores the allocation errors, too. I don't think that this cleanup code is required. Jaroslav -- Jaroslav Kysela <perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On 21/04/28 02:30PM, Jaroslav Kysela wrote: > Dne 28. 04. 21 v 14:21 Leon Romanovsky napsal(a): > > On Wed, Apr 28, 2021 at 02:04:49PM +0200, Jaroslav Kysela wrote: > >> Dne 26. 04. 21 v 19:50 bkkarthik napsal(a): > >>> On 21/04/26 08:04AM, Leon Romanovsky wrote: > >>>> On Sun, Apr 25, 2021 at 01:13:01AM +0530, Anupama K Patil wrote: > >>>>> 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. > >>>> > >>>> What exactly do you fix for such an old code? > >>> > >>> I was not aware that this code is so old. This fix was made after checkpatch reported assignment inside an if-statement. > >>> Please ignore this patch if th change is not necessary as the code is probably not being used anywhere :) > >>> > >>> Maybe the code has to be marked as obsolete in the MAINTAINERS file to prevent patches being sent? > >>> > >>>> > >>>>> > >>>>> 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); > >>>>> + return 0; > >>>>> +} > >>>> > >>>> Please don't add one line functions that are called only once and have > >>>> return value that no one care about it. > >>> > >>> These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > >>> Maybe those should be changed? > >> > >> Which code you refer? I see: > >> > >> for_each_pci_dev(dev) > >> pci_proc_attach_device(dev); > > > > He talks about isapnp_proc_detach_*() functions. > > But only this patch introduced those functions. The pci_proc_init() code does > not call pci_proc_detach_*() functions and ignores the allocation errors, too. The changes in this patch make isapnp_proc_init() look at the return value of isapnp_proc_attach_device() and call isapnp_proc_detach_device() if that returns an error code. > I don't think that this cleanup code is required. Oh okay! karthik > > Jaroslav > > -- > Jaroslav Kysela <perex@perex.cz> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
On Tue, 27 Apr 2021 07:26:27 +0300, Leon Romanovsky said: > On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote: > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > > Maybe those should be changed? > > Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too. Will that actually build correctly if it's an embedded system or something build with CONFIG_PROC_FS=n? I'd expect that to die a horrid death while linking vmlinx due to stuff inside that #ifdef calling symbols only present with PROC_FS=m/y. In general, inline ifdef's are frowned upon, so if you come across one in the kernel source, that's probably a *big* hint that either (a) refactoring the code to eliminate an inline ifdef was just too ugly to be allowed to live or (b) you *have* to put a guard around it because you're guaranteed a build failure otherwise.
On Thu, Apr 29, 2021 at 12:31:13AM -0400, Valdis Klētnieks wrote: > On Tue, 27 Apr 2021 07:26:27 +0300, Leon Romanovsky said: > > On Mon, Apr 26, 2021 at 11:20:32PM +0530, bkkarthik wrote: > > > These were only intended for a clean-up job, the idea of this function came from how PCI handles procfs. > > > Maybe those should be changed? > > > > Probably, the CONFIG_PROC_FS around pci_proc_*() is not needed too. > > Will that actually build correctly if it's an embedded system or something build with > CONFIG_PROC_FS=n? I'd expect that to die a horrid death while linking vmlinx due > to stuff inside that #ifdef calling symbols only present with PROC_FS=m/y. We are talking about pci_proc_detach_device() and pci_proc_detach_bus() here. They will build perfectly without CONFIG_PROC_FS. Thanks
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); + return 0; +} + 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); + 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); + dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir, &isapnp_proc_bus_proc_ops, dev); - if (!e) + if (!dev->procent) { + isapnp_proc_detach_bus(bus); 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); protocol_for_each_dev(&isapnp_protocol, dev) { - isapnp_proc_attach_device(dev); + dev_attach = isapnp_proc_attach_device(dev); + if (!dev_attach) { + pr_info("procfs: pnp: Unable to attach the device, not enough memory"); + isapnp_proc_detach_device(dev); + return -ENOMEM; + } } return 0; }