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

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

Hi Sashiko,

Will add DWC_TIM_CTRL_INT_MASK in the next version.

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

Call pm_runtime_disable() 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.
> 

Remove reset assert in remove function, as disabling PM and clocks
prevents further operation

Best regards,
Xuyang Dong

> > +		if (ret)
> > +			dev_warn(&pdev->dev, "failed to assert reset: %d\n",
> > +				 ret);
> > +	}
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3