Re: [PATCH v4 06/10] gpio: mxc: convert probe error handling to devres

[Peng Fan <peng.fan@oss.nxp.com>]

On Thu, Oct 08, 2026 at 04:47:48AM -0500, Bartosz Golaszewski wrote:
>On Wed, 7 Oct 2026 12:59:26 +0200, sashiko-bot@kernel.org said:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] pm: usage counter underflow due to mixed devm and manual autosuspend
>> --
>>
>> commit dc287cc46ea891b838f3b39a1fd6ed65d627a965
>> Author: Peng Fan <peng.fan@nxp.com>
>>
>>     gpio: mxc: convert probe error handling to devres
>>
>>     This patch converts the driver's probe error handling to use devres. It
>>     replaces irq_domain_create_legacy() with devm_irq_domain_instantiate()
>>     and updates the PM runtime initialization to use devm_ variants, eliminating
>>     the need for manual cleanup labels.
>>
>>> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
>>> index 5da603569d88d..54b09f1a4b509 100644
>>> --- a/drivers/gpio/gpio-mxc.c
>>> +++ b/drivers/gpio/gpio-mxc.c
>>> @@ -484,9 +485,13 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>>>  	if (IS_ERR(port->clk))
>>>  		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_get_noresume(&pdev->dev);
>>> +	if (err)
>>> +		return dev_err_probe(&pdev->dev, err, "Failed to get PM runtime\n");
>>> +
>>> +	err = devm_pm_runtime_set_active_enabled(&pdev->dev);
>>> +	if (err)
>>> +		return dev_err_probe(&pdev->dev, err, "Failed to enable PM runtime\n");
>>
>> [ ... ]
>>
>>> @@ -558,14 +567,6 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>>>  	pm_runtime_put_autosuspend(&pdev->dev);
>>
>> [Severity: High]
>> Since devm_pm_runtime_get_noresume() increments the usage counter and queues
>> a devres action to drop it on unbind, will this manual call to
>> pm_runtime_put_autosuspend() cause an issue?
>>
>> Because the devres action remains queued, when the device is unbound (e.g.,
>> via device tree overlay removal or hot-unplug), the devres cleanup will
>> execute and decrement the usage counter a second time. Could this cause a PM
>> usage counter underflow, corrupting the device and parent power state?
>>
>
>Sounds right, please remove this call.

we need to keep it. Without it, the gpio port will never runs into suspended state.

I think it should be not be marked as High severity (:

The driver sets suppress_bind_attrs = true and has no .remove callback. So never
unbinds. Even if we do have unbind:
Probe success:
  devm_pm_runtime_get_noresume()     -> usage_count = 1, devres queues put_noidle
  devm_pm_runtime_set_active_enabled -> devres queues disable + set_suspended
  ...probe succeeds...
  pm_runtime_put_autosuspend()       -> usage_count = 0, device may suspend

Unbind (devres cleanup, reverse order):
  [1] pm_runtime_disable_action       -> PM runtime disabled
  [2] pm_runtime_set_suspended_action -> RPM_SUSPENDED
  [3] pm_runtime_put_noidle_action    -> usage_count = -1  <- underflow

With PM runtime disabled[1], pm_runtime_put_noidle() just does atomic_dec() on
a counter nobody reads - no idle/suspend work is queued, no parent power
state is touched.


So to make things a bit cleaner, we need use pm_runtime_get_resume(no devres),
and define __free for it.

DEFINE_FREE(pm_runtime_put_noidle, struct device *, if (_T) pm_runtime_put_noidle(_T))
Then in probe:
pm_runtime_get_noresume(dev);

struct device *pm_guard __free(pm_runtime_put_noidle) = dev;

err = devm_pm_runtime_set_active_enabled(dev);
if (err)
    return dev_err_probe(dev, err, "...");  // guard fires -> put_noidle

/* ... all error returns automatically balanced ... */

/* success — disarm guard, do the real put */
no_free_ptr(pm_guard);
pm_runtime_put_autosuspend(dev);
return 0;

Thanks,
Peng

>
>Bart
>