Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210330085655.12615-1-wsa+renesas@sang-engineering.com> <20210330085655.12615-2-wsa+renesas@sang-engineering.com>
In-Reply-To: <20210330085655.12615-2-wsa+renesas@sang-engineering.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 30 Mar 2021 13:49:32 +0300
Message-ID: <CAHp75VcVQJ6ezyHUc8TMd0qp453QgLL42N5GqWOy5oxrp5_qnQ@mail.gmail.com>
Subject: Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
To:     Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Mar 30, 2021 at 11:58 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> This is a simple logic analyzer using GPIO polling. It comes with a
> script to isolate a CPU for polling. While this is definately not a

definitely

> production level analyzer, it can be a helpful first view when remote
> debugging. Read the documentation for details.

I would like to look at it closer, but don't have time right now. So,
some kind of a shallow review.

But the idea is, let's say, interesting.

> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

...

> +The binding documentation is in the ``misc`` folder of the Kernel binding
> +documentation.

Can't you give a reference in terms of reST format?

...

> +Note that the process will return you back to the prompt but a sub-process is
> +still sampling in the background. Unless this finished, you will not find a

this is finished

> +result file in the current or specified directory. Please also note that
> +currently this sub-process is not killable! For the above example, we will then
> +need to trigger I2C communication:

Shouldn't you use :: instead of : to mark sections as code excerpts?

> +    i2cdetect -y -r <your bus number>

...

> +config GPIO_LOGIC_ANALYZER
> +       tristate "Simple GPIO logic analyzer"
> +       depends on GPIOLIB || COMPILE_TEST
> +       help
> +         This option enables support for a simple logic analyzer using polled
> +         GPIOs. Use the 'tools/debugging/gpio-logic-analyzer' script with this
> +         driver. The script will make using it easier and can also isolate a
> +         CPU for the polling task. Note that this is still a last resort
> +         analyzer which can be affected by latencies and non-determinant code
> +         paths. However, for e.g. remote development, it may be useful to get
> +         a first view and aid further debugging.

Module name?

...

> +#include <linux/of.h>

Can you switch to use device property API?

...

> +#define GPIO_LA_NAME "gpio-logic-analyzer"
> +#define GPIO_LA_DEFAULT_BUF_SIZE SZ_256K

> +/* can be increased if needed */
> +#define GPIO_LA_MAX_PROBES 8
> +#define GPIO_LA_PROBES_MASK 7

Does this assume the power-of-two number of probes?
Perhaps using BIT(x) and (BIT(x) - 1) will clarify that.

...

> +struct gpio_la_poll_priv {
> +       unsigned long ndelay;
> +       u32 buf_idx;
> +       struct mutex lock;
> +       struct debugfs_blob_wrapper blob;
> +       struct gpio_descs *descs;
> +       struct dentry *debug_dir, *blob_dent;
> +       struct debugfs_blob_wrapper meta;
> +       unsigned long gpio_delay;
> +       unsigned int trigger_len;

> +       u8 trigger_data[PAGE_SIZE];

This is not good for fragmentation (basically you make your struct to
occupy 2 pages, one of which will be almost wasted). Better to have a
pointer here and allocate one page by get_zero_page() or so.

> +};
> +
> +static struct dentry *gpio_la_poll_debug_dir;
> +
> +static int fops_capture_set(void *data, u64 val)
> +{
> +       struct gpio_la_poll_priv *priv = data;
> +       u8 *la_buf = priv->blob.data;
> +       unsigned long state = 0;
> +       int i, ret;
> +
> +       if (!la_buf)
> +               return -ENOMEM;

> +       if (val) {

if (!val)
  return 0;

makes your life easier.

> +               mutex_lock(&priv->lock);
> +               if (priv->blob_dent) {
> +                       debugfs_remove(priv->blob_dent);
> +                       priv->blob_dent = NULL;
> +               }
> +
> +               priv->buf_idx = 0;
> +
> +               local_irq_disable();
> +               preempt_disable_notrace();
> +
> +               for (i = 0; i < priv->trigger_len; i++) {
> +                       u8 data = priv->trigger_data[i];
> +                       do {
> +                               ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
> +                                                           priv->descs->info, &state);
> +
> +                               if (ret)
> +                                       goto gpio_err;
> +                       } while (!!(state & BIT(data & GPIO_LA_PROBES_MASK)) != !!(data & 0x80));
> +               }
> +
> +               if (priv->trigger_len) {
> +                       la_buf[priv->buf_idx++] = state;
> +                       ndelay(priv->ndelay);
> +               }
> +
> +               while (priv->buf_idx < priv->blob.size && ret == 0) {
> +                       ret = gpiod_get_array_value(priv->descs->ndescs, priv->descs->desc,
> +                                             priv->descs->info, &state);
> +                       la_buf[priv->buf_idx++] = state;
> +                       ndelay(priv->ndelay);
> +               }
> +gpio_err:
> +               preempt_enable_notrace();
> +               local_irq_enable();
> +               if (ret)

> +                       pr_err("%s: couldn't read GPIOs: %d\n", __func__, ret);

Haven't noticed if you are using pr_fmt(). It may be better than using __func__.

Btw, it seems you have a struct device for that or so. Why don't you
use dev_err()?

> +               priv->blob_dent = debugfs_create_blob("sample_data", 0400, priv->debug_dir, &priv->blob);
> +               mutex_unlock(&priv->lock);
> +       }
> +
> +       return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_capture, NULL, fops_capture_set, "%llu\n");


> +static ssize_t trigger_write(struct file *file, const char __user *ubuf,
> +                            size_t count, loff_t *offset)
> +{
> +       struct seq_file *m = file->private_data;
> +       struct gpio_la_poll_priv *priv = m->private;
> +       char *buf;
> +       int i, trigger_len = 0;
> +
> +       priv->trigger_len = 0;
> +
> +       if (count & 1)
> +           return -EINVAL;
> +
> +       buf = memdup_user(ubuf, count);
> +       if (IS_ERR(buf))
> +               return PTR_ERR(buf);
> +
> +       for (i = 0; i < count; i += 2) {
> +               u8 val;
> +
> +               if (buf[i] < '1' || buf[i] > '0' + GPIO_LA_MAX_PROBES)

So, you can't increase the amount of probes without breaking this
entire parser (it will go somewhere to symbols and letters...).

> +                       goto bail_out;
> +
> +               val = buf[i] - '1';
> +
> +               switch (toupper(buf[i + 1])) {
> +               case 'L':
> +                       priv->trigger_data[trigger_len] = val;
> +                       trigger_len++;
> +                       break;
> +               case 'H':
> +                       priv->trigger_data[trigger_len] = val | 0x80;
> +                       trigger_len++;
> +                       break;
> +               case 'R':
> +                       priv->trigger_data[trigger_len] = val;
> +                       priv->trigger_data[trigger_len + 1] = val | 0x80;
> +                       trigger_len += 2;
> +                       break;
> +               case 'F':
> +                       priv->trigger_data[trigger_len] = val | 0x80;
> +                       priv->trigger_data[trigger_len + 1] = val;
> +                       trigger_len += 2;
> +                       break;
> +               default:
> +                       goto bail_out;
> +               }
> +
> +               if (trigger_len > PAGE_SIZE)    /* should never happen */

Shouldn't you return OVERFLOW here or something like that?

> +                       goto bail_out;
> +
> +       }
> +
> +       priv->trigger_len = trigger_len;
> +
> +bail_out:
> +       kfree(buf);
> +       return priv->trigger_len ? count : -EINVAL;
> +}

I'm not a fan of yet another parser in the kernel. Can you provide a
bit of description of the format?

...

> +       priv->debug_dir = debugfs_create_dir(dev_name(dev), gpio_la_poll_debug_dir);

> +       if (IS_ERR(priv->debug_dir))
> +               return PTR_ERR(priv->debug_dir);

Shouldn't be checked AFAIU.

...

> +static const struct of_device_id gpio_la_poll_of_match[] = {
> +       { .compatible = GPIO_LA_NAME, },

> +       { },

No comma needed.

> +};
> +MODULE_DEVICE_TABLE(of, gpio_la_poll_of_match);

...

> +       -t|--trigger <str>: pattern to use as trigger. <str> consists of n two-char pairs. First
> +                           char is channel number starting at "1". Second char is trigger level:
> +                           "L" - low; "H" - high; "R" - rising; "F" - falling
> +Examples:
> +Samples $NUMSAMPLES at 1MHz with already prepared CPU or automatically prepare CPU1 if needed

with an already
prepares

> +       '$0'
> +Samples 50us at 2MHz waiting for falling edge on channel 2. CPU usage as above.
> +       '$0 -d 50 -s 2000000 -t "2F"'
> +
> +Note that the process exits after checking all parameters but a sub-process still works in
> +the background. The result is only available once the sub-process finished. As the time of

finishes

> +writing, the sub-process is not killable, so be extra careful that your triggers work.
> +
> +Result is a .sr file to be consumed with PulseView from the free Sigrok project. It is
> +a zip file which also contains the binary sample data which may be consumed by others.
> +The filename is the logic analyzer instance name plus a since-epoch timestamp.
> +EOF
> +}

-- 
With Best Regards,
Andy Shevchenko