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
| ||
|
Date: Fri, 25 Sep 2020 20:26:51 +0800 From: Kent Gibson <warthog618@...il.com> To: Andy Shevchenko <andy.shevchenko@...il.com> Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>, Bartosz Golaszewski <bgolaszewski@...libre.com>, Linus Walleij <linus.walleij@...aro.org>, Arnd Bergmann <arnd@...db.de> Subject: Re: [PATCH v9 09/20] gpiolib: cdev: support edge detection for uAPI v2 On Fri, Sep 25, 2020 at 12:35:49PM +0300, Andy Shevchenko wrote: > On Thu, Sep 24, 2020 at 6:07 AM Kent Gibson <warthog618@...il.com> wrote: > > On Wed, Sep 23, 2020 at 06:47:28PM +0300, Andy Shevchenko wrote: > > > On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@...il.com> wrote: > > ... > > > Also, this code is drawn from lineevent_irq_thread(), which is ordered > > this way. > > Negative conditionals are slightly harder to read. > > ... > > > > > + if (!line->timestamp_ns) { > > > > + le.timestamp_ns = ktime_get_ns(); > > > > + if (lr->num_lines != 1) > > > > + line->req_seqno = atomic_inc_return(&lr->seqno); > > > > + } else { > > > > + le.timestamp_ns = line->timestamp_ns; > > > > > + } > > > > > > Ditto. > > > > Firstly, drawn from lineevent_irq_thread() which is structured this way. > > > > In this case the comment relates to the condition being true, so > > re-ordering the if/else would be confusing - unless the comment were > > moved into the corresponding body?? > > Yes. > Does that mean I should re-order and move the comment into the body? That would work for me - the normal case is line->timestamp_ns being set. > ... > > > > > + irq = gpiod_to_irq(line->desc); > > > > + if (irq <= 0) > > > > + return -ENODEV; > > > > > > So, you mean this is part of ABI. Can we return more appropriate code, > > > because getting no IRQ doesn't mean we don't have a device. > > > Also does 0 case have the same meaning? > > > > Firstly, this code is drawn from lineevent_create(), so any changes > > here should be considered for there as well - though this may > > constitute an ABI change?? > > For v1 probably, for v2 we are free to fix this. > > > I agree ENODEV doesn't seem right here. Are you ok with ENXIO? > > Yes. > Will do. And in the debounce patch as well. > > From gpiod_to_irq(): > > > > /* Zero means NO_IRQ */ > > if (!retirq) > > return -ENXIO; > > > > so it can't even return a 0 :-| - we're just being cautious. > > I would drop = part then. > ok, but you'd better not come after me in a subsequent review for not checking the 0 case! Cheers, Kent.
Powered by blists - more mailing lists