Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220329091126.4730-1-wsa+renesas@sang-engineering.com>
 <20220329091126.4730-2-wsa+renesas@sang-engineering.com> <ZHkQDTvk6I2q-9CF@surfacebook>
In-Reply-To: <ZHkQDTvk6I2q-9CF@surfacebook>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Fri, 2 Jun 2023 08:51:50 +0200
Message-ID: <CAMuHMdUaugQ5+Zhmg=oe=X2wvhazMiT=K-su0EJYKzD4Hdyn3Q@mail.gmail.com>
Subject: Re: [PATCH v8 1/1] gpio: add sloppy logic analyzer using polling
To:     andy.shevchenko@gmail.com
Cc:     Wolfram Sang <wsa+renesas@sang-engineering.com>,
        linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-renesas-soc@vger.kernel.org,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Greg KH <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

Hi Andy,

CC GregKH

On Thu, Jun 1, 2023 at 11:40 PM <andy.shevchenko@gmail.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@linux-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