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

[Martin Kaiser <martin@kaiser.cx>]

The sashiko review makes sense to me. We should probably call
pm_runtime_put_noidle after post_pm_err.

Best regards,
Martin

Thus wrote Christophe JAILLET (christophe.jaillet@wanadoo.fr):

> If an error occurs after a successful cc_trng_pm_init() call, it must be
> undone by a corresponding cc_trng_pm_fini() call.

> The commit in Fixes has erroneously updated the error handling path if
> cc_trng_pm_get() fails.

> Fixes: 8bcd96893848 ("hwrng: cctrng - let devres enable the clock")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested only.

> This patch also undo the clean-up done by commit a7ea3d63e897 ("hwrng:
> cctrng - simplify cctrng_probe()"). Keeping an error path makes sense with
> 2 users. But if preferred, the missing cc_trng_pm_fini() could also be
> added after the test for errors in cc_trng_pm_get().
> ---
>  drivers/char/hw_random/cctrng.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index 03401ee59a8e..608b3321c05a 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;
>  }

>  static void cctrng_remove(struct platform_device *pdev)
> -- 
> 2.56.0