Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > > Co-developed-by: B K Karthik > > Signed-off-by: B K Karthik > > Signed-off-by: Anupama K Patil > > --- > > 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 > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > Co-developed-by: B K Karthik > Signed-off-by: B K Karthik > Signed-off-by: Anupama K Patil > --- > 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 Linux Sound Maintainer; ALSA Project; Red Hat, Inc. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > > >>> Co-developed-by: B K Karthik > > >>> Signed-off-by: B K Karthik > > >>> Signed-off-by: Anupama K Patil > > >>> --- > > >>> 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 > > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. signature.asc Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 >>> Co-developed-by: B K Karthik >>> Signed-off-by: B K Karthik >>> Signed-off-by: Anupama K Patil >>> --- >>> 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 Linux Sound Maintainer; ALSA Project; Red Hat, Inc. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > > Co-developed-by: B K Karthik > > Signed-off-by: B K Karthik > > Signed-off-by: Anupama K Patil > > --- > > 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 > signature.asc Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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. pgpz44CcAsPiA.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > >>> Co-developed-by: B K Karthik > >>> Signed-off-by: B K Karthik > >>> Signed-off-by: Anupama K Patil > >>> --- > >>> 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 > Linux Sound Maintainer; ALSA Project; Red Hat, Inc. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
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 > Co-developed-by: B K Karthik > Signed-off-by: B K Karthik > Signed-off-by: Anupama K Patil > --- > 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
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 > Co-developed-by: B K Karthik > Signed-off-by: B K Karthik > Signed-off-by: Anupama K Patil > --- > 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
[PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
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 Co-developed-by: B K Karthik Signed-off-by: B K Karthik Signed-off-by: Anupama K Patil --- 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; +} + 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 signature.asc Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices
On Sun, 25 Apr 2021 01:13:01 +0530, Anupama K Patil said: > Changed sprintf() to the kernel-space function scnprintf() as it returns > the actual number of bytes written. > + if (!bus->procdir) { > + scnprintf(name, 16, "%02x", bus->number); > + scnprintf(name, 16, "%02x", dev->number); Why do this when you don't *use* the number of bytes written, but instead ignore the value returned? For bonus points: Given the %02x format, under what conditions can it return a value other than 2? pgpJubZ75HV_W.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies