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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Exported function rp1_pwm_read_tachometer accesses devm-allocated driver data and hardware registers without synchronization or power management, risking Use-After-Free and bus faults.
- [High] Calling DIV_ROUND_UP_ULL with a 64-bit divisor causes compiler warnings and link errors on 32-bit platforms.
- [High] Returning a positive value (1) from round_waveform_tohw violates kernel error handling conventions and breaks sysfs writes.
- [Medium] The driver is configured as a tristate module but uses builtin_platform_driver(), preventing module unloading.
- [Medium] Failure to check regmap_read return values can lead to the use of uninitialized stack variables.
- [Medium] Implicit zero-initialization in struct assignment destroys previously read hardware state.
- [Medium] Channel is enabled before the global update flag is toggled, risking mis-sampling of period and duty registers.
- [Low] Missing #include <linux/bitfield.h> for the FIELD_PREP macro.
--

commit ea42a2b908766bfc4699d8f5c75d07f5d52b24b8
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. This patch adds the supporting driver framework.

> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> index 0000000000000..5e85d4204ef4a
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
> @@ -0,0 +1,452 @@

[ ... ]

> +#define RP1_PWM_CHAN_CTRL(chan)			(0x014 + ((chan) * 0x10))
> +#define RP1_PWM_CHAN_CTRL_POLARITY		BIT(3)
> +#define RP1_PWM_CHAN_CTRL_FIFO_POP_MASK		BIT(8)
> +#define RP1_PWM_CHAN_CTRL_MODE			GENMASK(2, 0)
> +enum rp1_pwm_ctrl_mode {
> +	RP1_PWM_CHAN_CTRL_MODE_ZERO,
> +	RP1_PWM_CHAN_CTRL_MODE_TE_MS,
> +	RP1_PWM_CHAN_CTRL_MODE_PC_MS,
> +	RP1_PWM_CHAN_CTRL_MODE_PD_ENC,
> +	RP1_PWM_CHAN_CTRL_MODE_MSB_SER,
> +	RP1_PWM_CHAN_CTRL_MODE_PPM,
> +	RP1_PWM_CHAN_CTRL_MODE_LE_MS,
> +	RP1_PWM_CHAN_CTRL_MODE_LSB_SER,
> +};
> +
> +#define RP1_PWM_CHAN_CTRL_DEFAULT		(RP1_PWM_CHAN_CTRL_FIFO_POP_MASK +  \
> +						FIELD_PREP(RP1_PWM_CHAN_CTRL_MODE, \
> +						RP1_PWM_CHAN_CTRL_MODE_TE_MS))

[Severity: Low]
Does this rely on an implicit inclusion for the FIELD_PREP macro?

It appears linux/bitfield.h is missing, which could lead to build failures if
headers are refactored elsewhere in the kernel.

[ ... ]

> +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]
Can regmap_read() fail here and leave value uninitialized? 

If the underlying MMIO read were to fail, the uninitialized stack variable
would be modified and written directly back into the hardware register. This
pattern also occurs in rp1_pwm_write_waveform(), rp1_pwm_read_waveform(),
and rp1_pwm_read_tachometer().

[ ... ]

> +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 (period_ticks > U32_MAX) {
> +		period_ticks = U32_MAX;
> +	} else if (period_ticks < 2) {
> +		period_ticks = 2;
> +		ret = 1;
> +	}

[Severity: High]
Does returning a positive value (1) here violate kernel error handling
conventions and break sysfs write semantics? 

Returning a positive value instead of 0 or a negative error code might cause
VFS write() implementations to treat the result as a partially completed write,
forcing userspace into infinite retry loops when a small period is requested.

[ ... ]

> +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 {
> +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> +			/* 100% duty cycle case */
> +			ticks = 0;
> +		} else {
> +			ticks = (u64)wfhw->period_ticks + 1 - wfhw->duty_ticks;
> +		}
> +		wf->duty_length_ns = DIV_ROUND_UP_ULL(ticks * NSEC_PER_SEC, clk_rate);
> +		wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1 - ticks) *
> +						      NSEC_PER_SEC, clk_rate);
> +	}

[Severity: High]
Can this cause build failures on 32-bit architectures?

DIV_ROUND_UP_ULL delegates to do_div, which expects a 32-bit divisor. Passing
the uncast 64-bit clk_rate directly for period_length_ns, duty_length_ns, and
duty_offset_ns might trigger a __bad_size() link error when compiled on 32-bit
platforms allowed by COMPILE_TEST.

[ ... ]

> +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 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);
> +	regmap_write(rp1->regmap, RP1_PWM_GLB_CTRL, ctrl);
> +
> +	rp1_pwm_apply_config(chip, pwm);

[Severity: Medium]
Does enabling the channel before toggling the global update flag expose the
hardware to a mis-sampling window?

Asserting CHANNEL_ENABLE and writing it to hardware before SET_UPDATE is
applied via rp1_pwm_apply_config() violates hardware synchronization rules,
potentially causing the PWM clock domain to act on mis-sampled settings.

[ ... ]

> +static int rp1_pwm_read_waveform(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u32 value;
> +
> +	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &value);
> +	wfhw->enabled = !!(value & RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm));
> +
> +	regmap_read(rp1->regmap, RP1_PWM_CHAN_CTRL(pwm->hwpwm), &value);
> +	wfhw->inverted_polarity = !!(value & RP1_PWM_CHAN_CTRL_POLARITY);
> +
> +	if (wfhw->enabled) {
> +		regmap_read(rp1->regmap, RP1_PWM_RANGE(pwm->hwpwm), &wfhw->period_ticks);
> +		regmap_read(rp1->regmap, RP1_PWM_DUTY(pwm->hwpwm), &wfhw->duty_ticks);
> +	} else {
> +		*wfhw = (typeof(*wfhw)) {
> +			.period_ticks = 0,
> +			.duty_ticks = 0,
> +		};
> +	}

[Severity: Medium]
Does this struct assignment implicitly clear the inverted_polarity and enabled
fields?

These fields were populated by regmap_read() calls earlier in the function,
and zeroing them out via implicit zero-initialization here seems to discard
the synchronized hardware state when reading a disabled channel.

[ ... ]

> +int rp1_pwm_read_tachometer(struct device *dev)
> +{
> +	struct pwm_chip *chip;
> +	struct rp1_pwm *rp1;
> +	u32 tach_val;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	chip = dev_get_drvdata(dev);
> +	if (!chip)
> +		return -ENODEV;
> +
> +	rp1 = pwmchip_get_drvdata(chip);
> +	if (!rp1)
> +		return -ENODEV;
> +
> +	regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> +
> +	return (int)tach_val;
> +}
> +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");

[Severity: High]
Can calling this exported function concurrently with module unbind result in a
use-after-free?

Retrieving driver data here without synchronization might allow a consumer
driver to race with pwmchip_remove() and the subsequent freeing of the chip
and rp1 structures.

Additionally, since the device clock is disabled during rp1_pwm_suspend(),
would invoking this MMIO read during system suspend trigger a synchronous
external abort?

[ ... ]

> +static struct platform_driver rp1_pwm_driver = {
> +	.probe = rp1_pwm_probe,
> +	.remove = rp1_pwm_remove,
> +	.driver = {
> +		.name = "rp1-pwm",
> +		.of_match_table = rp1_pwm_of_match,
> +		.pm = pm_ptr(&rp1_pwm_pm_ops),
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +builtin_platform_driver(rp1_pwm_driver);

[Severity: Medium]
Is there a reason builtin_platform_driver() is used instead of
module_platform_driver() for a driver configured as a tristate module?

Using builtin_platform_driver() suppresses the generation of a module_exit
function, which permanently blocks unloading the module despite the driver
exporting an rp1_pwm_remove() function.

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