Re: [PATCH RFC/RFT 1/1] misc: add simple logic analyzer using polling
From: Andy Shevchenko
Date: Tue Mar 30 2021 - 06:50:34 EST
On Tue, Mar 30, 2021 at 11:58 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
...
> +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