linux-kernel - Re: [PATCH v9 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]

Date: Wed, 12 Jun 2024 12:03:31 +0200
From: Wolfram Sang <wsa+renesas@...g-engineering.com>
To: Andy Shevchenko <andy.shevchenko@...il.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

Hi Andi,


> > +#include <linux/delay.h>
> 
> + device.h
> + err.h

OK about the includes.

> > +	mutex_lock(&priv->blob_lock);
> 
> guard() (from cleanup.h)?

If you insist. I doesn't save an exit path, so I don't think this will
improve readability of the code. But I don't mind...

> > +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.

Well, then we can just leave it for now and change it later, if desired.

> > +	mutex_init(&priv->blob_lock);
> 
> devm_mutex_init()

OK.

> > +		new_meta = devm_krealloc(dev, meta, meta_len + add_len, GFP_KERNEL);
> 
> Can it be rewritten to use devm_krealloc_array()?

'meta' is not an array?

> > +	dev_info(dev, "initialized");
> 
> Do we need this? Existence of folder in debugfs is enough indication of
> success, no?

Since the script can now list instances easily, this can be argued.
Still, I don't think this matters much for a debug-only device.

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

Yes.

> > +late_initcall(gpio_la_poll_init);
> 
> Why? Can we add a comment?

Sure.

> Btw, have you tried `shellcheck` against your script?

We did this in one of the 8 previous iterations.

All the best,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)