Re: [PATCH] gpio: pca953x: Handle direction register read errors in IRQ paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] gpio: pca953x: mixing guard() and goto in pca953x_irq_bus_sync_unlock()
--

commit f8c41495cf791bc6655e128fd9192a150f30aa3c
Author: Sahaj Chaudhari <sahaj123.sc@gmail.com>

gpio: pca953x: Handle direction register read errors in IRQ paths

This commit adds error checking for direction register reads in
pca953x_irq_bus_sync_unlock(), pca953x_irq_pending(), and
pca953x_irq_setup() to avoid using uninitialized stack bitmaps when
pca953x_read_regs() fails.

> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index aac9ea7520b60..b6f0849d3bdf9 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -862,6 +862,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  	DECLARE_BITMAP(irq_mask, MAX_LINE);
>  	DECLARE_BITMAP(reg_direction, MAX_LINE);
>  	int level;
> +	int ret;
>  
>  	guard(mutex)(&chip->i2c_lock);
>  
> @@ -880,7 +881,9 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  	}
>  
>  	/* Switch direction to input if needed */
> -	pca953x_read_regs(chip, chip->regs->direction, reg_direction);
> +	ret = pca953x_read_regs(chip, chip->regs->direction, reg_direction);
> +	if (ret)
> +		goto out;
>  
>  	bitmap_or(irq_mask, chip->irq_trig_fall, chip->irq_trig_raise, gc->ngpio);
>  	bitmap_or(irq_mask, irq_mask, chip->irq_trig_level_high, gc->ngpio);
> @@ -890,6 +893,7 @@ static void pca953x_irq_bus_sync_unlock(struct irq_data *d)
>  	for_each_andnot_bit(level, irq_mask, reg_direction, gc->ngpio)
>  		pca953x_gpio_direction_input_unlocked(&chip->gpio_chip, level);
>  
> +out:
>  	mutex_unlock(&chip->irq_lock);
>  }

[Severity: Low]
In drivers/gpio/gpio-pca953x.c, pca953x_irq_bus_sync_unlock() uses
guard(mutex)(&chip->i2c_lock) at function scope. When pca953x_read_regs()
returns an error, this change jumps to the new out label to unlock
chip->irq_lock.

Per include/linux/cleanup.h:

    * Lastly, given that the benefit of cleanup helpers is removal of
    * "goto", and that the "goto" statement can jump between scopes, the
    * expectation is that usage of "goto" and cleanup helpers is never
    * mixed in the same function. I.e. for a given routine, convert all
    * resources that need a "goto" cleanup to scope-based cleanup, or
    * convert none of them.

Could pca953x_irq_bus_sync_unlock() avoid mixing guard() and goto by wrapping
the direction update in an if (!ret) block (or using scoped_guard(mutex,
&chip->i2c_lock) around the I2C accesses before unlocking chip->irq_lock)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018165003.3372368-1-sahaj123.sc@gmail.com?part=1