Re: [PATCH v8 3/3] pwm: dwc: add of/platform support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Incorrect PWM polarity mapping and violation of the atomic PWM API by using pwm->args instead of state for 0N100PWM-capable hardware.
- [High] System suspend unconditionally fails if any PWM channel is enabled, breaking system-wide sleep transitions.
- [High] PM runtime usage counter leak in dwc_pwm_resume when system resume fails.
- [High] The function dwc_pwm_get_state() divides by the clock rate without verifying if it is zero, leading to a kernel panic.
- [High] The driver ignores the return value of pm_runtime_get_sync() in dwc_pwm_plat_remove(), leading to potential bus faults.
- [Medium] The error handling path in dwc_pwm_plat_probe() double-disables the device clocks if no PWM channels are initially enabled.

Pre-existing issues:
- [High] Unchecked pm_runtime_get_sync failure and leaked reference in dwc_pwm_apply.
--

commit 061974fbba26ee19760c596a4145184402b90275
Author: Xuyang Dong <dongxuyang@eswincomputing.com>

pwm: dwc: add of/platform support

The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.

The controller is reset only when no PWM channel is enabled.
Otherwise, clocks are enabled and the runtime PM state is updated
to reflect the active hardware configuration.

> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 6dabec93a3c64..b9a9d6ae0b45b 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -44,21 +46,55 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,

[ ... ]

> +	if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN) {
> +		/*
> +		 * Calculate width of low and high period in terms of input
> +		 * clock periods and check are the result within HW limits
> +		 * between 0 and 2^32 periods.
> +		 * Use mul_u64_u64_div_u64() to avoid overflowing the 64-bit
> +		 * intermediate result and to round down to the nearest
> +		 * achievable hardware value, as required by the PWM core.
> +		 */
> +		tmp = mul_u64_u64_div_u64(state->duty_cycle, dwc->clk_rate,
> +					  NSEC_PER_SEC);
> +		if (tmp >= (1ULL << 32))
> +			return -ERANGE;
> +
> +		if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +			high = tmp;
> +		else
> +			low = tmp;

[Severity: High]
Is it correct to evaluate the static default pwm->args.polarity here
instead of state->polarity?

If the args polarity is INVERSED, this logic maps the duty_cycle to the
HIGH period. This causes the signal to be active-high, directly violating
the requested INVERSED state from the PWM core.

[ ... ]

> @@ -121,10 +161,19 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     struct pwm_state *state)
>  {
>  	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +	unsigned long clk_rate;
>  	u64 duty, period;
>  	u32 ctrl, ld, ld2;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(pwmchip_parent(chip));
> +	if (ret)
> +		return ret;

[Severity: High]
This is a pre-existing issue, but while looking at the runtime PM handling
here, I noticed that dwc_pwm_apply() (not modified by this patch) calls
pm_runtime_get_sync(pwmchip_parent(chip)) without checking its return value.

If resuming the parent device fails, dwc_pwm_apply() proceeds to call
__dwc_pwm_configure_timer(), which attempts to access hardware registers
while clocks might be disabled, potentially leading to a synchronous external
abort. It also permanently leaks the PM usage reference on failure. Should
dwc_pwm_apply() also be updated to check for failure?

> -	pm_runtime_get_sync(pwmchip_parent(chip));
> +	if (dwc->clk)
> +		dwc->clk_rate = clk_get_rate(dwc->clk);
> +
> +	clk_rate = dwc->clk_rate;

[ ... ]

>  	if (ctrl & DWC_TIM_CTRL_PWM) {
> -		duty = (ld + 1) * dwc->clk_ns;
> -		period = (ld2 + 1)  * dwc->clk_ns;
> -		period += duty;
> +		if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN) {
> +			if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +				duty = ld2;

[Severity: High]
Similar to the issue above in __dwc_pwm_configure_timer(), should this
check state->polarity instead of pwm->args.polarity to correctly report
the active duty cycle?

> +			else
> +				duty = ld;
> +			period = (u64)ld + ld2;
> +		} else {
> +			duty = ld + 1;
> +			period = ld2 + 1;
> +			period += duty;
> +		}
>  	} else {
> -		duty = (ld + 1) * dwc->clk_ns;
> +		duty = ld + 1;
>  		period = duty * 2;
>  	}
>  
>  	state->polarity = PWM_POLARITY_INVERSED;
> -	state->period = period;
> -	state->duty_cycle = duty;
> +	state->period = mul_u64_u64_div_u64(period, NSEC_PER_SEC, clk_rate);
> +	state->duty_cycle = mul_u64_u64_div_u64(duty, NSEC_PER_SEC, clk_rate);

[Severity: High]
Could clk_rate be zero here if the device tree provides an uninitialized or
dummy clock?

dwc->clk_rate is initialized using clk_get_rate() during probe without
verifying if it is non-zero. If it is 0, these divisions will cause a kernel
panic when the PWM core calls get_state() during pwmchip_add().

> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> new file mode 100644
> index 0000000000000..34713cb7299e6
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c

[ ... ]

> +	/*
> +	 * If any PWM channel is enabled, mark device active and hold runtime PM
> +	 * references for each enabled channel. Otherwise, gate the clocks.
> +	 */
> +	if (pwm_en) {
> +		pm_runtime_set_active(dev);
> +		for (i = 0; i < chip->npwm; i++) {
> +			if (ctrl[i])
> +				pm_runtime_get_noresume(dev);
> +		}
> +	} else {
> +		clk_disable_unprepare(dwc->clk);
> +		clk_disable_unprepare(dwc->bus_clk);
> +	}
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = pwmchip_add(chip);
> +	if (ret) {
> +		dev_err(dev, "failed to add pwm chip\n");
> +		goto pm_disable;
> +	}
> +
> +	return 0;
> +
> +pm_disable:
> +	pm_runtime_disable(dev);
> +	if (pwm_en) {
> +		for (i = 0; i < chip->npwm; i++) {
> +			if (ctrl[i])
> +				pm_runtime_put_noidle(dev);
> +		}
> +	}
> +disable_clk:
> +	clk_disable_unprepare(dwc->clk);
> +disable_busclk:
> +	clk_disable_unprepare(dwc->bus_clk);

[Severity: Medium]
If pwmchip_add() fails when pwm_en is false, will this error path
double-disable the clocks?

During probe, if no PWM channels are enabled, dwc->clk and dwc->bus_clk
are explicitly disabled in the else branch above. If pwmchip_add() then
fails, the execution jumps to pm_disable and falls through to disable_clk
and disable_busclk, executing clk_disable_unprepare() on both clocks a
second time. This causes a clock reference count underflow.

[ ... ]

> +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
> +	struct dwc_pwm_drvdata *data = platform_get_drvdata(pdev);
> +	struct pwm_chip *chip = data->chips[0];
> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +	unsigned int idx;
> +	int ret;
> +
> +	pm_runtime_get_sync(&pdev->dev);

[Severity: High]
Does pm_runtime_get_sync() need its return value checked here?

If the PM framework fails to resume the device during removal (e.g., due to a
power domain failure), the device's clocks or power will remain off.

> +	pwmchip_remove(chip);
> +
> +	for (idx = 0; idx < chip->npwm; idx++) {
> +		if (dwc_pwm_readl(dwc, DWC_TIM_CTRL(idx)) & DWC_TIM_CTRL_EN)

[Severity: High]
This isn't a bug introduced in this exact line, but as a continuation of the
previous concern: if pm_runtime_get_sync() failed, this dwc_pwm_readl()
will access unclocked memory-mapped I/O, which triggers a bus fault.

[ ... ]

> +static int dwc_pwm_suspend(struct device *dev)
> +{
> +	struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
> +	struct pwm_chip *chip = data->chips[0];
> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +	unsigned int idx;
> +	int ret;
> +
> +	if (pm_runtime_status_suspended(dev)) {
> +		ret = dwc_pwm_runtime_resume(dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (idx = 0; idx < chip->npwm; idx++) {
> +		if (chip->pwms[idx].state.enabled)
> +			return -EBUSY;

[Severity: High]
Does returning -EBUSY when a PWM channel is enabled unintentionally break
system-wide sleep transitions?

If a userspace sysfs client or a consumer device leaves a PWM running, this
aborts the entire system suspend sequence.

[ ... ]

> +static int dwc_pwm_resume(struct device *dev)
> +{
> +	struct dwc_pwm_drvdata *data = dev_get_drvdata(dev);
> +	struct pwm_chip *chip = data->chips[0];
> +	struct dwc_pwm *dwc = to_dwc_pwm(chip);
> +	unsigned int idx;
> +	bool pm_flags;
> +	int ret;
> +
> +	/* Check if device was runtime suspended before system resume */
> +	pm_flags = pm_runtime_status_suspended(dev);
> +	if (pm_flags) {
> +		/*
> +		 * Use PM framework to resume device
> +		 * (calls dwc_pwm_runtime_resume)
> +		 */
> +		ret = pm_runtime_get_sync(dev);
> +		if (ret < 0)
> +			return ret;

[Severity: High]
Can this leak the PM usage counter on failure?

pm_runtime_get_sync() increments the PM usage counter even if it returns an
error. If this call fails, propagating the error early leaves the device's
usage counter incremented, preventing it from ever entering runtime suspend
again. Should pm_runtime_put_noidle() be called before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623071329.2034-1-dongxuyang@eswincomputing.com?part=3