linux-kernel - Re: [PATCH v5 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdYTor-c2qvE=6YD4A+NmvpLgS3LsOfNpBZ5EdTrDkGgkg@mail.gmail.com>
Date:   Wed, 19 May 2021 01:50:39 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Cc:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH v5 1/2] gpio: Add support for IDT 79RC3243x GPIO controller

Hi Thomas,

thanks for your patch!

I can see this is starting to look really good.

There is one thing that confuses me:

On Fri, May 14, 2021 at 2:33 PM Thomas Bogendoerfer
<tsbogend@...ha.franken.de> wrote:

> IDT 79RC3243x SoCs integrated a gpio controller, which handles up
> to 32 gpios. All gpios could be used as an interrupt source.
>
> Signed-off-by: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> ---
> Changes in v5:
(...)

> +static int idt_gpio_irq_set_type(struct irq_data *d, unsigned int flow_type)
> +{
(...)
> +       /* hardware only supports level triggered */
> +       if (sense == IRQ_TYPE_NONE || (sense & IRQ_TYPE_EDGE_BOTH))
> +               return -EINVAL;
(...)
> +       irq_set_handler_locked(d, handle_level_irq);

But:

> +static void idt_gpio_ack(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct idt_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> +
> +       writel(~BIT(d->hwirq), ctrl->gpio + IDT_GPIO_ISTAT);
> +}
(...)
> +       .irq_ack = idt_gpio_ack,

Correct me if I'm wrong but I thing .irq_ack() is only called
from handle_edge_irq ... so never in this case.

Can this ACK just be deleted?

The code in the ACK callback also looks really weird:
write all bits except for the current IRQ into the status
register? It's usually the other way around with these
things. That really makes me suspect it is unused.

Yours,
Linus Walleij