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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware reset is asserted after gating clocks, which can cause bus hangs.
- [High] Race condition between runtime PM check and disable causes a permanent clock leak.

Pre-existing issues:
- [High] Unmasking the timer interrupt without an IRQ handler can cause a severe interrupt storm.
--

commit a50609ba96938c02fbc94f54318c95561f661758
Author: Xuyang Dong <dongxuyang@eswincomputing.com>
Subject: 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. Implements polarity control by exploiting 
the timer's dual load registers.

> diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> index 6dabec93a3c64..49f0657d4ad44 100644
> --- a/drivers/pwm/pwm-dwc-core.c
> +++ b/drivers/pwm/pwm-dwc-core.c

[ ... ]

> @@ -85,6 +138,9 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
>  	 * periods are set by Load Count registers.
>  	 */
>  	ctrl = DWC_TIM_CTRL_MODE_USER | DWC_TIM_CTRL_PWM;
> +	if (dwc->features & DWC_TIM_CTRL_0N100PWM_EN)
> +		ctrl |= DWC_TIM_CTRL_0N100PWM_EN;
> +
>  	dwc_pwm_writel(dwc, ctrl, DWC_TIM_CTRL(pwm->hwpwm));

[Severity: High]
This isn't a bug introduced by this patch, but does writing this control
register unmask the timer interrupt?

The ctrl value is constructed without DWC_TIM_CTRL_INT_MASK set. If this
hardware is on an OF platform where interrupts are commonly shared, could
this unmasked timer flood the system with interrupts since there is no
IRQ handler?

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

[ ... ]

> +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;
> +
> +	pwmchip_remove(chip);
> +
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		dev_warn(&pdev->dev, "failed to resume device: %d\n", ret);
> +	} else {
> +		for (idx = 0; idx < chip->npwm; idx++) {
> +			if (dwc_pwm_readl(dwc, DWC_TIM_CTRL(idx)) &
> +					  DWC_TIM_CTRL_EN)
> +				pm_runtime_put_noidle(&pdev->dev);
> +		}
> +		pm_runtime_put_sync(&pdev->dev);
> +	}
> +
> +	if (!pm_runtime_status_suspended(&pdev->dev)) {
> +		clk_disable_unprepare(dwc->clk);
> +		clk_disable_unprepare(dwc->bus_clk);
> +	}
> +	pm_runtime_disable(&pdev->dev);

[Severity: High]
Could this ordering cause a permanent clock leak if a concurrent runtime
PM resume occurs?

If userspace triggers a resume (e.g. via sysfs) between the
pm_runtime_status_suspended() check and pm_runtime_disable(), the
dwc_pwm_runtime_resume() callback will re-enable the clocks.

Once pm_runtime_disable() runs, the PM state is frozen, but the driver will
have lost track of the re-enabled clocks. Should pm_runtime_disable() be
called before manually rolling back the clock state?

> +
> +	if (dwc->rst) {
> +		ret = reset_control_assert(dwc->rst);

[Severity: High]
Does asserting the reset while the IP block's clocks are disabled cause
a bus lockup?

In dwc_pwm_plat_remove(), the clocks are guaranteed to be off here
(either implicitly by allowing the device to suspend, or explicitly via the
clk_disable_unprepare() calls just above).

On AMBA/APB buses, asserting a reset on an unclocked IP block often prevents
the reset signal from propagating, leading to a system hang when the reset
controller attempts to complete the transaction.

> +		if (ret)
> +			dev_warn(&pdev->dev, "failed to assert reset: %d\n",
> +				 ret);
> +	}
> +}

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