Re: [PATCH v5 10/13] gpio: mxc: convert probe error handling to devres

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] pm: double-put of PM runtime usage count on probe success
- [High] gpio: mxc: NULL ptr dereference on probe error due to inverted PM cleanup order
- [High] gpio: mxc: early gpiochip exposure returns -ENXIO from to_irq
--

commit 8d3ca67baef31cc2d1c0b0ff840208db9ae18818
Author: Peng Fan <peng.fan@nxp.com>

gpio: mxc: convert probe error handling to devres

This commit converts the manual probe error handling in mxc_gpio_probe() to
devres management by utilizing devm_pm_runtime_set_active_enabled() and
devm_irq_domain_instantiate(). Error path labels are eliminated, replacing
them with direct returns.

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 3d5a6566d3f43..6074b1e3d6834 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
[ ... ]
> @@ -491,8 +492,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  
>  	struct device *pm_guard __free(pm_runtime_put_noidle) = dev;
>  
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> +	err = devm_pm_runtime_set_active_enabled(dev);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to enable PM runtime\n");
[ ... ]
> @@ -534,27 +536,31 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  
>  	err = devm_gpiochip_add_data(dev, &port->gen_gc.gc, port);
>  	if (err)
> -		goto out_bgio;
> +		return err;

[Severity: High]
Could exposing the GPIO chip before the IRQ domain is ready cause issues for
concurrent consumers?

Because devm_gpiochip_add_data() is called before the IRQ domain is
instantiated, a concurrent consumer might call gpiod_to_irq() while
port->domain is still NULL.

In this scenario, irq_find_mapping() inside mxc_gpio_to_irq() will safely
handle the NULL domain and return 0. However, gpiod_to_irq() translates a
0 return value from a custom to_irq callback as -ENXIO. This bypasses the
gpiolib protection that normally returns -EPROBE_DEFER for uninitialized
irqchips, causing the consumer probe to fail permanently.

>  
>  	irq_base = devm_irq_alloc_descs(dev, -1, 0, 32, numa_node_id());
> -	if (irq_base < 0) {
> -		err = irq_base;
> -		goto out_bgio;
> -	}
> +	if (irq_base < 0)
> +		return irq_base;

[Severity: High]
Does this sequence create a window for a NULL pointer dereference on probe
error?

When returning an error after devm_gpiochip_add_data(), the
__free(pm_runtime_put_noidle) cleanup will execute immediately, dropping the
usage count to 0. Since devm_pm_runtime_set_active_enabled() uses devres, PM
runtime won't be disabled until later during devres release.

If a concurrent consumer requests and frees a GPIO during this window, it can
drop the final usage count and trigger mxc_gpio_runtime_suspend(). Because
platform_set_drvdata() hasn't been called yet, dev_get_drvdata() will return
NULL, which is then dereferenced in mxc_gpio_save_regs().

[ ... ]
> @@ -566,13 +572,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	pm_runtime_put_autosuspend(dev);
>  
>  	return 0;

[Severity: High]
Does the self-assignment of no_free_ptr() earlier in this block fail to
inhibit the cleanup?

The mxc_gpio_probe() function ends with:

	pm_guard = no_free_ptr(pm_guard);
	pm_runtime_put_autosuspend(dev);

	return 0;

The no_free_ptr() macro is implemented as a statement expression that
nullifies the internal pointer but evaluates to the original pointer value.
Assigning this output back to the same variable overwrites the nullified
value with the original pointer.

Because pm_guard is still non-NULL when the function returns, the
pm_runtime_put_noidle() cleanup function will also execute, resulting in a
double-put of the PM runtime usage count on success.

Can this be resolved by using no_free_ptr(pm_guard) without the assignment?

> -
> -out_irqdomain_remove:
> -	irq_domain_remove(port->domain);
> -out_bgio:
> -	pm_runtime_disable(dev);
> -	dev_info(dev, "%s failed with errno %d\n", __func__, err);
> -	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-gpio-mxc-cleanup-v5-0-1e59e6ddfa00@nxp.com?part=10