Re: [PATCH v8 1/1] gpio: add sloppy logic analyzer using polling

[Greg KH]

On Fri, Jun 02, 2023 at 08:51:50AM +0200, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> CC GregKH
> 
> On Thu, Jun 1, 2023 at 11:40 PM <andy.shevchenko@xxxxxxxxx> wrote:
> > Tue, Mar 29, 2022 at 11:11:26AM +0200, Wolfram Sang kirjoitti:
> > > This is a sloppy logic analyzer using GPIOs. It comes with a script to
> > > isolate a CPU for polling. While this is definitely not a production
> > > level analyzer, it can be a helpful first view when remote debugging.
> > > Read the documentation for details.
> >
> > One note since I have done recent review and realize one issue with debugfs.
> >
> > ...
> >
> > > +     priv->debug_dir = debugfs_create_dir(devname, gpio_la_poll_debug_dir);
> >
> > If this fails with NULL...
> >
> > > +     debugfs_create_blob("meta_data", 0400, priv->debug_dir, &priv->meta);
> > > +     debugfs_create_ulong("delay_ns", 0600, priv->debug_dir, &priv->delay_ns);
> > > +     debugfs_create_ulong("delay_ns_acquisition", 0400, priv->debug_dir, &priv->acq_delay);
> > > +     debugfs_create_file_unsafe("buf_size", 0600, priv->debug_dir, priv, &fops_buf_size);
> > > +     debugfs_create_file_unsafe("capture", 0200, priv->debug_dir, priv, &fops_capture);
> > > +     debugfs_create_file_unsafe("trigger", 0200, priv->debug_dir, priv, &fops_trigger);
> >
> > ...and any of these is not, we will end up with the file in a root folder of debugfs...
> >
> > > +     dev_info(dev, "initialized");

Nit, please remove this line.  Drivers are quiet when they work
properly, don't add to a mess in the kernel log.

> > ...
> >
> > > +static int gpio_la_poll_remove(struct platform_device *pdev)
> > > +{
> > > +     struct gpio_la_poll_priv *priv = platform_get_drvdata(pdev);
> > > +
> > > +     mutex_lock(&priv->lock);
> > > +     debugfs_remove_recursive(priv->debug_dir);
> >
> > ...and this one won't remove it.
> >
> > > +     mutex_unlock(&priv->lock);
> > > +     mutex_destroy(&priv->lock);
> > > +
> > > +     return 0;
> > > +}
> >
> > ...
> >
> > However, I haven't checked if it's pure theoretical issue with the current code
> > base of debugfs or a potential problem. Easy fix is to check an error code and
> 
> I think debugfs_create_dir() can only fail reasonably due to OOM.
> 
> > skip the files creation. Not sure if driver will be useful in that case.
> 
> Having to add such error checks would really be unfortunate, because
> one of the design principles of debugfs is that there is never a need
> to check for errors.

If you really want, you can check the return value of the directory
creation and just return and keep going forward (do NOT propagate an
error upwards as drivers need to keep working if debugfs is hosed).

But really, the only way the call can fail is if you did something wrong
and tried to create a directory that was already there, so don't even
worry about checking the return value, all is fine.

thanks,

greg k-h