| Message ID | 20261003-gpio-mxc-cleanup-v1-5-dad728ce27f2@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: mxc: bug fixes and probe cleanup | expand |
On Sat, Oct 3, 2026 at 3:09 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > Switch pm_runtime_get_noresume() and pm_runtime_enable() to their > devm-managed variants so that pm_runtime_put_noidle() and > pm_runtime_disable() are handled automatically by devres on both > probe failure and device unbind. > > Remove the out_bgio goto label and replacing all error paths with replace > direct returns using dev_err_probe(), which provides better > diagnostic output and handles -EPROBE_DEFER. ... > - pm_runtime_get_noresume(&pdev->dev); > + devm_pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > + devm_pm_runtime_enable(&pdev->dev); Definitely not. There is little point to using devm_*() if you don't check the return value. ... > err = gpio_generic_chip_init(&port->gen_gc, &config); > if (err) > - goto out_bgio; > + return dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n"); > err = devm_gpiochip_add_data(&pdev->dev, &port->gen_gc.gc, port); > if (err) > - goto out_bgio; > + return dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n"); These (and more) don't belong to the change — split it to the logically isolated ones. One patch for dev_err_probe() and another for PM calls.
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] gpio: mxc: PM usage counter underflow due to devm_pm_runtime_get_noresume and manual put - [High] gpio: mxc: ignored devm_pm_runtime_enable and get_noresume returns -- commit e443f1e5bd54a410ade428061ede6f400132fc30 Author: Peng Fan <peng.fan@nxp.com> gpio: mxc: use devres-managed PM runtime and dev_err_probe This patch switches the driver to use devres-managed PM runtime functions and replaces goto error paths with direct returns using dev_err_probe(). > diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c > index 3c395c82d7d46..73e19d2bf2351 100644 > --- a/drivers/gpio/gpio-mxc.c > +++ b/drivers/gpio/gpio-mxc.c > @@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev) > if (of_device_is_compatible(np, "fsl,imx8qm-gpio")) > port->is_imx8qm = true; > > - pm_runtime_get_noresume(&pdev->dev); > + devm_pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > + devm_pm_runtime_enable(&pdev->dev); [Severity: High] Should the return values of devm_pm_runtime_get_noresume() and devm_pm_runtime_enable() be checked? Both functions allocate memory for devres actions and can return error codes like -ENOMEM. If devm_pm_runtime_get_noresume() fails, it drops its reference internally by calling pm_runtime_put_noidle(). If we ignore this failure and continue, the probe proceeds under the false assumption that it holds a PM reference. The manual call to pm_runtime_put_autosuspend() at the end of mxc_gpio_probe() would then immediately underflow the PM usage counter. [ ... ] > @@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev) > pm_runtime_put_autosuspend(&pdev->dev); [Severity: High] Will this result in a PM usage counter underflow when the device is unbound? Because devm_pm_runtime_get_noresume() registers a devres action to call pm_runtime_put_noidle() on device unbind, the initial reference is already managed. By manually calling pm_runtime_put_autosuspend() here at the end of probe to enter autosuspend, the usage counter is decremented early. When the device is subsequently unregistered or unbound, the devres cleanup will execute and drop the already zeroed usage counter, causing a kernel warning. > > return 0; > - > -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; > }
On Sat, Oct 03, 2026 at 08:06:47PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Switch pm_runtime_get_noresume() and pm_runtime_enable() to their > devm-managed variants so that pm_runtime_put_noidle() and > pm_runtime_disable() are handled automatically by devres on both > probe failure and device unbind. > > Remove the out_bgio goto label and replacing all error paths with > direct returns using dev_err_probe(), which provides better > diagnostic output and handles -EPROBE_DEFER. > > Assisted-by: LLM > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/gpio/gpio-mxc.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c > index 3c395c82d7d4..73e19d2bf235 100644 > --- a/drivers/gpio/gpio-mxc.c > +++ b/drivers/gpio/gpio-mxc.c > @@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev) > if (of_device_is_compatible(np, "fsl,imx8qm-gpio")) > port->is_imx8qm = true; > > - pm_runtime_get_noresume(&pdev->dev); > + devm_pm_runtime_get_noresume(&pdev->dev); > pm_runtime_set_active(&pdev->dev); > - pm_runtime_enable(&pdev->dev); > + devm_pm_runtime_enable(&pdev->dev); devm_pm_runtime_set_active_enabled() can include pm_runtime_set_active() and need check return value here. And why call pm_runtime_get_noresume() before enable()? Frank > > /* disable the interrupt and clear the status */ > writel(0, port->base + GPIO_IMR); > @@ -502,7 +502,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 dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n"); > > port->gen_gc.gc.request = mxc_gpio_request; > port->gen_gc.gc.free = mxc_gpio_free; > @@ -518,31 +518,27 @@ 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 dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n"); > > 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 dev_err_probe(&pdev->dev, irq_base, "Failed to alloc irq desc\n"); > > 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; > - } > + if (!port->domain) > + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to create irq domain\n"); > > err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain); > if (err) > - goto out_bgio; > + return dev_err_probe(&pdev->dev, err, "Failed to add irq_domain_remove\n"); > > 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_bgio; > + return dev_err_probe(&pdev->dev, err, "Failed mxc_gpio_init_gc\n"); > > list_add_tail(&port->node, &mxc_gpio_ports); > > @@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev) > pm_runtime_put_autosuspend(&pdev->dev); > > return 0; > - > -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; > } > > static void mxc_gpio_save_regs(struct mxc_gpio_port *port) > > -- > 2.51.0 > >
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index 3c395c82d7d4..73e19d2bf235 100644 --- a/drivers/gpio/gpio-mxc.c +++ b/drivers/gpio/gpio-mxc.c @@ -474,9 +474,9 @@ static int mxc_gpio_probe(struct platform_device *pdev) if (of_device_is_compatible(np, "fsl,imx8qm-gpio")) port->is_imx8qm = true; - pm_runtime_get_noresume(&pdev->dev); + devm_pm_runtime_get_noresume(&pdev->dev); pm_runtime_set_active(&pdev->dev); - pm_runtime_enable(&pdev->dev); + devm_pm_runtime_enable(&pdev->dev); /* disable the interrupt and clear the status */ writel(0, port->base + GPIO_IMR); @@ -502,7 +502,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 dev_err_probe(&pdev->dev, err, "Failed to init gpio chip\n"); port->gen_gc.gc.request = mxc_gpio_request; port->gen_gc.gc.free = mxc_gpio_free; @@ -518,31 +518,27 @@ 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 dev_err_probe(&pdev->dev, err, "Failed to add gpiochip data\n"); 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 dev_err_probe(&pdev->dev, irq_base, "Failed to alloc irq desc\n"); 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; - } + if (!port->domain) + return dev_err_probe(&pdev->dev, -ENODEV, "Failed to create irq domain\n"); err = devm_add_action_or_reset(&pdev->dev, mxc_gpio_irq_domain_remove, port->domain); if (err) - goto out_bgio; + return dev_err_probe(&pdev->dev, err, "Failed to add irq_domain_remove\n"); 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_bgio; + return dev_err_probe(&pdev->dev, err, "Failed mxc_gpio_init_gc\n"); list_add_tail(&port->node, &mxc_gpio_ports); @@ -552,12 +548,6 @@ static int mxc_gpio_probe(struct platform_device *pdev) pm_runtime_put_autosuspend(&pdev->dev); return 0; - -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; } static void mxc_gpio_save_regs(struct mxc_gpio_port *port)