| Message ID | 20210422180322.7wlyg63kv3n2k6id@ubuntu (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | drivers: pnp: proc.c: Removed unnecessary varibles | expand |
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
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
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!
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!
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!
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
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
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; > } > >
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; }
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(-)