Re: [PATCH] pwm: fsl-ftm: Unwind resume failures

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3008 bytes --]

On Fri, Aug 28, 2026 at 05:20:14PM +0800, Pengpeng Hou wrote:
> fsl_pwm_resume() ignores all clock-enable and register-cache replay
> failures.  A failure can leave a prefix of requested PWM channels
> holding clocks while runtime PM reports the chip resumed.
> 
> Check each acquisition and regcache_sync(), then unwind completed
> channels in the reverse of the suspend order.  Restore cache-only and
> dirty state when the register replay fails.
> 
> The issue was identified via static analysis and manually reviewed.
> 
> Fixes: 97d0b42e39a7 ("pwm: ftm: Add Power Management support for FTM PWM")
> 

This empty line should be dropped.

> Assisted-by: LLM
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/pwm/pwm-fsl-ftm.c | 44 +++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> index 35406b2e1925..7a61ccb75963 100644
> --- a/drivers/pwm/pwm-fsl-ftm.c
> +++ b/drivers/pwm/pwm-fsl-ftm.c
> @@ -501,7 +501,7 @@ static int fsl_pwm_resume(struct device *dev)
>  {
>  	struct pwm_chip *chip = dev_get_drvdata(dev);
>  	struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
> -	int i;
> +	int i, ret;
>  
>  	for (i = 0; i < chip->npwm; i++) {
>  		struct pwm_device *pwm = &chip->pwms[i];
> @@ -509,20 +509,52 @@ static int fsl_pwm_resume(struct device *dev)
>  		if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>  			continue;
>  
> -		clk_prepare_enable(fpc->ipg_clk);
> +		ret = clk_prepare_enable(fpc->ipg_clk);
> +		if (ret)
> +			goto unwind;
>  
>  		if (!pwm_is_enabled(pwm))
>  			continue;
>  
> -		clk_prepare_enable(fpc->clk[fpc->period.clk_select]);
> -		clk_prepare_enable(fpc->clk[FSL_PWM_CLK_CNTEN]);
> +		ret = clk_prepare_enable(fpc->clk[fpc->period.clk_select]);
> +		if (ret) {
> +			clk_disable_unprepare(fpc->ipg_clk);
> +			goto unwind;
> +		}
> +
> +		ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_CNTEN]);
> +		if (ret) {
> +			clk_disable_unprepare(fpc->clk[fpc->period.clk_select]);
> +			clk_disable_unprepare(fpc->ipg_clk);
> +			goto unwind;
> +		}
>  	}
>  
>  	/* restore all registers from cache */
>  	regcache_cache_only(fpc->regmap, false);
> -	regcache_sync(fpc->regmap);
> +	ret = regcache_sync(fpc->regmap);
> +	if (!ret)
> +		return 0;

I think this is semantically right, but still surprising as it looks
like error handling.

>  
> -	return 0;
> +	regcache_cache_only(fpc->regmap, true);
> +	regcache_mark_dirty(fpc->regmap);
> +
> +unwind:
> +	while (i--) {
> +		struct pwm_device *pwm = &chip->pwms[i];
> +
> +		if (!test_bit(PWMF_REQUESTED, &pwm->flags))
> +			continue;
> +
> +		if (pwm_is_enabled(pwm)) {
> +			clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_CNTEN]);
> +			clk_disable_unprepare(fpc->clk[fpc->period.clk_select]);
> +		}
> +
> +		clk_disable_unprepare(fpc->ipg_clk);
> +	}
> +
> +	return ret;
>  }
>  #endif

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]