Re: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
- [High] Register context loss on suspend/resume silently breaks PWM output.
- [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
--

--- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
commit 0c98703d857577902e57d25b42a3d46dfbde5795
Author: Naushir Patuck <naush@raspberrypi.com>

pwm: rp1: Add RP1 PWM controller driver

The Raspberry Pi RP1 southbridge features an embedded PWM controller with
4 output channels, alongside an RPM interface to read the fan speed on the
Raspberry Pi 5. Add the supporting driver.

> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c

[ ... ]

> +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const void *_wfhw,
> +					 struct pwm_waveform *wf)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u64 clk_rate = rp1->clk_rate;
> +	u64 ticks;
> +
> +	*wf = (struct pwm_waveform){ };
> +
> +	if (!wfhw->enabled)
> +		return 0;
> +
> +	wf->period_length_ns = DIV_ROUND_UP_ULL(((u64)wfhw->period_ticks + 1) * NSEC_PER_SEC,
> +						clk_rate);
> +
> +	if (!wfhw->inverted_polarity) {
> +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> +						      (u32)clk_rate);
> +	} else {

[Severity: High]
Does the normal polarity case need to clamp the duty ticks similar to the
inverted polarity case?

If the bootloader configures a continuous-high state where the raw hardware
duty ticks exceed the period ticks + 1, this path calculates a duty_length_ns
that is strictly greater than the period_length_ns.

When the PWM core subsequently reads this initial state, pwm_wf2state()
handles duty > period by treating it as an inverted polarity and calculating
duty_cycle = period - duty_length_ns. This subtraction can silently
underflow, causing the core to interpret the hardware state as having a badly
corrupted duty cycle.

> +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> +			/* 100% duty cycle case */
> +			ticks = 0;
> +		} else {

[ ... ]

> +static int rp1_pwm_write_waveform(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  const void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u32 value, ctrl;
> +
> +	/* set polarity */
> +	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
> +	if (!wfhw->inverted_polarity)
> +		value &= ~RP1_PWM_CHAN_CTRL_POLARITY;
> +	else
> +		value |= RP1_PWM_CHAN_CTRL_POLARITY;
> +	regmap_write(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), value);

[Severity: High]
Could the PWM controller lose its hardware configuration after a system
suspend and resume?

During rp1_pwm_request(), the channel control register is initialized with
essential bits like RP1_PWM_CHAN_CTRL_DEFAULT, which includes the
MODE_TE_MS and FIFO_POP_MASK configurations.

If the hardware loses power during suspend, the registers reset. On resume,
when a consumer re-applies its state, this regmap_read() will read the wiped
hardware register. The driver will then update only the polarity bit and
write it back, failing to restore the required operational mode bits that
were initialized during the request phase.

> +
> +	/* early exit if disabled */

[ ... ]

> +static struct platform_driver rp1_pwm_driver = {
> +	.probe = rp1_pwm_probe,
> +	/*
> +	 * .remove is not needed because this driver cannot be removed/unbound
> +	 * cause it relies on syscon which cannot be removed
> +	 */
> +	.driver = {
> +		.name = "rp1-pwm",

[Severity: High]
Is it safe to omit the remove callback here?

Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
device is removed, reset, or unbound via sysfs, it will forcibly unbind all
child devices.

Without a remove callback to run cleanup, pwmchip_remove() is never called
and the clock is left enabled. Because the backing memory is devm-managed,
the devres subsystem will eventually free the pwm_chip and regmap memory.
This can leave dangling pointers in the core pwm_chips IDR and the global
syscon_list, which could result in a use-after-free.

> +		.of_match_table = rp1_pwm_of_match,
> +		.pm = pm_ptr(&rp1_pwm_pm_ops),
> +		.suppress_bind_attrs = true,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784539788.git.andrea.porta@suse.com?part=2