Linux GPIO subsystem development
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc: imx@lists.linux.dev, linux-gpio@vger.kernel.org, Frank.Li@kernel.org
Subject: Re: [PATCH v2 5/9] gpio: mxc: convert probe error handling to devres
Date: Mon, 05 Oct 2026 14:34:17 +0000	[thread overview]
Message-ID: <20261005143417.D6D8D1F000FF@smtp.kernel.org> (raw)
In-Reply-To: <20261005-gpio-mxc-cleanup-v2-5-bdc3afbb35e2@nxp.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PM: runtime usage counter leak on probe error paths
--

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

    gpio: mxc: convert probe error handling to devres

This patch replaces manual resource unwinding in mxc_gpio_probe() with devm
variants and eliminates the error path labels. It returns directly upon
failure instead of jumping to cleanup code.

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 1bd9f5beae4a7..871938e4cc959 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c

[ ... ]

> @@ -486,8 +487,11 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  		return PTR_ERR(port->clk);
>  
>  	pm_runtime_get_noresume(&pdev->dev);
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	err = devm_pm_runtime_set_active_enabled(&pdev->dev);
> +	if (err) {
> +		pm_runtime_put_noidle(&pdev->dev);
> +		return dev_err_probe(&pdev->dev, err, "Failed to enable PM runtime\n");
> +	}

[Severity: Medium]
The usage counter is manually incremented here with pm_runtime_get_noresume(),
but this specific reference does not appear to be managed by a devm helper.

[ ... ]

> @@ -513,7 +517,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  
>  	err = gpio_generic_chip_init(&port->gen_gc, &config);
>  	if (err)
> -		goto out_bgio;
> +		return err;

[Severity: Medium]
Does this direct return leak the PM usage counter? The original code jumped to
the out_bgio label which called pm_runtime_put_noidle() to balance the manual
pm_runtime_get_noresume() call earlier. Since pm_runtime_get_noresume() is
not devm-managed, returning directly here seems to leave the usage counter
elevated.

[ ... ]

> @@ -529,27 +533,31 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  
>  	err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port);
>  	if (err)
> -		goto out_bgio;
> +		return err;
>  
>  	irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0, 32, numa_node_id());
> -	if (irq_base < 0) {
> -		err = irq_base;
> -		goto out_bgio;
> -	}
> +	if (irq_base < 0)
> +		return irq_base;
> +
> +	d_info = (struct irq_domain_info) {
> +		.fwnode		= dev_fwnode(&pdev->dev),
> +		.size		= 32,
> +		.hwirq_max	= 32,
> +		.virq_base	= irq_base,
> +		.ops		= &irq_domain_simple_ops,
> +		.dev		= &pdev->dev,
> +	};
>  
> -	port->domain = irq_domain_create_legacy(dev_fwnode(&pdev->dev), 32, irq_base, 0,
> -						&irq_domain_simple_ops, NULL);
> -	if (!port->domain) {
> -		err = -ENODEV;
> -		goto out_bgio;
> -	}
> +	port->domain = devm_irq_domain_instantiate(&pdev->dev, &d_info);
> +	if (IS_ERR(port->domain))
> +		return PTR_ERR(port->domain);
>  
>  	irq_domain_set_pm_device(port->domain, &pdev->dev);
>  
>  	/* gpio-mxc can be a generic irq chip */
>  	err = mxc_gpio_init_gc(port, irq_base);
>  	if (err < 0)
> -		goto out_irqdomain_remove;
> +		return err;

[Severity: Medium]
Do these direct returns for devm_gpiochip_add_data(), devm_irq_alloc_descs(),
devm_irq_domain_instantiate(), and mxc_gpio_init_gc() failures also skip
dropping the PM usage counter for the same reason?

[ ... ]

> @@ -559,14 +567,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  	pm_runtime_put_autosuspend(&pdev->dev);
>  
>  	return 0;
> -
> -out_irqdomain_remove:
> -	irq_domain_remove(port->domain);
> -out_bgio:
> -	pm_runtime_disable(&pdev->dev);
> -	pm_runtime_put_noidle(&pdev->dev);
> -	dev_info(&pdev->dev, "%s failed with errno %d\n", __func__, err);
> -	return err;
>  }

[Severity: Medium]
With the out_bgio label removed, the explicit pm_runtime_put_noidle() cleanup
is gone. This could prevent the device from entering runtime suspend if probed
again or on probe deferral. Should the get_noresume() call be converted to a
devm-managed action, or should the explicit cleanup be retained on these
error paths?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-gpio-mxc-cleanup-v2-0-bdc3afbb35e2@nxp.com?part=5

  reply	other threads:[~2026-10-05 14:34 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-05 14:21 [PATCH v2 0/9] gpio: mxc: bug fixes and probe cleanup Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 1/9] gpio: mxc: fix race between chained IRQ handler install and probe completion Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 2/9] gpio: mxc: fix wakeup_pads bit operations Peng Fan (OSS)
2026-10-06  8:36   ` Andy Shevchenko
2026-10-06 12:52     ` Peng Fan
2026-10-05 14:21 ` [PATCH v2 3/9] gpio: mxc: replace of_device_is_compatible() with hwdata flags Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 4/9] gpio: mxc: convert pad wakeup compatible checks to " Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 5/9] gpio: mxc: convert probe error handling to devres Peng Fan (OSS)
2026-10-05 14:34   ` sashiko-bot [this message]
2026-10-05 14:21 ` [PATCH v2 6/9] gpio: mxc: switch probe error paths to dev_err_probe() Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 7/9] gpio: mxc: use local dev variable Peng Fan (OSS)
2026-10-05 14:34   ` sashiko-bot
2026-10-05 14:21 ` [PATCH v2 8/9] gpio: mxc: introduce MXC_ICR macros and use field_prep/field_get Peng Fan (OSS)
2026-10-05 14:21 ` [PATCH v2 9/9] gpio: mxc: use BIT() macro for single-bit operations Peng Fan (OSS)
2026-10-06 10:16   ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20261005143417.D6D8D1F000FF@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=Frank.Li@kernel.org \
    --cc=imx@lists.linux.dev \
    --cc=linux-gpio@vger.kernel.org \
    --cc=peng.fan@oss.nxp.com \
    --cc=sashiko-reviews@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox