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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware state desynchronization in `.free` causes unclocked register accesses.
- [Medium] Incorrect rejection of small valid duty cycles leading to API contract violation.
--

commit b5c8de1f767ebe7d5f7953c6daf99b312944518f
Author: Hal Feng <hal.feng@starfivetech.com>

pwm: Add OpenCores PTC PWM driver

Add PWM driver for OpenCores PTC IP core.

> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> index 0000000000000..297c24d5805ce
> --- /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)
> +{
[ ... ]
> +	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 && state->duty_cycle))
> +		return -EINVAL;

[Severity: Medium]
Does this code reject valid, small duty cycles that round down to 0?

The PWM framework requires drivers to satisfy requests by configuring the
closest achievable hardware state, preferring to round down. Returning -EINVAL
here instead of allowing a 0% duty cycle breaks this API contract.

> +
> +	if (!was_enabled) {
> +		ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> +		if (ret < 0)
> +			return ret;
> +	}

[Severity: High]
Can this result in unclocked register accesses causing a bus fault?

If a consumer frees an enabled PWM, ocores_pwm_free() disables the hardware
and drops the pm_runtime reference. However, the PWM core does not update
state.enabled on free.

If the PWM is requested again and pwm_apply() is called with enabled = true,
was_enabled evaluates to true because of the stale cached state. This skips
pm_runtime_resume_and_get() and writes to registers with the clock disabled.

> +
> +	writel(0, ddata->base + OCPWM_CTRL);
> +	writel(OCPWM_CTRL_RST, ddata->base + OCPWM_CTRL);
> +
> +	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;
> +}
> +
> +static void ocores_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct ocores_pwm_device *ddata = pwmchip_get_drvdata(chip);
> +
> +	if (pwm_is_enabled(pwm)) {
> +		writel(0, ddata->base + OCPWM_CTRL);
> +		pm_runtime_put_sync(pwmchip_parent(chip));
> +	}
> +}

[Severity: High]
Does disabling the hardware here violate the PWM framework's contract?

Freed PWMs are expected to remain in their current state. Dropping the PM
reference without notifying the PWM core desynchronizes the hardware state
from the framework's cached state, triggering the unclocked access in
ocores_pwm_apply() described above.

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