linux-kernel - Re: [PATCH v4 3/4] pwm: tiehrpwm: ensure clock and runtime PM are enabled if hardware is active

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <xuprgpxmzbdi6yq74xpzpsuzytpuy7x5ektq2pt3l4vd7f2rhw@gpsxqzrhljg6>
Date: Tue, 13 May 2025 11:33:38 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: "Rafael V. Volkmer" <rafael.v.volkmer@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v4 3/4] pwm: tiehrpwm: ensure clock and runtime PM are
 enabled if hardware is active

Hello Rafael,

On Sat, Apr 19, 2025 at 04:58:30PM -0300, Rafael V. Volkmer wrote:
> During probe, if the hardware is already active, it is not guaranteed
> that the clock is enabled. To address this, ehrpwm_pwm_probe() now
> checks whether the PWM is enabled and ensures that the necessary
> resources are initialized.
> 
> Signed-off-by: Rafael V. Volkmer <rafael.v.volkmer@...il.com>
> ---
>  drivers/pwm/pwm-tiehrpwm.c | 46 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> index cde331a73696..23530d53e177 100644
> --- a/drivers/pwm/pwm-tiehrpwm.c
> +++ b/drivers/pwm/pwm-tiehrpwm.c
> @@ -583,15 +583,50 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct ehrpwm_pwm_chip *pc;
> +	struct pwm_device *pwm;
> +	struct pwm_state state;
>  	struct pwm_chip *chip;
>  	struct clk *clk;
> +	bool tbclk_enabled;
>  	int ret;
>  
> +	u16 aqcsfrc_reg, aqctl_reg;
> +
> +	u8 csf_bits;
> +
>  	chip = devm_pwmchip_alloc(&pdev->dev, NUM_PWM_CHANNEL, sizeof(*pc));
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  	pc = to_ehrpwm_pwm_chip(chip);
>  
> +	pwm = &chip->pwms[0];
> +
> +	if (pwm->hwpwm == 0) {

This is always true. And actually a lowlevel driver shouldn't need 
chip->pwms at all.

> +		aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> +		csf_bits = FIELD_GET(AQCSFRC_CSFA_MASK, aqcsfrc_reg);
> +		aqctl_reg = readw(pc->mmio_base + AQCTLA);
> +	} else {
> +		aqcsfrc_reg = readw(pc->mmio_base + AQCSFRC);
> +		csf_bits = FIELD_GET(AQCSFRC_CSFB_MASK, aqcsfrc_reg);
> +		aqctl_reg = readw(pc->mmio_base + AQCTLB);
> +	}
> +
> +	if (csf_bits)
> +		state.enabled = false;
> +	else if (aqctl_reg)
> +		state.enabled = true;
> +	else
> +		state.enabled = false;
> +
> +	if (state.enabled) {
> +		ret = clk_enable(pc->tbclk);
> +		if (ret) {
> +			dev_err_probe(&pdev->dev, ret, "clk_prepare_enable() failed");
> +			goto err_pwmchip_remove;
> +		}
> +		tbclk_enabled = true;
> +	}

You don't check the 2nd PWM, this is needed however. You even need to
call pm_runtime_get_sync() twice if both PWMs are on.

>  	clk = devm_clk_get(&pdev->dev, "fck");
>  	if (IS_ERR(clk)) {
>  		if (of_device_is_compatible(np, "ti,am33xx-ecap")) {
> @@ -626,6 +661,15 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	if (state.enabled) {
> +		ret = pm_runtime_get_sync(&pdev->dev);
> +		if (ret < 0) {
> +			dev_err_probe(&pdev->dev, ret, "pm_runtime_get_sync() failed");
> +			clk_disable_unprepare(pc->tbclk);
> +			goto err_pwmchip_remove;
> +			}

indention inconsistency.

> +	}
> +
>  	ret = pwmchip_add(chip);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> @@ -637,6 +681,8 @@ static int ehrpwm_pwm_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +err_pwmchip_remove:
> +		pwmchip_remove(chip);

That's wrong. When the `goto err_pwmchip_remove` is taken, pwmchip_add
wasn't called yet.

>  err_clk_unprepare:
>  	clk_unprepare(pc->tbclk);
>  

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)