Re: [PATCH] hwrng: cctrng - Fix an error handling path in cctrng_probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwrng: cctrng: unbalanced pm_runtime_disable() in cctrng_probe() error path
--

commit a9816f5e80718b9a879081c61af5922aa650d7de
Author: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

hwrng: cctrng - Fix an error handling path in cctrng_probe

This commit routes cc_trng_pm_get() and devm_hwrng_register() failures in
cctrng_probe() to a common post_pm_err label that calls cc_trng_pm_fini().

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index 03401ee59a8e8..608b3321c05aa 100644
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
> @@ -524,8 +524,10 @@ static int cctrng_probe(struct platform_device *pdev)
>  
>  	/* increment device's usage counter */
>  	rc = cc_trng_pm_get(dev);
> -	if (rc)
> -		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
> +	if (rc) {
> +		dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
> +		goto post_pm_err;
> +	}
>  
>  	/* set pending_hw to verify that HW won't be triggered from read */
>  	atomic_set(&drvdata->pending_hw, 1);
> @@ -534,8 +536,7 @@ static int cctrng_probe(struct platform_device *pdev)
>  	rc = devm_hwrng_register(dev, &drvdata->rng);
>  	if (rc) {
>  		dev_err(dev, "Could not register hwrng device.\n");
> -		cc_trng_pm_fini(drvdata);
> -		return rc;
> +		goto post_pm_err;
>  	}
>  
>  	/* trigger HW to start generate data */
> @@ -548,6 +549,11 @@ static int cctrng_probe(struct platform_device *pdev)
>  	dev_info(dev, "ARM cctrng device initialized\n");
>  
>  	return 0;
> +
> +post_pm_err:
> +	cc_trng_pm_fini(drvdata);
> +
> +	return rc;

[Severity: Medium]
Does calling cc_trng_pm_fini() in the post_pm_err error path cause an
unbalanced pm_runtime_disable() and leak a dev->power.disable_depth
reference when cc_trng_pm_get() or devm_hwrng_register() fails?

In drivers/char/hw_random/cctrng.c, cc_trng_pm_init() only configures
autosuspend and sets the runtime status to active without enabling runtime
PM:

static int cc_trng_pm_init(struct cctrng_drvdata *drvdata)
{
	struct device *dev = &(drvdata->pdev->dev);

	/* must be before the enabling to avoid redundant suspending */
	pm_runtime_set_autosuspend_delay(dev, CC_TRNG_SUSPEND_TIMEOUT);
	pm_runtime_use_autosuspend(dev);
	/* set us as active - note we won't do PM ops until cc_trng_pm_go()! */
	return pm_runtime_set_active(dev);
}

Runtime PM is only enabled later in cctrng_probe() via
cc_trng_pm_go() -> pm_runtime_enable(), after both cc_trng_pm_get() and
devm_hwrng_register() have succeeded:

static void cc_trng_pm_go(struct cctrng_drvdata *drvdata)
{
	struct device *dev = &(drvdata->pdev->dev);

	/* enable the PM module*/
	pm_runtime_enable(dev);
}

static void cc_trng_pm_fini(struct cctrng_drvdata *drvdata)
{
	struct device *dev = &(drvdata->pdev->dev);

	pm_runtime_disable(dev);
}

Because cc_trng_pm_fini() calls pm_runtime_disable(), it pairs with
cc_trng_pm_go() rather than cc_trng_pm_init(). Jumping to post_pm_err before
cc_trng_pm_go() has run calls pm_runtime_disable() when runtime PM was never
enabled, incrementing dev->power.disable_depth from 1 to 2.

When cctrng_probe() returns an error, device_unbind_cleanup() calls
pm_runtime_reinit() in drivers/base/power/runtime.c, which already resets
runtime_status to RPM_SUSPENDED and clears use_autosuspend when runtime PM
is disabled, but does not reset dev->power.disable_depth. If the device is
subsequently re-probed, cc_trng_pm_go() only decrements disable_depth from 2
to 1, leaving runtime PM permanently disabled for the device.

In addition, if devm_hwrng_register() fails after cc_trng_pm_get() succeeds,
wouldn't cctrng_probe() need to undo the usage count increment from
cc_trng_pm_get() rather than calling cc_trng_pm_fini()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b66b3df3d178a04b35690d0eb36dee409a95aa5d.1792782868.git.christophe.jaillet@wanadoo.fr?part=1