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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 2 Aug 2020 11:31:58 +0800 From: Kent Gibson <warthog618@...il.com> To: Bartosz Golaszewski <bgolaszewski@...libre.com> Cc: Andy Shevchenko <andy.shevchenko@...il.com>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, Linus Walleij <linus.walleij@...aro.org> Subject: Re: [PATCH v2 05/18] gpiolib: cdev: support GPIO_GET_LINE_IOCTL and GPIOLINE_GET_VALUES_IOCTL On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@...il.com> wrote: > > > > [snip] > > > > > > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > > > +{ > > > > + int i, sum = 0; > > > > + > > > > + for (i = 0; i < pad_size; i++) > > > > + sum |= padding[i]; > > > > + > > > > + return sum; > > > > +} > > > > > > Reimplementation of memchr_inv() ? > > > > > > > I was hoping to find an existing function, surely checking a region is > > zeroed is a common thing, right?, so this was a place holder as much > > as anything. Not sure memchr_inv fits the bill, but I'll give it a > > try... > > > > If you don't find an appropriate function: please put your new > implementation in lib/ so that others may reuse it. > Changed to memchr_inv. > > > ... > > > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > > > Much better to read is > > > > > > unsigned int i = lc->num_attrs; > > > > > > while (i--) { > > > ... > > > } > > > > > > > Really? I find that the post-decrement in the while makes determining the > > bounds of the loop more confusing. > > > > Agreed, Andy: this is too much nit-picking. :) > I was actually hoping for some feedback on the direction of that loop, as it relates to the handling of multiple instances of the same attribute associated with a given line. The reverse loop here implements a last in wins policy, but I'm now thinking the kernel should be encouraging userspace to only associate a given attribute with a line once, and that a first in wins would help do that - as additional associations would be ignored. Alternatively, the kernel should enforce that an attribute can only be associated once, but that would require adding more request validation. > [snip] > > > > ... > > > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); > > > > > > I prefer to see this split, but it's minor. > > > > > > > + if (IS_ERR(desc)) { > > > > + ret = PTR_ERR(desc); > > > > + goto out_free_line; > > > > + } > > > > > > ... > > > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > > + offset); > > > > > > Perhaps tracepoint / event? > > > > > > > Again, a cut-and-paste from V1, and I have no experience with > > tracepoints or events, so I have no opinion on that. > > > > So, yeah - perhaps? > > > > I think it's a good idea to add some proper instrumentation this time > other than much less reliable logs. Can you take a look at > include/trace/events/gpio.h? Adding new GPIO trace events should be > pretty straightforward by copy-pasti... drawing inspiration from > existing ones. > You only want tracepoints to replace those dev_dbg()s, so when a line is requested? What about the release? Any other points? Cheers, Kent.
Powered by blists - more mailing lists