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

> 
> 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 <[email protected]>
> 
> 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.
> 

Hi Sashiko,

Do not add a dependency on PM. Additionally, the issues below will be 
fixed to ensure compatibility with and without PM support.

> > 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.
> 

The PWM core's chip->nonatomic_lock (per-chip mutex) serializes
all apply() and get_state() calls across all channels on the same
chip. The apparent write to dwc->clk_rate in both
__dwc_pwm_configure_timer() and dwc_pwm_get_state() cannot race
because both code paths are reached only under this lock.
  
No locking concerns.

> > @@ -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()?
> 

It is the same as above.

> > 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.
> 

Deassert reset before reading MMIO registers. The channel-enable
scan reads DWC_TIM_CTRL while the device may still be held in reset
after a previous remove(). Reading MMIO while in reset causes a bus
stall on many SoCs; call reset_control_deassert() before the scan.

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

Keep clocks enabled when CONFIG_PM is disabled. When CONFIG_PM=n,
pm_runtime callbacks are stubs that never invoke the runtime_resume
hook, so clocks gated in probe are never re-enabled before MMIO
access in apply() and get_state(). Condition the clock gating on
IS_ENABLED(CONFIG_PM), and update the pm_disable error path to
disable clocks when CONFIG_PM=n (they were never gated in that case).

> [ ... ]
> > +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?
> 

With the above fix, the clocks in probe will not be disabled when 
CONFIG_PM=n. This prevents the double-disable issue.

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

Use pm_runtime_resume_and_get() instead of calling
dwc_pwm_runtime_resume() directly. 

Move the register read/write operations from dwc_pwm_suspend/resume() into 
dwc_pwm_runtime_suspend/resume(). 
Switch system sleep callbacks to pm_runtime_force_suspend() and
pm_runtime_force_resume(), instead of dwc_pwm_suspend() and
dwc_pwm_resume().

Would this approach be more suitable?

Best regards,
Xuyang Dong
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3