Re: [PATCH v5 2/2] mfd: arizona: Convert GPIO IRQ handling to descriptors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Migration from legacy gpio to gpiod incorrectly replaces physical line reads with logical value reads, breaking IRQ polling logic for active-low GPIOs.

Pre-existing issues:
- [High] Negative error codes from GPIO reads are implicitly treated as boolean true, causing an infinite loop in the IRQ thread on read failure.
--

commit 2b527ef2c7b9b60397ceb6125cd15341ac4e28ff
Author: Linus Walleij <linusw@kernel.org>

mfd: arizona: Convert GPIO IRQ handling to descriptors

This commit converts the arizona polling GPIO handling to use a GPIO descriptor
instead of passing a global GPIO number as platform data.

> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index 544016d420fe2..cf8e42662585a 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -136,21 +136,19 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
[ ... ]
>  		if (!arizona->irq_gpiod) {
>  			break;
>  		} else if (arizona->pdata.irq_flags & IRQF_TRIGGER_RISING &&
> -			   gpio_get_value_cansleep(arizona->pdata.irq_gpio)) {
> +			   gpiod_get_value_cansleep(arizona->irq_gpiod)) {

[Severity: High]
Does this incorrectly check the logical value of the GPIO instead of the raw
physical state?

The legacy gpio_get_value_cansleep() returned the raw physical state of the
pin. The new gpiod_get_value_cansleep() returns the logical state, which
incorporates the ACTIVE_LOW flag from the devicetree or software node.

Because the IRQ trigger flags describe physical voltage transitions, comparing
these physical flags against a logical value might be incorrect. For example,
if a user defines irq-gpios with GPIO_ACTIVE_LOW for a falling edge interrupt,
a physically low line becomes logically 1, causing the polling condition
!gpiod_get_value_cansleep() to evaluate to 0, which fails to poll when the
interrupt is active.

Conversely, when the line is idle, it evaluates to 1, causing the IRQ handler
to poll infinitely.

Should this use gpiod_get_raw_value_cansleep() to preserve the original
physical semantics?

[Severity: High]
This is a pre-existing issue, but can this loop forever if the GPIO read
returns a negative error code?

gpiod_get_value_cansleep() (and its legacy predecessor) can return a negative
error code (e.g., -EIO) if reading the GPIO state fails over the bus. Since
negative values are non-zero, this evaluates to true for IRQF_TRIGGER_RISING,
setting poll = true and causing an infinite loop in the IRQ handler thread if
the read continues to fail.

Conversely, for IRQF_TRIGGER_FALLING, !(-EIO) evaluates to false, which
immediately aborts the poll.

Could the return value be stored in an integer and explicitly checked for
negative errors before evaluating it as a boolean?

>  			poll = true;
>  		} else if (arizona->pdata.irq_flags & IRQF_TRIGGER_FALLING &&
> -			   !gpio_get_value_cansleep(arizona->pdata.irq_gpio)) {
> +			   !gpiod_get_value_cansleep(arizona->irq_gpiod)) {
>  			poll = true;
>  		}
>  	} while (poll);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-mfd-arizona-irq-v5-0-ebeda2e925da@kernel.org?part=2