Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices

[Leon Romanovsky]

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: 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  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

___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Device driver .shutdown() VS .remove()

[Luca Ceresoli]

Hi Jim,

thanks for the discussion.

On 22/04/21 20:05, jim.cro...@gmail.com wrote:
> 
> 
> On Thu, Apr 22, 2021 at 1:37 AM Luca Ceresoli  > wrote:
> 
> Hello,
> 
> despite having been searching for documentation I couldn't find out the
> exact and detailed difference between the .shutdown() and .remove()
> calls in struct device_driver.
> 
> 
> so, I'll start by saying I know next to nothing, but most of what you
> said sounds good

Good to know.

> From the above it looks like the shutdown() actions must be a subset of
> remove() actions.
> 
> 
> but subset is a bit vague.
> theres 2 dimensions to think about.
> lifetime -  shutdown/startup is surely the longer window, add/remove
> happen within that
> hierarchy - subsystems must be ready to handle add / remove
> 
> fwiw, I 'grepped' (with ack), it shows a few drivers with both functions,
> many others with just remove
> you could see in detail what the difference is.
> 
> $> ack '(\.remove|\.shutdown)\b' drivers/
>  
> drivers/rtc/rtc-twl.c
> 645: .remove = twl_rtc_remove,
> 646: .shutdown = twl_rtc_shutdown,

I've been looking at a few of the drivers that implement both calls, and
they either look similar or they call the shutdown function within the
remove function. This reinforces the idea that remove() should do the
same things as shutdown() plus free all kernel resources.

The only exception that I noticed happen for devices that don't have to
be completely turned off during shutdown or reboot, such as RTCs.

-- 
Luca


___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

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  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!

___
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: [PATCH] drivers: pnp: proc.c: Handle errors while attaching devices

[Barnabás Pőcze]

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