Re: [PATCH 2/2] gpio: mxc: use BIT() macro

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value from `enable_irq_wake()` leads to permanently corrupted wakeup state.
- [High] Missing locking causes a data race on the shared `port->wakeup_pads` state.
--

commit bd0f86527c727096d8a5aca0060d7d08ddaeda28
Author: Alexander Stein <alexander.stein@ew.tq-group.com>

gpio: mxc: use BIT() macro

This commit updates the gpio-mxc driver to use the BIT() macro instead of
open-coding bit shifts.

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c

[ ... ]

> @@ -330,13 +330,13 @@ static int gpio_set_wake_irq(struct irq_data *d, u32 enable)
>  			ret = enable_irq_wake(port->irq_high);
>  		else
>  			ret = enable_irq_wake(port->irq);
> -		port->wakeup_pads |= (1 << gpio_idx);
> +		port->wakeup_pads |= BIT(gpio_idx);

[Severity: High]
This is a pre-existing issue, but does this function properly handle
failures from enable_irq_wake()?

It appears that port->wakeup_pads is modified before validating the return
value of enable_irq_wake() or disable_irq_wake(). If enable_irq_wake() fails
(for instance, if the parent IRQ does not support wakeup), the bit remains
erroneously set.

The irq subsystem core reacts to the failure by leaving the child IRQ's
wake_depth at 0. As a result, subsequent calls to disable_irq_wake() by the
driver will be ignored (triggering an unbalanced irq wake disable warning)
and will never invoke gpio_set_wake_irq() to clear the bit. This causes the
bit to become permanently stuck. 

During system suspend, mxc_gpio_noirq_suspend() could then erroneously
program the SCU to enable pad wakeup for this pin, leading to unintended
system wakeups.

[Severity: High]
This is also a pre-existing issue, but is there a data race when modifying
port->wakeup_pads?

This callback is invoked by irq_set_irq_wake(), which only holds the
desc->lock of the specific child IRQ (the individual GPIO pin). It does not
seem to hold the shared GPIO chip's gc->lock.

Since port->wakeup_pads tracks the wakeup state of all 32 pins on the
port, if multiple drivers concurrently configure wakeup for different pins on
the same port, they would hold independent child locks and concurrently
modify port->wakeup_pads. Could this lead to lost updates, potentially
resulting in the system failing to wake up from suspend?

>  	} else {
>  		if (port->irq_high && (gpio_idx >= 16))
>  			ret = disable_irq_wake(port->irq_high);
>  		else
>  			ret = disable_irq_wake(port->irq);
> -		port->wakeup_pads &= ~(1 << gpio_idx);
> +		port->wakeup_pads &= ~BIT(gpio_idx);
>  	}
>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522070118.800671-1-alexander.stein@ew.tq-group.com?part=2