linux-kernel - Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support

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]

Message-ID: <537a2becc81360f314a4293f7bb619ed2a377cb6.camel@svanheule.net>
Date:   Mon, 29 Mar 2021 19:28:28 +0200
From:   Sander Vanheule <sander@...nheule.net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     devicetree <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Bert Vermeulen <bert@...t.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v4 2/2] gpio: Add Realtek Otto GPIO support

Hi Andy,

Thank you for clarifying your remarks. I'll support for building as a
module, and have implemented the gpio_irq_chip->init_hw() callback.

On Mon, 2021-03-29 at 13:26 +0300, Andy Shevchenko wrote:
> On Fri, Mar 26, 2021 at 11:11 PM Sander Vanheule <
> sander@...nheule.net> wrote:
> > On Fri, 2021-03-26 at 20:19 +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 26, 2021 at 2:05 PM Sander Vanheule <
> > > sander@...nheule.net>
> > > wrote:
> > > > +static const struct of_device_id realtek_gpio_of_match[] = {
> > > > +       { .compatible = "realtek,otto-gpio" },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8380-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Not sure why this flag is needed right now. Drop it completely for
> > > good.
> > > > +       },
> > > > +       {
> > > > +               .compatible = "realtek,rtl8390-gpio",
> > > > +               .data = (void *)GPIO_INTERRUPTS
> > > 
> > > Ditto
> > 
> > Linus Walleij asked this question too after v1:
> > https://lore.kernel.org/linux-gpio/e9f0651e5fb52b7d56361ceb30b41759b6f2ec13.camel@svanheule.net/
> > 
> > Note that the fall-back compatible doesn't have this flag set.
> 
> AFAICS all, except one have this flag, I suggest you to do other way
> around, i.e. check compatible string in the code. Or do something more
> clever. What happens if you have this flag enabled for the fallback
> node?
> 
> If two people ask the same, it might be a smoking gun.
> 

Testing for the fallback wouldn't work, since of_device_is_compatible()
would always match. Setting the (inverse) flag only on the fallback
would indeed reduce the clutter.

If the port order is reversed w.r.t. to the current implementation,
enabling a GPIO+IRQ would enable the same pin on a different port. I
don't think the result would be catastrophical, but it would result in
unexpected behaviour. When A0 and C0 are then enabled, A0 interrupts
would actually come from C0, and vice versa.

   Intended port | A | B | C | D
-----------------+---+---+---+---
Actual GPIO port | D | C | B | A
 Actual IRQ port | B | A | D | C

If only the actual GPIO ports change, at least you can still use a
modified GPIO line number and polling. The user could just leave out
the optional irq-controller from the devicetree, but I would rather
have it enforced in some way.


Best,
Sander