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

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

Hi,

On 17:17 Fri 03 Jul     , sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Dangling syscon pointer and Use-After-Free upon device unbind due to devres regmap cleanup.
> - [High] Non-atomic Read-Modify-Write of shared syscon registers causing data corruption.
> --
> 
> commit ed6030e515052cf4e3fde5de4c8c7ffcb06a014a
> 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.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Co-developed-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> 
> > diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> > new file mode 100644
> > index 0000000000000..8ef5ec9ed3a07
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-rp1.c
> > @@ -0,0 +1,435 @@
> [ ... ]
> > +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: High]
> In rp1_pwm_apply_config(), since this regmap is exposed to other drivers via
> syscon (like the RP1 RPM fan driver), could this manual read-modify-write
> sequence race with concurrent register updates?
> 
> The regmap lock is released between regmap_read() and regmap_write(), which
> might allow another syscon client's changes to RP1_PWM_GLB_CTRL to be
> overwritten with stale data. Does this need to use regmap_update_bits()
> instead?

The registers are guaranteed by design to never overlap: the producer is the fw
which writes in the phase register and the consumer will ionly read that register.
No other registers are shared.

> 
> > +}
> [ ... ]
> > +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]
> Does rp1_pwm_write_waveform() present the same non-atomic read-modify-write
> race condition here when updating RP1_PWM_CHAN_CTRL?

See above.

> 
> > +
> > +	/* early exit if disabled */
> > +	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
> > +	if (!wfhw->enabled) {
> > +		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
> > +		goto exit_disable;
> > +	}
> > +
> > +	/* set period and duty cycle */
> > +	regmap_write(rp1->regmap,
> > +		     RP1_PWM_RANGE(pwm->hwpwm), wfhw->period_ticks);
> > +	regmap_write(rp1->regmap,
> > +		     RP1_PWM_DUTY(pwm->hwpwm), wfhw->duty_ticks);
> > +
> > +	/* enable the channel */
> > +	ctrl |= RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
> > +exit_disable:
> > +	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);
> 
> [Severity: High]
> And similarly here in rp1_pwm_write_waveform() for RP1_PWM_GLB_CTRL?

See above.

> 
> > +
> > +	rp1_pwm_apply_config(chip, pwm);
> > +
> > +	return 0;
> > +}
> [ ... ]
> > +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;
> 
> [Severity: High]
> In rp1_pwm_probe(), because rp1->regmap is managed by devres (allocated via
> devm_regmap_init_mmio), it will be automatically freed when the device is
> unbound. However, of_syscon_register_regmap() registers it globally.
> 
> Since there is no mechanism to unregister the regmap from syscon upon device
> removal, wouldn't unbinding the parent PCI device leave a dangling pointer
> in the global syscon list?
> 
> Could this lead to a use-after-free if another driver attempts to use this
> syscon client after this device is unbound?
>

The device is not hotpluggable and the driver cannot be unloaded/unbounded, so
there should be no dangling pointer around.

Regards,
Andrea
 
> > +
> > +err_remove_chip:
> > +	pwmchip_remove(chip);
> > +err_disable_clk:
> > +	clk_disable_unprepare(rp1->clk);
> > +
> > +	return ret;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783097764.git.andrea.porta@suse.com?part=2