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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Probe fails unconditionally when CONFIG_PM is disabled.
- [High] Teardown symmetry violation in `ocores_pwm_pm_disable()` leads to reset assertion without an active clock.
- [High] Race condition between `pm_runtime_enable()` and `pm_runtime_get_noresume()` allows premature hardware access without a clock.
- [High] Missing context restore on system resume leads to silently broken PWM output.
- [High] Runtime PM usage count leak permanently blocks PM suspend if the driver is unbound while the PWM is enabled.
- [Medium] Subsystem rule violation: Using asynchronous `pm_runtime_put()` before `pm_runtime_disable()` causes race conditions.
- [Medium] Hardware minimum period and duty cycle limitations are not enforced, allowing invalid configuration.
- [Medium] Unconditional hardware disable in probe() breaks bootloader handover.
- [Low] Masking of true error codes from devm_pwmchip_alloc().
--

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

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

> --- /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)
> +{
[ ... ]
> +	if (state->enabled) {
> +		if (!pwm_is_enabled(pwm)) {
> +			ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> +			if (ret < 0)
> +				return ret;
> +		}

Does this leak the runtime PM usage count if the driver is unbound while the
PWM is enabled?

If the driver is unbound via sysfs while the PWM is actively running, the PWM
core marks the chip non-operational but does not disable the running PWM
hardware. The devres teardown action ocores_pwm_pm_disable() will run and
forcefully turn off the hardware, but it doesn't decrement the elevated PM
usage counter.

Since the struct device object persists across bind/unbind cycles, could this
leaked usage count permanently prevent the device from entering runtime suspend
on subsequent binds?

> +	} else {
> +		if (pwm_is_enabled(pwm)) {
> +			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);

The header comment notes the hardware minimum period and duty cycle is 1 clock
cycle. If the requested state->period is smaller than one clock cycle, wouldn't
mul_u64_u32_div() silently truncate the result to 0?

Writing 0 to the OCPWM_LRC and OCPWM_HRC registers violates this hardware
constraint and might cause undefined IP behavior. Should the driver validate
that period_data >= 1 and return -EINVAL if the request cannot be satisfied?

> +	if (period_data > U32_MAX)
> +		period_data = U32_MAX;
[ ... ]
> +static int ocores_pwm_runtime_resume(struct device *dev)
> +{
> +	struct ocores_pwm_device *ddata = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(ddata->clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable clock\n");
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops ocores_pwm_pm_ops = {
> +	RUNTIME_PM_OPS(ocores_pwm_runtime_suspend,
> +		       ocores_pwm_runtime_resume, NULL)
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +			    pm_runtime_force_resume)
> +};

Will this silently break PWM output upon resuming from system sleep?

During system suspend, if the device's power domain is powered off, the
hardware register contents are typically lost.

The ocores_pwm_runtime_resume() callback only re-enables the clock; it doesn't
save and restore the PWM configuration registers. If a PWM was actively
outputting a signal before system suspend, will it fail to output the correct
signal after the system wakes up?

> +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))
> +		ocores_pwm_runtime_suspend(dev);
> +
> +	reset_control_assert(ddata->rst);
> +}

Could asserting the reset while the clock is turned off cause issues?

In ocores_pwm_probe(), the hardware initialization order is strictly clock
enable followed by reset deassertion. The explicit probe error path also
respects this by reversing the order, asserting reset before disabling the
clock.

However, ocores_pwm_pm_disable() calls ocores_pwm_runtime_suspend()
(which turns off the clock) before calling reset_control_assert(). If the IP
requires an active clock to propagate the reset signal, asserting the reset
while the clock is turned off might fail or hang the bus.

> +static int ocores_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ocores_pwm_device *ddata;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata));
> +	if (IS_ERR(chip))
> +		return -ENOMEM;

Should this return PTR_ERR(chip) instead of hardcoding -ENOMEM?

devm_pwmchip_alloc() returns an ERR_PTR that can propagate various error codes,
and returning -ENOMEM masks the true failure reason.

> +	chip->ops = &ocores_pwm_ops;
> +	ddata = pwmchip_get_drvdata(chip);
[ ... ]
> +	ret = reset_control_deassert(ddata->rst);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		goto err_reset_assert;

Does this fail probe unconditionally when CONFIG_PM is disabled?

When CONFIG_PM is disabled, the dummy implementation of
pm_runtime_set_active() returns 1. The if (ret) check evaluates to true
when ret = 1, causing the probe function to jump to the error path.

Should this check if (ret < 0) to distinguish PM framework errors from
successful status returns?

> +	pm_runtime_enable(dev);
> +
> +	ret = devm_add_action_or_reset(dev, ocores_pwm_pm_disable, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add pm disable action\n");
> +
> +	pm_runtime_get_noresume(dev);
> +
> +	writel(0, ddata->base + OCPWM_CTRL);

Are there a couple of issues with this sequence?

First, pm_runtime_enable() is called while the runtime PM usage counter is 0.
This opens a window where the PM core can asynchronously suspend the device.
The driver then calls pm_runtime_get_noresume(), which increments the usage
counter but does not resume the device if it was already suspended.

If the device was suspended during this window, the subsequent writel()
would access hardware registers while the clock is turned off.
Should pm_runtime_get_noresume() be called before pm_runtime_enable()?

Second, does this unconditional writel(0, ...) break bootloader handover?
If the bootloader previously configured and enabled the PWM, this write will
instantly power it down. Could this cause hardware state disruption until
the consumer driver re-enables it?

> +	pm_runtime_put(dev);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");

Is it safe to use the asynchronous pm_runtime_put() here?

If devm_pwmchip_add() fails, the devres teardown action ocores_pwm_pm_disable()
runs immediately and invokes pm_runtime_disable(). This forcefully cancels
the pending asynchronous suspend work from pm_runtime_put(), leaving the device
in an undefined PM state.

The Power Management subsystem guidelines dictate that pm_runtime_put_sync()
must be used instead of pm_runtime_put() when pm_runtime_disable() might follow
immediately.

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