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

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: <CAMuHMdUaugQ5+Zhmg=oe=X2wvhazMiT=K-su0EJYKzD4Hdyn3Q@mail.gmail.com>
Date:   Fri, 2 Jun 2023 08:51:50 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     andy.shevchenko@...il.com
Cc:     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>,
        Greg KH <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v8 1/1] gpio: add sloppy logic analyzer using polling

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");
>
> ...
>
> > +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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds