linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
@ 2021-04-22 18:03 Anupama K Patil
  2021-04-23 21:08 ` Shuah Khan
  2021-04-28 12:17 ` Jaroslav Kysela
  0 siblings, 2 replies; 9+ messages in thread
From: Anupama K Patil @ 2021-04-22 18:03 UTC (permalink / raw)
  To: Jaroslav Kysela, Rafael J. Wysocki, linux-acpi, linux-kernel,
	skhan, Adam, bkkarthik, gregkh, kernelnewbies,
	linux-kernel-mentees

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

de, e are two variables of the type 'struct proc_dir_entry'
which can be removed to save memory. This also fixes a coding style
issue reported by checkpatch where we are suggested to make assignment
outside the if statement.

Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com>
---
 drivers/pnp/isapnp/proc.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
index 785a796430fa..1ae458c02656 100644
--- a/drivers/pnp/isapnp/proc.c
+++ b/drivers/pnp/isapnp/proc.c
@@ -57,21 +57,20 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = {
 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)) {
+	if (!bus->procdir) {
 		sprintf(name, "%02x", bus->number);
-		de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
-		if (!de)
+		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,
+	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir,
 					    &isapnp_proc_bus_proc_ops, dev);
-	if (!e)
+	if (!dev->procent)
 		return -ENOMEM;
-	proc_set_size(e, 256);
+	proc_set_size(dev->procent, 256);
 	return 0;
 }
 
-- 
2.25.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-22 18:03 [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles Anupama K Patil
@ 2021-04-23 21:08 ` Shuah Khan
  2021-04-26  4:57   ` Leon Romanovsky
  2021-04-28 12:12   ` Jaroslav Kysela
  2021-04-28 12:17 ` Jaroslav Kysela
  1 sibling, 2 replies; 9+ messages in thread
From: Shuah Khan @ 2021-04-23 21:08 UTC (permalink / raw)
  To: Anupama K Patil, Jaroslav Kysela, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Adam, bkkarthik, gregkh, kernelnewbies,
	linux-kernel-mentees, Shuah Khan

On 4/22/21 12:03 PM, Anupama K Patil wrote:
> de, e are two variables of the type 'struct proc_dir_entry'
> which can be removed to save memory. This also fixes a coding style
> issue reported by checkpatch where we are suggested to make assignment
> outside the if statement.
> 

Sounds like a reasonable change.

> Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com>
> ---
>   drivers/pnp/isapnp/proc.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
> index 785a796430fa..1ae458c02656 100644
> --- a/drivers/pnp/isapnp/proc.c
> +++ b/drivers/pnp/isapnp/proc.c
> @@ -57,21 +57,20 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = {
>   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)) {
> +	if (!bus->procdir) {
>   		sprintf(name, "%02x", bus->number);

It would make sense to change this to scnprintf()

> -		de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
> -		if (!de)
> +		bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
> +		if (!bus->procdir)
>   			return -ENOMEM;
>   	}
>   	sprintf(name, "%02x", dev->number);

It would make sense to change this to scnprintf()

> -	e = dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, de,
> +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir,
>   					    &isapnp_proc_bus_proc_ops, dev);
> -	if (!e)
> +	if (!dev->procent)
>   		return -ENOMEM;

Shouldn't the procdir be deleted when proc_create_data() fails?

> -	proc_set_size(e, 256);
> +	proc_set_size(dev->procent, 256);
>   	return 0;
>   }
>   
> 

Note that isapnp_proc_init() doesn't check isapnp_proc_attach_device()
return and handle errors and cleanup.

thanks,
-- Shuah



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-23 21:08 ` Shuah Khan
@ 2021-04-26  4:57   ` Leon Romanovsky
  2021-04-26 12:00     ` Rafael J. Wysocki
  2021-04-28 12:12   ` Jaroslav Kysela
  1 sibling, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-04-26  4:57 UTC (permalink / raw)
  To: Shuah Khan
  Cc: Anupama K Patil, Jaroslav Kysela, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Adam, bkkarthik, gregkh, kernelnewbies,
	linux-kernel-mentees

On Fri, Apr 23, 2021 at 03:08:03PM -0600, Shuah Khan wrote:
> On 4/22/21 12:03 PM, Anupama K Patil wrote:
> > de, e are two variables of the type 'struct proc_dir_entry'
> > which can be removed to save memory. This also fixes a coding style
> > issue reported by checkpatch where we are suggested to make assignment
> > outside the if statement.
> > 
> 
> Sounds like a reasonable change.

It is unclear how much changes to ISA code are welcomed.
According to the Wikipedia, even Windows Vista disabled ISA PnP by default.
https://en.wikipedia.org/wiki/Legacy_Plug_and_Play#Specifications

> 
> > Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com>
> > ---
> >   drivers/pnp/isapnp/proc.c | 13 ++++++-------
> >   1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
> > index 785a796430fa..1ae458c02656 100644
> > --- a/drivers/pnp/isapnp/proc.c
> > +++ b/drivers/pnp/isapnp/proc.c
> > @@ -57,21 +57,20 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = {
> >   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)) {
> > +	if (!bus->procdir) {
> >   		sprintf(name, "%02x", bus->number);
> 
> It would make sense to change this to scnprintf()
> 
> > -		de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
> > -		if (!de)
> > +		bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
> > +		if (!bus->procdir)
> >   			return -ENOMEM;
> >   	}
> >   	sprintf(name, "%02x", dev->number);
> 
> It would make sense to change this to scnprintf()
> 
> > -	e = dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, de,
> > +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir,
> >   					    &isapnp_proc_bus_proc_ops, dev);
> > -	if (!e)
> > +	if (!dev->procent)
> >   		return -ENOMEM;
> 
> Shouldn't the procdir be deleted when proc_create_data() fails?

It needs but only if proc_mkdir() was called in this function.

Thanks

> 
> > -	proc_set_size(e, 256);
> > +	proc_set_size(dev->procent, 256);
> >   	return 0;
> >   }
> > 
> 
> Note that isapnp_proc_init() doesn't check isapnp_proc_attach_device()
> return and handle errors and cleanup.
> 
> thanks,
> -- Shuah
> 
> 
> 
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies@kernelnewbies.org
> https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-26  4:57   ` Leon Romanovsky
@ 2021-04-26 12:00     ` Rafael J. Wysocki
  2021-04-26 12:50       ` Leon Romanovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2021-04-26 12:00 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Shuah Khan, Anupama K Patil, Jaroslav Kysela, Rafael J. Wysocki,
	ACPI Devel Maling List, Linux Kernel Mailing List, Adam,
	bkkarthik, Greg Kroah-Hartman, kernelnewbies,
	linux-kernel-mentees

On Mon, Apr 26, 2021 at 6:57 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Apr 23, 2021 at 03:08:03PM -0600, Shuah Khan wrote:
> > On 4/22/21 12:03 PM, Anupama K Patil wrote:
> > > de, e are two variables of the type 'struct proc_dir_entry'
> > > which can be removed to save memory. This also fixes a coding style
> > > issue reported by checkpatch where we are suggested to make assignment
> > > outside the if statement.
> > >
> >
> > Sounds like a reasonable change.
>
> It is unclear how much changes to ISA code are welcomed.

Real fixes and obvious cleanups are, not much more than that.

> According to the Wikipedia, even Windows Vista disabled ISA PnP by default.
> https://en.wikipedia.org/wiki/Legacy_Plug_and_Play#Specifications

It is indeed unclear how many systems with this interface still run
Linux, but as long as the code is in the tree, there's nothing wrong
with attempting to improve it.  There's no assurance that all such
patches will be applied, though.

Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-26 12:00     ` Rafael J. Wysocki
@ 2021-04-26 12:50       ` Leon Romanovsky
  2021-04-26 17:52         ` bkkarthik
  0 siblings, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2021-04-26 12:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Shuah Khan, Anupama K Patil, Jaroslav Kysela, Rafael J. Wysocki,
	ACPI Devel Maling List, Linux Kernel Mailing List, Adam,
	bkkarthik, Greg Kroah-Hartman, kernelnewbies,
	linux-kernel-mentees

On Mon, Apr 26, 2021 at 02:00:58PM +0200, Rafael J. Wysocki wrote:
> On Mon, Apr 26, 2021 at 6:57 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Apr 23, 2021 at 03:08:03PM -0600, Shuah Khan wrote:
> > > On 4/22/21 12:03 PM, Anupama K Patil wrote:
> > > > de, e are two variables of the type 'struct proc_dir_entry'
> > > > which can be removed to save memory. This also fixes a coding style
> > > > issue reported by checkpatch where we are suggested to make assignment
> > > > outside the if statement.
> > > >
> > >
> > > Sounds like a reasonable change.
> >
> > It is unclear how much changes to ISA code are welcomed.
> 
> Real fixes and obvious cleanups are, not much more than that.

While first part is easy to determine, the second one is more blurry.

> 
> > According to the Wikipedia, even Windows Vista disabled ISA PnP by default.
> > https://en.wikipedia.org/wiki/Legacy_Plug_and_Play#Specifications
> 
> It is indeed unclear how many systems with this interface still run
> Linux, but as long as the code is in the tree, there's nothing wrong
> with attempting to improve it.  There's no assurance that all such
> patches will be applied, though.
> 
> Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-26 12:50       ` Leon Romanovsky
@ 2021-04-26 17:52         ` bkkarthik
  2021-04-27  4:18           ` Leon Romanovsky
  0 siblings, 1 reply; 9+ messages in thread
From: bkkarthik @ 2021-04-26 17:52 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Rafael J. Wysocki, Shuah Khan, Anupama K Patil, Jaroslav Kysela,
	Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List, Adam, Greg Kroah-Hartman,
	kernelnewbies, linux-kernel-mentees

On 21/04/26 03:50PM, Leon Romanovsky wrote:
> On Mon, Apr 26, 2021 at 02:00:58PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Apr 26, 2021 at 6:57 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Fri, Apr 23, 2021 at 03:08:03PM -0600, Shuah Khan wrote:
> > > > On 4/22/21 12:03 PM, Anupama K Patil wrote:
> > > > > de, e are two variables of the type 'struct proc_dir_entry'
> > > > > which can be removed to save memory. This also fixes a coding style
> > > > > issue reported by checkpatch where we are suggested to make assignment
> > > > > outside the if statement.
> > > > >
> > > >
> > > > Sounds like a reasonable change.
> > >
> > > It is unclear how much changes to ISA code are welcomed.

If changes to ISA code aren't welcomed, should these be marked obsolete in the MAINTIANERS file?

> > 
> > Real fixes and obvious cleanups are, not much more than that.
> 
> While first part is easy to determine, the second one is more blurry.
> 
> > 
> > > According to the Wikipedia, even Windows Vista disabled ISA PnP by default.
> > > https://en.wikipedia.org/wiki/Legacy_Plug_and_Play#Specifications

I wasn't aware until after this reply. Sorry about that!

thanks,

karthik

> > 
> > It is indeed unclear how many systems with this interface still run
> > Linux, but as long as the code is in the tree, there's nothing wrong
> > with attempting to improve it.  There's no assurance that all such
> > patches will be applied, though.
> > 
> > Thanks!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-26 17:52         ` bkkarthik
@ 2021-04-27  4:18           ` Leon Romanovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2021-04-27  4:18 UTC (permalink / raw)
  To: bkkarthik
  Cc: Rafael J. Wysocki, Shuah Khan, Anupama K Patil, Jaroslav Kysela,
	Rafael J. Wysocki, ACPI Devel Maling List,
	Linux Kernel Mailing List, Adam, Greg Kroah-Hartman,
	kernelnewbies, linux-kernel-mentees

On Mon, Apr 26, 2021 at 11:22:54PM +0530, bkkarthik wrote:
> On 21/04/26 03:50PM, Leon Romanovsky wrote:
> > On Mon, Apr 26, 2021 at 02:00:58PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Apr 26, 2021 at 6:57 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Fri, Apr 23, 2021 at 03:08:03PM -0600, Shuah Khan wrote:
> > > > > On 4/22/21 12:03 PM, Anupama K Patil wrote:
> > > > > > de, e are two variables of the type 'struct proc_dir_entry'
> > > > > > which can be removed to save memory. This also fixes a coding style
> > > > > > issue reported by checkpatch where we are suggested to make assignment
> > > > > > outside the if statement.
> > > > > >
> > > > >
> > > > > Sounds like a reasonable change.
> > > >
> > > > It is unclear how much changes to ISA code are welcomed.
> 
> If changes to ISA code aren't welcomed, should these be marked obsolete in the MAINTIANERS file?

I think so, but think that "Odd Fixes" better describes that Rafael wrote.

Thanks

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-23 21:08 ` Shuah Khan
  2021-04-26  4:57   ` Leon Romanovsky
@ 2021-04-28 12:12   ` Jaroslav Kysela
  1 sibling, 0 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2021-04-28 12:12 UTC (permalink / raw)
  To: Shuah Khan, Anupama K Patil, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Adam, bkkarthik, gregkh, kernelnewbies,
	linux-kernel-mentees

Dne 23. 04. 21 v 23:08 Shuah Khan napsal(a):
> On 4/22/21 12:03 PM, Anupama K Patil wrote:
>> de, e are two variables of the type 'struct proc_dir_entry'
>> which can be removed to save memory. This also fixes a coding style
>> issue reported by checkpatch where we are suggested to make assignment
>> outside the if statement.
>>
> 
> Sounds like a reasonable change.
> 
>> Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com>
>> ---
>>   drivers/pnp/isapnp/proc.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
>> index 785a796430fa..1ae458c02656 100644
>> --- a/drivers/pnp/isapnp/proc.c
>> +++ b/drivers/pnp/isapnp/proc.c
>> @@ -57,21 +57,20 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = {
>>   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)) {
>> +	if (!bus->procdir) {
>>   		sprintf(name, "%02x", bus->number);
> 
> It would make sense to change this to scnprintf()

The name is 16 bytes, so sprintf is safe here.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles
  2021-04-22 18:03 [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles Anupama K Patil
  2021-04-23 21:08 ` Shuah Khan
@ 2021-04-28 12:17 ` Jaroslav Kysela
  1 sibling, 0 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2021-04-28 12:17 UTC (permalink / raw)
  To: Anupama K Patil, Rafael J. Wysocki, linux-acpi, linux-kernel,
	skhan, Adam, bkkarthik, gregkh, kernelnewbies,
	linux-kernel-mentees

Dne 22. 04. 21 v 20:03 Anupama K Patil napsal(a):
> de, e are two variables of the type 'struct proc_dir_entry'
> which can be removed to save memory. This also fixes a coding style
> issue reported by checkpatch where we are suggested to make assignment
> outside the if statement.
> 
> Signed-off-by: Anupama K Patil <anupamakpatil123@gmail.com>

The change is straight without any functionality modification.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

> ---
>  drivers/pnp/isapnp/proc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pnp/isapnp/proc.c b/drivers/pnp/isapnp/proc.c
> index 785a796430fa..1ae458c02656 100644
> --- a/drivers/pnp/isapnp/proc.c
> +++ b/drivers/pnp/isapnp/proc.c
> @@ -57,21 +57,20 @@ static const struct proc_ops isapnp_proc_bus_proc_ops = {
>  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)) {
> +	if (!bus->procdir) {
>  		sprintf(name, "%02x", bus->number);
> -		de = bus->procdir = proc_mkdir(name, isapnp_proc_bus_dir);
> -		if (!de)
> +		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,
> +	dev->procent = proc_create_data(name, S_IFREG | S_IRUGO, bus->procdir,
>  					    &isapnp_proc_bus_proc_ops, dev);
> -	if (!e)
> +	if (!dev->procent)
>  		return -ENOMEM;
> -	proc_set_size(e, 256);
> +	proc_set_size(dev->procent, 256);
>  	return 0;
>  }
>  
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-28 12:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-22 18:03 [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles Anupama K Patil
2021-04-23 21:08 ` Shuah Khan
2021-04-26  4:57   ` Leon Romanovsky
2021-04-26 12:00     ` Rafael J. Wysocki
2021-04-26 12:50       ` Leon Romanovsky
2021-04-26 17:52         ` bkkarthik
2021-04-27  4:18           ` Leon Romanovsky
2021-04-28 12:12   ` Jaroslav Kysela
2021-04-28 12:17 ` Jaroslav Kysela

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).