[PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Anupama K Patil]

[-- 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 --]

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[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

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Leon Romanovsky]

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

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Rafael J. Wysocki]

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!

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Leon Romanovsky]

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!

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[bkkarthik]

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!

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Leon Romanovsky]

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

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Jaroslav Kysela]

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.

Re: [PATCH] drivers: pnp: proc.c: Removed unnecessary varibles

[Jaroslav Kysela]

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.