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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Disabling the PWM in the `.free` callback causes state desynchronization, leading to an unclocked register access and system crash or unbalanced PM references on subsequent usage.
- [High] The driver's reported inversed polarity contradicts its disabled state behavior, turning the output ON when consumers try to disable it.
- [High] The driver rejects valid 0% duty cycle requests with `-EINVAL`.
- [High] Incorrect teardown ordering in `ocores_pwm_pm_disable` asserts the reset line while the IP clock is disabled.
--

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

This patch adds a PWM driver for the OpenCores PTC IP core. It supports
basic PWM functionality and is intended for StarFive SoCs.

> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> index 0000000000000..e01494a336335
> --- /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) {
> +			writel(0, ddata->base + OCPWM_CTRL);

[Severity: High]
Does this drive the output to the active state when the PWM is disabled?

ocores_pwm_get_state() hardcodes the polarity to PWM_POLARITY_INVERSED. Under
inverted polarity, the active state is LOW and the inactive state is HIGH. The
PWM framework requires that when a PWM is disabled, it must emit its inactive
state.

However, writing 0 to OCPWM_CTRL immediately sets the output to LOW (the
active state). This means disabling the PWM results in a 100% duty cycle
instead of 0%.

> +			pm_runtime_put(pwmchip_parent(chip));
> +		}
> +		return 0;
> +	}
> +
> +	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)
> +		return -EINVAL;

[Severity: High]
Does this code reject a valid 0% duty cycle request?

A 0% duty cycle is a fully valid configuration in the PWM subsystem, often used
by consumers like pwm-backlight to dim the output to zero without fully
disabling the PWM. Returning -EINVAL when duty_data evaluates to 0 breaks
these consumers.

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

[Severity: High]
Can this logic write to hardware registers while the clock is disabled?

If a consumer frees the PWM, ocores_pwm_free() disables the hardware and
drops the PM runtime reference. However, the PWM framework does not update
the cached pwm->state.enabled when a PWM is freed.

If the PWM is subsequently requested again and ocores_pwm_apply() is called,
was_enabled (which comes from pwm_is_enabled(pwm)) will still evaluate to
true. If the new consumer sets state->enabled to true, it will skip
pm_runtime_resume_and_get() and proceed to write directly to the PWM
registers with the IP clock disabled, resulting in a synchronous external
abort or crash. Alternatively, if the new consumer sets enabled to false,
it will call pm_runtime_put(), leading to a usecount underflow.

> +
> +	writel(0, ddata->base + OCPWM_CTRL);
> +	writel(OCPWM_CTRL_RST, ddata->base + OCPWM_CTRL);
> +
[ ... ]
> +static void ocores_pwm_pm_disable(void *data)
> +{
> +	struct device *dev = data;
> +	struct ocores_pwm_device *ddata = dev_get_drvdata(dev);
> +
> +	pm_runtime_disable(dev);
> +
> +	if (!pm_runtime_status_suspended(dev)) {
> +		/* Balance probe's pm_runtime_get_noresume() for bootloader-enabled PWM. */
> +		if (readl(ddata->base + OCPWM_CTRL) & OCPWM_CTRL_EN)
> +			pm_runtime_put_noidle(dev);
> +
> +		ocores_pwm_runtime_suspend(dev);
> +	}
> +
> +	reset_control_assert(ddata->rst);

[Severity: High]
Does this ordering assert the reset signal while the IP clock is disabled?

The code calls ocores_pwm_runtime_suspend() (which disables the clock) before
calling reset_control_assert(). Asserting a reset line while the corresponding
IP clock is disabled can cause bus hangs on APB buses or fail to propagate the
reset correctly into the hardware block.

The error path in ocores_pwm_probe() demonstrates the correct teardown
ordering by asserting the reset before suspending the runtime PM.

> +}

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