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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Registering a devm-managed regmap with the syscon subsystem leads to a Use-After-Free.
- [High] The `wfhw->inverted_polarity` field is left uninitialized when disabling the PWM channel, leading to random inactive pin states.
- [Medium] Underflow in duty cycle calculation when hardware DUTY register exceeds RANGE + 1.
- [Medium] Non-atomic read-modify-write sequences on shared global registers race with syscon consumers.
- [Medium] Hardware configuration registers are not restored during system resume.
- [Low] 64-bit modulo and division operations cause link errors on 32-bit platforms.
--

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

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

[ ... ]

> +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u32 value;
> +
> +	/* update the changed registers on the next strobe to avoid glitches */
> +	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
> +	value |= RP1_PWM_GLB_CTRL_SET_UPDATE;
> +	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, value);
> +}

[Severity: Medium]
Will this read-modify-write sequence race with syscon consumers?

Since this driver exposes its regmap via syscon to allow other drivers to
access it concurrently, a separate regmap_read() and regmap_write()
releases the internal lock in between.

If a syscon consumer modifies RP1_PWM_GLB_CTRL simultaneously, will its
changes be silently overwritten? Should this use regmap_update_bits()
instead?

[ ... ]

> +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +				       struct pwm_device *pwm,
> +				       const struct pwm_waveform *wf,
> +				       void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u64 period_ticks, duty_ticks, offset_ticks;
> +	struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u64 clk_rate = rp1->clk_rate;
> +	int ret = 0;
> +
> +	if (!wf->period_length_ns) {
> +		wfhw->enabled = false;
> +		return 0;
> +	}

[Severity: High]
Does this early return leave wfhw->inverted_polarity uninitialized?

If the PWM core passes uninitialized memory for _wfhw, returning here
without setting inverted_polarity means rp1_pwm_write_waveform() will later
read uninitialized memory to configure the RP1_PWM_CHAN_CTRL_POLARITY bit.

Because the hardware drives the pin to 0 or 1 based on polarity when disabled,
could this cause the disabled state to unpredictably flip between high and low?

[ ... ]

> +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
> +	duty_ticks = min(duty_ticks, period_ticks);
> +	offset_ticks = mul_u64_u64_div_u64(wf->duty_offset_ns, clk_rate, NSEC_PER_SEC);
> +	if (offset_ticks >= period_ticks)
> +		offset_ticks %= period_ticks;

[Severity: Low]
Will this 64-bit modulo operation cause link errors on 32-bit builds?

Since both offset_ticks and period_ticks are u64, using the modulo
operator directly will emit a call to __umoddi3. Since Kconfig allows
COMPILE_TEST, will this break the build on 32-bit architectures?

[ ... ]

> +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,
> +						      clk_rate);

[Severity: Low]
Is it safe to pass clk_rate as a u64 divisor to DIV_ROUND_UP_ULL()?

DIV_ROUND_UP_ULL uses do_div() internally which expects a 32-bit divisor.
Will this cause division helpers to be missing during 32-bit builds?

> +	} else {
> +		ticks = (u64)wfhw->period_ticks + 1 - wfhw->duty_ticks;

[Severity: Medium]
Can this calculation underflow if the hardware DUTY register exceeds RANGE + 1?

rp1_pwm_read_waveform() fetches period_ticks and duty_ticks directly from
the hardware registers without clamping. If the bootloader configures a
duty cycle greater than the period (to force a continuous 100% output),
would this undergo an unsigned underflow?

Could this result in an astronomically large u64 value for ticks, corrupting
the reported duty_length_ns?

> +		wf->duty_length_ns = DIV_ROUND_UP_ULL(ticks * NSEC_PER_SEC, clk_rate);
> +		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
> +	}
> +
> +	return 0;
> +}
> +
> +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: Medium]
Is this another non-atomic read-modify-write susceptible to data races?

Similar to rp1_pwm_apply_config(), will syscon consumer modifications to
RP1_PWM_CHAN_CTRL be lost if they occur between the read and write?

[ ... ]

> +static int rp1_pwm_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config);
> +	if (IS_ERR(rp1->regmap))
> +		return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n");
[ ... ]
> +	ret = of_syscon_register_regmap(np, rp1->regmap);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to register syscon\n");
> +		goto err_remove_chip;
> +	}
> +
> +	return 0;
> +
> +err_remove_chip:
> +	pwmchip_remove(chip);
> +err_disable_clk:
> +	clk_disable_unprepare(rp1->clk);
> +
> +	return ret;
> +}
> +
> +static void rp1_pwm_remove(struct platform_device *pdev)
> +{
> +	struct pwm_chip *chip = platform_get_drvdata(pdev);
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +
> +	pwmchip_remove(chip);
> +
> +	if (rp1->clk_enabled) {
> +		clk_disable_unprepare(rp1->clk);
> +		rp1->clk_enabled = false;
> +	}
> +}

[Severity: High]
Will registering a devm-managed regmap with the syscon subsystem lead to a
use-after-free on device unbind?

The syscon subsystem stores this regmap pointer in a global, permanent list
and provides no API to unregister it. If the RP1 device is unbound, devres
will automatically free the regmap memory.

Would any subsequent syscon lookup by consumers return this freed memory,
triggering a use-after-free?

[ ... ]

> +static int rp1_pwm_resume(struct device *dev)
> +{
> +	struct pwm_chip *chip = dev_get_drvdata(dev);
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	int ret;
> +
> +	ret = clk_prepare_enable(rp1->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock on resume: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	rp1->clk_enabled = true;
> +
> +	return 0;
> +}

[Severity: Medium]
Does the resume handler need to restore the hardware register state?

The PM operations only toggle the clock, but do not save and restore the
PWM configuration registers (such as RP1_PWM_GLB_CTRL, CHAN_CTRL, RANGE,
DUTY), and the regmap lacks a cache configuration to do this automatically.

If the hardware loses its state during S3 suspend, will the controller
wake up in its default reset state (stopped), breaking consumer devices
that expect the PWM signal to be maintained?

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