| 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
| ||
|
Message-ID: <ZmcN6lJAGVxY3Ok2@surfacebook.localdomain>
Date: Mon, 10 Jun 2024 17:30:02 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Wolfram Sang <wsa+renesas@...g-engineering.com>
Cc: linux-renesas-soc@...r.kernel.org, Jonathan Corbet <corbet@....net>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Kent Gibson <warthog618@...il.com>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v9 1/1] gpio: add sloppy logic analyzer using polling
Mon, Jun 10, 2024 at 01:26:59PM +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.
...
> +#include <linux/ctype.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
+ device.h
+ err.h
> +#include <linux/gpio/consumer.h>
> +#include <linux/init.h>
> +#include <linux/ktime.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sizes.h>
> +#include <linux/timekeeping.h>
+ types.h
> +#include <linux/vmalloc.h>
...
> +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; /* zeroed because GPIO arrays are bitfields */
> + unsigned long delay;
> + ktime_t start_time;
> + unsigned int i;
> + int ret;
> +
> + if (!val)
> + return 0;
> +
> + if (!la_buf)
> + return -ENOMEM;
> +
> + if (!priv->delay_ns)
> + return -EINVAL;
> + mutex_lock(&priv->blob_lock);
guard() (from cleanup.h)?
> + if (priv->blob_dent) {
> + debugfs_remove(priv->blob_dent);
> + priv->blob_dent = NULL;
> + }
> +
> + priv->buf_idx = 0;
> +
> + local_irq_disable();
> + preempt_disable_notrace();
> +
> + /* Measure delay of reading GPIOs */
> + start_time = ktime_get();
> + for (i = 0; i < GPIO_LA_NUM_TESTS; i++) {
> + ret = gpio_la_get_array(priv->descs, &state);
> + if (ret)
> + goto out;
> + }
> +
> + priv->acq_delay = ktime_sub(ktime_get(), start_time) / GPIO_LA_NUM_TESTS;
> + if (priv->delay_ns < priv->acq_delay) {
> + ret = -ERANGE;
> + goto out;
> + }
> +
> + delay = priv->delay_ns - priv->acq_delay;
> +
> + /* Wait for triggers */
> + for (i = 0; i < priv->trig_len; i += 2) {
> + do {
> + ret = gpio_la_get_array(priv->descs, &state);
> + if (ret)
> + goto out;
> +
> + ndelay(delay);
> + } while ((state & priv->trig_data[i]) != priv->trig_data[i + 1]);
> + }
> +
> + /* With triggers, final state is also the first sample */
> + if (priv->trig_len)
> + la_buf[priv->buf_idx++] = state;
> +
> + /* Sample */
> + while (priv->buf_idx < priv->blob.size) {
> + ret = gpio_la_get_array(priv->descs, &state);
> + if (ret)
> + goto out;
> +
> + la_buf[priv->buf_idx++] = state;
> + ndelay(delay);
> + }
> +out:
> + preempt_enable_notrace();
> + local_irq_enable();
> + if (ret)
> + dev_err(priv->dev, "couldn't read GPIOs: %d\n", ret);
> +
> + kfree(priv->trig_data);
> + priv->trig_data = NULL;
> + priv->trig_len = 0;
> +
> + priv->blob_dent = debugfs_create_blob("sample_data", 0400, priv->debug_dir, &priv->blob);
> + mutex_unlock(&priv->blob_lock);
> +
> + return ret;
> +}
...
> +static const struct file_operations fops_trigger = {
> + .owner = THIS_MODULE,
> + .open = trigger_open,
> + .write = trigger_write,
> + .llseek = no_llseek,
> + .release = single_release,
> +};
Wondering if you can use DEFINE_SHOW_STORE_ATTRIBUTE(), or if it makes sense.
It might be that it requires to use DEFINE_SHOW_ATTRIBUTE() for the sake of
consistency, but I don't remember if there is a difference WRT debugfs usage.
...
> +static int gpio_la_poll_probe(struct platform_device *pdev)
> +{
> + struct gpio_la_poll_priv *priv;
> + struct device *dev = &pdev->dev;
> + const char *devname = dev_name(dev);
> + const char *gpio_names[GPIO_LA_MAX_PROBES];
> + char *meta = NULL;
> + unsigned int i, meta_len = 0;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + mutex_init(&priv->blob_lock);
devm_mutex_init()
> + fops_buf_size_set(priv, GPIO_LA_DEFAULT_BUF_SIZE);
> +
> + priv->descs = devm_gpiod_get_array(dev, "probe", GPIOD_IN);
> + if (IS_ERR(priv->descs))
> + return PTR_ERR(priv->descs);
> +
> + /* artificial limit to keep 1 byte per sample for now */
> + if (priv->descs->ndescs > GPIO_LA_MAX_PROBES)
> + return -EFBIG;
> +
> + ret = device_property_read_string_array(dev, "probe-names", gpio_names,
> + priv->descs->ndescs);
> + if (ret >= 0 && ret != priv->descs->ndescs)
> + ret = -EBADR;
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "error naming the GPIOs");
> +
> + for (i = 0; i < priv->descs->ndescs; i++) {
> + unsigned int add_len;
> + char *new_meta, *consumer_name;
> +
> + if (gpiod_cansleep(priv->descs->desc[i]))
> + return -EREMOTE;
> +
> + consumer_name = kasprintf(GFP_KERNEL, "%s: %s", devname, gpio_names[i]);
> + if (!consumer_name)
> + return -ENOMEM;
> + gpiod_set_consumer_name(priv->descs->desc[i], consumer_name);
> + kfree(consumer_name);
> +
> + /* '10' is length of 'probe00=\n\0' */
> + add_len = strlen(gpio_names[i]) + 10;
> +
> + new_meta = devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL);
Can it be rewritten to use devm_krealloc_array()?
> + if (!new_meta)
> + return -ENOMEM;
> +
> + meta = new_meta;
> + meta_len += snprintf(meta + meta_len, add_len, "probe%02u=%s\n",
> + i + 1, gpio_names[i]);
> + }
> +
> + platform_set_drvdata(pdev, priv);
> + priv->dev = dev;
> +
> + priv->meta.data = meta;
> + priv->meta.size = meta_len;
> + priv->debug_dir = debugfs_create_dir(devname, gpio_la_poll_debug_dir);
> + 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);
> + dev_info(dev, "initialized");
Do we need this? Existence of folder in debugfs is enough indication of
success, no?
> + return 0;
> +}
...
> +static const struct of_device_id gpio_la_poll_of_match[] = {
> + { .compatible = GPIO_LA_NAME, },
Redundant inner comma.
> + { }
> +};
...
> +static int __init gpio_la_poll_init(void)
> +{
> + gpio_la_poll_debug_dir = debugfs_create_dir(GPIO_LA_NAME, NULL);
> +
> + return platform_driver_register(&gpio_la_poll_device_driver);
> +}
> +late_initcall(gpio_la_poll_init);
Why? Can we add a comment?
...
Btw, have you tried `shellcheck` against your script?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists