netdev - RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ 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: <CH2PR12MB3895D5E16EAA1D3E5796C177D7DC9@CH2PR12MB3895.namprd12.prod.outlook.com>
Date:   Thu, 16 Sep 2021 15:48:51 +0000
From:   Asmaa Mnebhi <asmaa@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        David Thompson <davthompson@...dia.com>
Subject: RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support

> +	/* Enable PHY interrupt by setting the priority level */

This should be an abstract driver for a collection of GPIO lines.
Yes, one of these GPIOs is used for the PHY, but the GPIO driver does not care. So please remove this comment.
Asmaa>> Done

> +	val = readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> +	val |= BIT(offset);
> +	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);

What exactly does this do? It appears to clear the interrupt, if i understand mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough to know if this is correct. It does mean if the interrupt signal is active but masked, and you enable it, you appear to loose the interrupt? Maybe you want the interrupt to fire as soon as it is enabled?

Asmaa>>
YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. Otherwise, we will not receive further interrupts.
YU_GPIO_CAUSE_OR_EVTEN0 - All interrupts are disabled by default. This register is what actually unmasks/enables the specific interrupt to start "firing".

> +static void mlxbf2_gpio_irq_mask(struct irq_data *irqd) {
> +	mlxbf2_gpio_irq_disable(irqd);
> +}
> +
> +static void mlxbf2_gpio_irq_unmask(struct irq_data *irqd) {
> +	mlxbf2_gpio_irq_enable(irqd);
> +}

Do these two functions have any value?

Asmaa>>
This code is actually not being called. enable/disable is what's being called. So I will remove it.

> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +	case IRQ_TYPE_LEVEL_MASK:
> +		fall = true;
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		rise = true;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		fall = true;
> +		break;

This looks wrong. You cannot map a level interrupt into an edge. It looks like your hardware only supports edges. If asked to do level, return -EINVAL.

Asmaa>> done

> +
> +	/* The INT_N interrupt level is active low.
> +	 * So enable cause fall bit to detect when GPIO
> +	 * state goes low.
> +	 */

I don't understand this comment.

Asmaa>> removed.