[PATCH 1/2] gpio: mxc: fix irq_high handling

[PATCH 1/2] gpio: mxc: fix irq_high handling

[Alexander Stein]

If port->irq_high is -1 (fsl,imx21-gpio compatible) and gpio_idx is >= 16
enable_irq_wake() is called with -1 which is wrong.

Fixes: 5f6d1998adeb ("gpio: mxc: release the parent IRQ in runtime suspend")
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
I don't have hardware to test. I just noticed this by code review.

 drivers/gpio/gpio-mxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 647b6f4861b74..12f11a6c96653 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -469,7 +469,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 		 * the handler is needed only once, but doing it for every port
 		 * is more robust and easier.
 		 */
-		port->irq_high = -1;
+		port->irq_high = 0;
 		port->mx_irq_handler = mx2_gpio_irq_handler;
 	} else
 		port->mx_irq_handler = mx3_gpio_irq_handler;
-- 
2.43.0

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

[Alexander Stein]

Do not open-code the BIT() macro.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/gpio/gpio-mxc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 12f11a6c96653..7e2690d92df6f 100644
--- 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);
 	} 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;
-- 
2.43.0

Re: [PATCH 1/2] gpio: mxc: fix irq_high handling

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing teardown of the chained IRQ handler in the `mxc_gpio_probe()` error path leads to a Use-After-Free vulnerability.
- [High] Calling `clk_prepare_enable()` during `syscore_ops.suspend` violates atomic context requirements.
- [High] In `mxc_gpio_runtime_resume()`, the chained IRQ handler is hooked before the GPIO clock is enabled, risking unclocked register access.
- [High] In `mxc_gpio_runtime_suspend()`, the GPIO clock is disabled before the chained IRQ handler is unhooked, risking unclocked register access.
- [High] `mx2_gpio_irq_handler` iterates over all GPIO ports and reads their registers unconditionally without checking their runtime PM or clock state.
--

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

gpio: mxc: fix irq_high handling

This commit fixes an issue where enable_irq_wake() is incorrectly called
with a negative value when port irq_high is -1 and gpio_idx is >= 16.
It resolves this by initializing irq_high to 0 for the fsl,imx21-gpio
compatible.

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 647b6f4861b74..12f11a6c96653 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -469,7 +469,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  		 * the handler is needed only once, but doing it for every port
>  		 * is more robust and easier.
>  		 */
> -		port->irq_high = -1;
> +		port->irq_high = 0;
>  		port->mx_irq_handler = mx2_gpio_irq_handler;

[Severity: High]
This is a pre-existing issue, but does the error path in mxc_gpio_probe()
properly tear down the chained IRQ handler?

If a subsequent initialization step fails, such as gpio_generic_chip_init()
or devm_irq_alloc_descs(), the error path does not appear to call
mxc_update_irq_chained_handler(port, false) to unhook it.

Since the port pointer is managed by devres and gets freed upon returning
the error, could a subsequent parent interrupt cause a use-after-free when
the IRQ core executes the handler and dereferences the freed port pointer?

[Severity: High]
This is a pre-existing issue, but is it safe for mx2_gpio_irq_handler()
to iterate over all GPIO ports without checking their runtime PM or clock
state?

The handler iterates through the global mxc_gpio_ports list and reads the
GPIO_IMR and GPIO_ISR registers for every port.

If a port is suspended and its clock is disabled, wouldn't attempting
to read its registers violate hardware constraints and potentially trigger
a bus fault?

>  	} else
>  		port->mx_irq_handler = mx3_gpio_irq_handler;

[Severity: High]
This is a pre-existing issue, but does mxc_gpio_runtime_resume() risk
accessing unclocked registers?

It calls mxc_update_irq_chained_handler(port, true) to hook the parent IRQ
handler before calling clk_prepare_enable(port->clk).

If an interrupt fires in this window, could the handler execute and
attempt to read device registers while the hardware clock is still
disabled?

[Severity: High]
This is a pre-existing issue, but there appears to be a similar race in
mxc_gpio_runtime_suspend().

The hardware clock is disabled via clk_disable_unprepare(port->clk) before
mxc_update_irq_chained_handler(port, false) unregisters the handler.

If a pending parent interrupt fires between these two calls, could the
active IRQ handler access the hardware registers without an active clock?

[Severity: High]
This is a pre-existing issue, but does mxc_gpio_syscore_suspend() violate
atomic context rules?

It executes in the late stages of system suspend where only one CPU is
online and local interrupts are hard-disabled. By calling
clk_prepare_enable(port->clk), it internally calls clk_prepare() which
takes the global prepare_lock mutex and includes a might_sleep() check.

Could this sleeping or mutex locking in an atomic context cause deadlocks
or trigger warnings?

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

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

[sashiko-bot]

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

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

[Frank Li]

On Fri, May 22, 2026 at 09:01:16AM +0200, Alexander Stein wrote:
> Do not open-code the BIT() macro.

Nit: Replace the open-code with the BIT() macro.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/gpio/gpio-mxc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 12f11a6c96653..7e2690d92df6f 100644
> --- 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);
>  	} 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;
> --
> 2.43.0
>

Re: [PATCH 1/2] gpio: mxc: fix irq_high handling

[Frank Li]

On Fri, May 22, 2026 at 09:01:15AM +0200, Alexander Stein wrote:
> If port->irq_high is -1 (fsl,imx21-gpio compatible) and gpio_idx is >= 16
> enable_irq_wake() is called with -1 which is wrong.
>
> Fixes: 5f6d1998adeb ("gpio: mxc: release the parent IRQ in runtime suspend")
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> I don't have hardware to test. I just noticed this by code review.

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
>  drivers/gpio/gpio-mxc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 647b6f4861b74..12f11a6c96653 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -469,7 +469,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  		 * the handler is needed only once, but doing it for every port
>  		 * is more robust and easier.
>  		 */
> -		port->irq_high = -1;
> +		port->irq_high = 0;
>  		port->mx_irq_handler = mx2_gpio_irq_handler;
>  	} else
>  		port->mx_irq_handler = mx3_gpio_irq_handler;
> --
> 2.43.0
>