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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing CONFIG_PM dependency leads to unclocked MMIO accesses and kernel panics.
- [High] Bypassing the PM framework during system suspend causes bus faults on devices attached to a PM domain.
- [High] Reading MMIO registers while the device is in reset causes a bus stall and kernel panic.
- [Medium] Data race on the shared `dwc->clk_rate` variable.
--

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

pwm: dwc: add of/platform support

This patch adds platform and OF device tree support for the DesignWare PWM
controller. It introduces runtime PM support, manages clock enabling and
disabling based on PWM channel usage, and implements polarity control by
swapping load count registers.

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -249,6 +249,16 @@ config PWM_DWC
[ ... ]
> +config PWM_DWC_OF
> +	tristate "DesignWare PWM Controller (OF bus)"
> +	depends on HAS_IOMEM && (OF || COMPILE_TEST)

[Severity: High]
Should this include a dependency on PM?

If the kernel is compiled without CONFIG_PM, Kconfig allows building
PWM_DWC_OF. During probe(), if no PWMs are enabled, the driver explicitly
disables dwc->clk and dwc->bus_clk and relies on PM runtime to re-enable
them. Because CONFIG_PM is disabled, pm_runtime_resume_and_get() evaluates to
a stub that returns 1 (success) without calling the runtime resume callback.
This leads to unclocked register accesses and kernel panics.

> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c
> @@ -44,21 +46,73 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
[ ... ]
> -	high = tmp - 1;
> +	if (dwc->clk)
> +		dwc->clk_rate = clk_get_rate(dwc->clk);

[Severity: Medium]
Can this write to dwc->clk_rate cause a data race?

The PWM framework serializes operations on a per-channel basis, but not
across the entire controller. If multiple threads concurrently request state
changes (apply) or state reads (get_state) on different PWM channels of the
same controller, they might simultaneously write to this shared variable.

> @@ -121,10 +183,23 @@ static int dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
[ ... ]
> +	if (dwc->clk)
> +		dwc->clk_rate = clk_get_rate(dwc->clk);

[Severity: Medium]
Does this introduce a similar data race on dwc->clk_rate as the one mentioned
above in __dwc_pwm_configure_timer()?

> diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> --- /dev/null
> +++ b/drivers/pwm/pwm-dwc-of.c
> @@ -0,0 +1,356 @@
[ ... ]
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = clk_prepare_enable(dwc->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable timer clock\n");
> +		goto disable_busclk;
> +	}
> +
> +	/*
> +	 * Check all channels to see if any channel is enabled.
> +	 * Read the control register of each channel and extract the enable bit
> +	 */
> +	for (i = 0; i < chip->npwm; i++) {
> +		ctrl[i] = dwc_pwm_readl(dwc, DWC_TIM_CTRL(i)) & DWC_TIM_CTRL_EN;
> +		if (ctrl[i])
> +			pwm_en = true;
> +	}
> +
> +	/*
> +	 * Only issue a reset pulse when all channels are disabled, so a PWM
> +	 * channel already running (e.g. configured by firmware before Linux
> +	 * took over) is left undisturbed.
> +	 */
> +	if (!pwm_en) {
> +		ret = reset_control_reset(dwc->rst);

[Severity: High]
Could this read from DWC_TIM_CTRL(i) cause a bus stall and kernel panic if the
device is currently in reset?

Module removal explicitly asserts the reset line in dwc_pwm_plat_remove(),
leaving the device in reset. On a subsequent probe (e.g., reloading the
module), this loop attempts to read from the MMIO space before the reset is
deasserted.

[ ... ]
> +	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);
> +	}

[Severity: High]
If CONFIG_PM is disabled in the kernel, does gating these clocks here cause
subsequent unclocked register accesses?

Since pm_runtime_resume_and_get() becomes a stub that returns 1 without
re-enabling clocks, subsequent operations like apply() or get_state() would
perform unclocked MMIO accesses leading to panics.

[ ... ]
> +static void dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
[ ... ]
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
> +		clk_disable_unprepare(dwc->clk);
> +		clk_disable_unprepare(dwc->bus_clk);
> +	}

[Severity: High]
If CONFIG_PM is disabled, pm_runtime_status_suspended() will always return
false. Does this cause a double-disable of the clocks if they were already
gated in probe?

[ ... ]
> +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++) {
> +		dwc->ctx[idx].cnt = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(idx));

[Severity: High]
Does calling the local dwc_pwm_runtime_resume() directly bypass the PM
framework?

If the device is attached to a generic power domain (genpd) and is
runtime-suspended, the PM domain itself might be powered off. Bypassing
pm_runtime_resume_and_get() means the PM domain is never powered back on,
and the subsequent dwc_pwm_readl() calls will access unpowered hardware
causing a bus fault.

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