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

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

On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule <sander@...nheule.net> wrote:
> On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule <
> > sander@...nheule.net> wrote:

...

> > > +#include <linux/swab.h>
> >
> > Not sure why you need this? See below.

> > > +       return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
> >
> > Why swab?! How is this supposed to work on BE CPUs?
> > Ditto for all swabXX() usage.
>
> My use of swab32/swahw32 has little to do with the CPU being BE or LE,
> but more with the register packing in the GPIO peripheral.
>
> The supported SoCs have port layout A-B-C-D in the registers, where
> firmware built with Realtek's SDK always denotes A0 as the first GPIO
> line. So bit 24 in a register has the value for A0 (with the exception
> of the IMR register).
>
> I wrote these wrapper functions to be able to use the BIT() macro with
> the GPIO line number, similar to how gpio-mmio uses ioread32be() when
> the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.
>
> For the IMR register, port A again comes first, but is now 16 bits wide
> instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
> this register.
>
> On the currently unsupported RTL9300-series, the port layout is
> reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
> functions won't be required. When support for this alternate port
> layout is added, some code will need to be added to differentiate
> between the two cases.

Yes, you have different endianess on the hardware level, why not to
use the proper accessors (with or without utilization of the above
mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)?

...

> > > +       case IRQ_TYPE_NONE:
> > > +               type = 0;
> > > +               handler = handle_bad_irq;
> > > +               break;
> >
> > Why is it here? Make it default like many other GPIO drivers do.

> > > +       irq_set_handler_locked(data, handler);
> >
> > handler is always the same. Use it directly here.
>
> I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that
> IRQ_TYPE_NONE should never be used as the new value, but only as the
> default initial value?

Initially you initialize the default handler to be "bad" (in order to
easily catch up issues with IRQ configurations).
When ->irq_set_type() is called, if everything is okay it will lock
the handler to the proper one.

-- 
With Best Regards,
Andy Shevchenko