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

[Andrea della Porta <andrea.porta@suse.com>]

Hi,

On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> 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.

The conditional right after the opening bracket of this else branch (see below
for the code) is there just for that: clamping the duty to sane values. So it
already takes care of that.

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

I think I've already answered this here: 
https://lore.kernel.org/all/aiGAINsTG8VZLn28@apocalypse/
so I guess Sashiko is not sotring the entire history as
context, so I'll reply it here:

AFAIK the RP1 is never powered-off, so just shutting down the clock should be ok
and no register are reset to default.

> 
> > +
> > +	/* 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.

This is true. Uwe, I guess I should reintroduce the .remove callback,
what do you think?

Thanks,
Andrea

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