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 <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.
participants (3)
-
bkkarthik -
Jaroslav Kysela -
Leon Romanovsky