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 03:12:43PM -0500, Frank Li wrote:
>On Wed, Oct 07, 2026 at 06:44:21PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> Replace irq_domain_create_legacy() with devm_irq_domain_instantiate()
>> and the open-coded pm_runtime_set_active() + pm_runtime_enable() pair
>> with devm_pm_runtime_set_active_enabled(), converting the remaining
>> manually-unwound resources in probe to devres management.
>>
>> With every allocation after the PM block now devm-managed, the
>> out_irqdomain_remove and out_bgio error-path labels are eliminated
>> entirely - probe errors simply return directly.
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  drivers/gpio/gpio-mxc.c | 49 +++++++++++++++++++++++++------------------------
>>  1 file changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
>> index 5da603569d88..54b09f1a4b50 100644
>> --- a/drivers/gpio/gpio-mxc.c
>> +++ b/drivers/gpio/gpio-mxc.c
>> @@ -449,6 +449,7 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>>  {
>>  	struct gpio_generic_chip_config config = { };
>>  	struct device_node *np = pdev->dev.of_node;
>> +	struct irq_domain_info d_info;
>>  	struct mxc_gpio_port *port;
>>  	int irq_count;
>>  	int irq_base;
>> @@ -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");
>
>look like needn't call devm_pm_runtime_Get_noresume() to pump ref count.

We need pm_runtime_get_noresume to keep ref count.

Without it, the device is at usage_count = 0 immediately after
pm_runtime_enable() (inside devm_pm_runtime_set_active_enabled).
There's hardware access after that point - writel() to GPIO_IMR/GPIO_ISR,
and devm_irq_setup_generic_chip() inside mxc_gpio_init_gc().
More critically, devm_gpiochip_add_data() makes the chip visible to consumers
mid-probe. With async probing, a consumer on another bus could do:
mxc_gpio_request()  -> pm_runtime_resume_and_get()  -> usage_count 0->1
mxc_gpio_free()     -> pm_runtime_put()             -> usage_count 1->0 -> idle -> suspend
That triggers mxc_gpio_runtime_suspend() which calls clk_disable_unprepare()
while probe is still accessing registers. The get_noresume keeps
usage_count >= 1 through the entire probe, preventing that.

So the correct order:
pm_runtime_get_noresume(dev);            // hold active through probe
devm_pm_runtime_set_active_enabled(dev); // set active + enable (devres-managed)
... probe with HW access ...
pm_runtime_put_autosuspend(dev);         // release, allow idle/suspend

Regards
Peng
>
>Frank
>
>> +
>> +	err = devm_pm_runtime_set_active_enabled(&pdev->dev);
>> +	if (err)
>> +		return dev_err_probe(&pdev->dev, err, "Failed to enable PM runtime\n");
>>
>>  	/* disable the interrupt and clear the status */
>>  	writel(0, port->base + GPIO_IMR);
>> @@ -512,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;
>>
>>  	port->gen_gc.gc.request = mxc_gpio_request;
>>  	port->gen_gc.gc.free = mxc_gpio_free;
>> @@ -528,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;
>>
>>  	list_add_tail(&port->node, &mxc_gpio_ports);
>>
>> @@ -558,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;
>>  }
>>
>>  static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
>>
>> --
>> 2.51.0
>>
>>
>