Re: [PATCH v7 2/2] pwm: dwc: add of/platform support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Registering the PWM chip before completing runtime PM initialization exposes the hardware to consumers while clocks might be turned off.
- [High] Manual hardware teardown in `.remove()` precedes `pwm_chip` deregistration, leading to potential bus faults.
- [High] TOCTOU race in `remove()` when conditionally acquiring a runtime PM reference allows concurrent runtime suspend while accessing registers.
- [High] System resume is incorrectly aborted if expected runtime PM idle operations return `-EAGAIN`.
- [High] Unconditional assertion of the reset control on the probe error path and during module removal permanently kills active hardware and unbalances the reset API.
- [High] `remove()` artificially drops runtime PM references that belong to active consumers, causing an underflow.
- [Medium] Integer overflow in timer configuration allows invalid large duty cycle/period values to bypass validation.
- [Medium] Clock references are leaked during module removal if the device is held in an active PM state but no PWM channels are enabled.
- [Medium] The driver rounds UP duty cycle and period calculations instead of rounding DOWN, violating PWM subsystem rules.
- [Medium] The driver evaluates the static device tree polarity instead of the dynamically requested runtime polarity, mapping the duty cycle incorrectly.

Pre-existing issues:
- [High] `dwc_pwm_get_state()` ignores the return value of `pm_runtime_get_sync()`, allowing unprotected hardware access.
--

commit 517966ff9e8e50806716056dd4bd3f2f88b832c0
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.

> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 6dabec93a3c64..a6de05e273216 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
[ ... ]
> @@ -44,21 +45,52 @@ 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.
> +		 */
> +		tmp = state->duty_cycle * dwc->clk_rate;

[Severity: Medium]
Could this multiplication overflow the 64-bit boundary before the hardware
limit check if a consumer requests an extremely large period? Both duty_cycle
and period are u64 nanoseconds, while clk_rate is an unsigned long. A
wrapped-around small value would evade the hardware limit check entirely.
Should this use mul_u64_u64_div_u64() to scale the values safely?

> +		tmp = DIV_ROUND_UP_ULL(tmp, NSEC_PER_SEC);

[Severity: Medium]
Does this violate the PWM subsystem rules by rounding up instead of down?
The PWM core framework strictly requires that the .apply callback rounds down
the requested period and duty cycle to the nearest achievable hardware value.

> +		if (tmp >= (1ULL << 32))
> +			return -ERANGE;
> +
> +		if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> +			high = tmp;
> +		else
> +			low = tmp;

[Severity: Medium]
Should this evaluate the dynamically requested state->polarity instead of the
static device tree polarity in pwm->args.polarity? Since dwc_pwm_apply()
enforces PWM_POLARITY_INVERSED, if the device tree was configured with
PWM_POLARITY_NORMAL, this will program the hardware with the inverted duty
cycle relative to what the consumer actually requested.

[ ... ]
> @@ -121,11 +157,17 @@ 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;
>  
>  	pm_runtime_get_sync(pwmchip_parent(chip));

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
pm_runtime_get_sync() is ignored here. If the function fails (for example,
returning -EACCES when PM runtime is disabled during driver removal, or before
it is enabled in probe), the hardware might not be powered on and clocks could
remain gated. The function will then proceed to read the DWC_TIM_CTRL and
DWC_TIM_LD_CNT registers, which can cause a synchronous external abort when
accessing unclocked hardware. Can we add a check for the return value here?

> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> new file mode 100644
> index 0000000000000..d8b1606e51af4
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c
[ ... ]
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret) {
> +		dev_err(dev, "failed to add pwm chip\n");
> +		goto reset_assert;
> +	}
> +
> +	data->chips[0] = chip;
> +	dev_set_drvdata(dev, data);
> +
> +	/*
> +	 * 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);

[Severity: High]
Does registering the PWM chip before enabling PM expose the hardware to
consumers prematurely? devm_pwmchip_add() is called before pm_runtime_enable().
If a consumer accesses the PWM during this window, the core driver's
pm_runtime_get_sync() call would fail with -EACCES, but still proceed to access
registers. At the same time, probe might gate the clocks if pwm_en is false,
leading to a bus fault.

> +
> +	return 0;
> +
> +reset_assert:
> +	reset_control_assert(dwc->rst);

[Severity: High]
Does this unconditionally assert the reset control on the error path? The
commit message mentions resetting only when no PWM channel is enabled. If
pwm_en was true, this forcibly kills actively running hardware. If pwm_en was
false, probe used reset_control_reset() (which is a pulse), so asserting it
here leaves the line permanently asserted in an unbalanced state.

> +disable_clk:
> +	clk_disable_unprepare(dwc->clk);
> +disable_busclk:
> +	clk_disable_unprepare(dwc->bus_clk);
> +
> +	return ret;
> +}
> +
> +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
[ ... ]
> +	/*
> +	 * Resume the device if it is runtime suspended to allow
> +	 * safe register access.
> +	 */
> +	pm_flags = pm_runtime_status_suspended(&pdev->dev);
> +	if (pm_flags)
> +		pm_runtime_get_sync(&pdev->dev);

[Severity: High]
Is there a race condition when conditionally acquiring this PM reference? If
pm_runtime_status_suspended() returns false, the code proceeds to read
registers without taking a reference. An auto-suspend timer or a concurrent
consumer could suspend the device immediately after this check, turning off the
clocks and causing the subsequent DWC_TIM_CTRL reads to trigger a bus fault.
Should pm_runtime_get_sync() be called unconditionally?

> +
> +	for (idx = 0; idx < chip->npwm; idx++) {
> +		if (dwc_pwm_readl(dwc, DWC_TIM_CTRL(idx)) & DWC_TIM_CTRL_EN) {
> +			pwm_en = true;
> +			pm_runtime_put_noidle(&pdev->dev);
> +		}
> +	}

[Severity: High]
Does dropping the PM reference here underflow the usage count for active
consumers? If a consumer has taken over the PWM channel, it logically owns that
reference via dwc_pwm_apply(). Because devres unregisters the PWM chip after
remove() finishes, the PWM core will eventually call
dwc_pwm_apply(..., enabled=false), which calls pm_runtime_put_sync(). Dropping
it here artificially might lead to a usage count underflow.

> +
> +	/*
> +	 * Re-suspend the device if it was runtime suspended prior to
> +	 * the register access.
> +	 */
> +	if (pm_flags)
> +		pm_runtime_put_sync(&pdev->dev);
> +
> +	if (pwm_en) {
> +		clk_disable_unprepare(dwc->clk);
> +		clk_disable_unprepare(dwc->bus_clk);
> +	}

[Severity: Medium]
Will this leak clock references if the device is active but no PWM channels are
enabled? If pwm_en is false but the device's PM runtime state is RPM_ACTIVE
(due to an open sysfs handle or a pending auto-suspend), skipping this block
leaves the clocks on. The subsequent pm_runtime_disable() does not turn off
clocks, permanently leaking them.

> +
> +	pm_runtime_disable(&pdev->dev);
> +	reset_control_assert(dwc->rst);
> +}

[Severity: High]
Does manually tearing down the hardware here lead to bus faults during devres
cleanup? Since the driver registers the chip using devm_pwmchip_add(), devres
will unregister the PWM chip after dwc_pwm_plat_remove() returns. During that
window, the device is fully accessible to consumers, but clocks are off and
reset is asserted. Additionally, unconditionally asserting the reset here
ignores the earlier logic to keep active hardware running if pwm_en was true.

[ ... ]
> +static int dwc_pwm_resume(struct device *dev)
> +{
[ ... ]
> +	for (idx = 0; idx < chip->npwm; idx++) {
> +		dwc_pwm_writel(dwc, dwc->ctx[idx].cnt, DWC_TIM_LD_CNT(idx));
> +		dwc_pwm_writel(dwc, dwc->ctx[idx].cnt2, DWC_TIM_LD_CNT2(idx));
> +		dwc_pwm_writel(dwc, dwc->ctx[idx].ctrl, DWC_TIM_CTRL(idx));
> +	}
> +
> +	if (pm_flags) {
> +		/* Balance the refcount taken by pm_runtime_get_sync
> +		 * if it was used
> +		 */
> +		ret = pm_runtime_put_sync(dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Should a legitimate -EAGAIN from pm_runtime_put_sync() abort the system resume?
According to PM subsystem rules, pm_runtime_put_sync() can return -EAGAIN if
another thread holds a usage reference. Returning this expected error code from
a system resume callback will unexpectedly abort the entire system resume
process for the device.

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