linux-kernel - Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

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:   Sat, 24 Apr 2021 14:35:37 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

On Sat, Apr 24, 2021 at 1:47 PM Thomas Bogendoerfer
<tsbogend@...ha.franken.de> wrote:
> On Fri, Apr 23, 2021 at 06:37:41PM +0300, Andy Shevchenko wrote:
> > On Thu, Apr 22, 2021 at 6:21 PM Thomas Bogendoerfer
> > <tsbogend@...ha.franken.de> wrote:

...

> > > +               virq = irq_linear_revmap(gc->irq.domain, bit);
> >
> > Is it guaranteed to be linear always?
>
> yes

OK.

...

> > > +       if (sense & ~(IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> >
> > There is a _BOTH variant.
>
> that's IRQ_TYPE_EDGE_BOTH. LEVEL_BOTH would be an interesing concept.

Sorry, I meant _MASK in case of level. No need to open code the
existing definition.

> > > +       ilevel = readl(ctrl->gpio + IDT_GPIO_ILEVEL);
> > > +       if (sense & IRQ_TYPE_LEVEL_HIGH)
> > > +               ilevel |= BIT(d->hwirq);
> > > +       else if (sense & IRQ_TYPE_LEVEL_LOW)
> > > +               ilevel &= ~BIT(d->hwirq);
> >
> > > +       else
> > > +               return -EINVAL;
> >
> > Is it a double check of the above?
>
> no, the above test is for anything not LEVEL and this now takes care
> to be at least LEVEL_LOW or LEVEL_HIGH. This doesn't check for LOW|HIGH,
> which I assumed nobody tries to set...

And? Seems you have it as a dead code.
In your case HIGH is the winner anyway.

...

> > > +       /* Mask interrupts. */
> > > +       ctrl->mask_cache = 0xffffffff;
> > > +       writel(ctrl->mask_cache, ctrl->pic + IDT_PIC_IRQ_MASK);
> >
> > What about using ->init_hw() call back?
>
> sure, doesn't look like it's worth the effort, but I changed it.

The problem here (which you may not notice from day 1) is the
ordering. We carefully put the ->init_hw() call in the proper place
and time.

...

> > > +       girq->handler = handle_level_irq;
> >
> > handle_bad_irq()
>
> the hardware only supports level interrupts. That's also why there is
> no handler change in idt_gpio_irq_set_type.

After I fixed a nasty issue in the pca953x related to Intel Galileo
platform, I may tell that setting the handler here is equal to putting
a mine for the future blown. When you set it in BAD here it will
reveal issues, if any, sooner than later.

-- 
With Best Regards,
Andy Shevchenko