lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2023060209-scouts-affection-f54d@gregkh>
Date:   Fri, 2 Jun 2023 08:40:45 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     andy.shevchenko@...il.com,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v8 1/1] gpio: add sloppy logic analyzer using polling

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@...il.com> 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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ