Re: [PATCH v19 3/3] pwm: Add OpenCores PTC PWM driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Hardware state corruption and PM runtime leak in `ocores_pwm_apply` on invalid parameter request.
--

commit b3a12f20d0a4ac7f1d2dc7a3d103fe2acf522975
Author: Hal Feng <hal.feng@starfivetech.com>
Subject: pwm: Add OpenCores PTC PWM driver

This commit adds a PWM driver for the OpenCores PTC IP core.

> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pwm/pwm-ocores.c

[ ... ]

> +static int ocores_pwm_apply(struct pwm_chip *chip,
> +			    struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct ocores_pwm_device *ddata = pwmchip_get_drvdata(chip);
> +	bool was_enabled = pwm_is_enabled(pwm);
> +	u64 period_data, duty_data;
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	if (state->enabled) {
> +		if (!was_enabled) {
> +			ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> +			if (ret < 0)
> +				return ret;
> +		}
> +	} else {
> +		if (was_enabled) {
> +			writel(0, ddata->base + OCPWM_CTRL);
> +			pm_runtime_put(pwmchip_parent(chip));
> +		}
> +		return 0;
> +	}
> +
> +	writel(0, ddata->base + OCPWM_CTRL);
> +	writel(OCPWM_CTRL_RST, ddata->base + OCPWM_CTRL);
> +
> +	period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC);
> +	if (period_data > U32_MAX)
> +		period_data = U32_MAX;
> +
> +	duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, NSEC_PER_SEC);
> +	if (duty_data > U32_MAX)
> +		duty_data = U32_MAX;
> +
> +	if (!period_data || !duty_data) {
> +		if (!was_enabled)
> +			pm_runtime_put(pwmchip_parent(chip));
> +		return -EINVAL;

[Severity: High]
Does stopping and resetting the hardware before validating the requested
parameters cause state corruption if the validation fails?

If period_data or duty_data evaluate to 0, the function returns -EINVAL.
Since the hardware was already stopped and reset, the framework's software
state will show the PWM as enabled, but the physical hardware will remain
disabled.

Additionally, if was_enabled is true, does this error path leak the PM
runtime reference?

Since OCPWM_CTRL_EN was prematurely cleared, ocores_pwm_pm_disable() will
later see the enable bit as 0 and skip its pm_runtime_put_noidle() call:

ocores_pwm_pm_disable() {
    ...
	if (!pm_runtime_status_suspended(dev)) {
		if (readl(ddata->base + OCPWM_CTRL) & OCPWM_CTRL_EN)
			pm_runtime_put_noidle(dev);
    ...
}

> +	}
> +
> +	writel(period_data, ddata->base + OCPWM_LRC);
> +	writel(duty_data, ddata->base + OCPWM_HRC);
> +	writel(OCPWM_CTRL_OE | OCPWM_CTRL_EN, ddata->base + OCPWM_CTRL);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615155759.129210-1-hal.feng@starfivetech.com?part=3