Re: [PATCH v2] pwm: imx27: Fix variable truncation in .apply()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Division by zero kernel panic if `clk_get_rate()` returns 0.
- [High] Missing bounds check on `state->period` causes integer overflows and silent hardware misconfiguration.
- [High] The 1.5us safety margin in the ERR051198 workaround mixes unprescaled and prescaled clock units.
--

commit e248ca7d296d611230c3b9bc5da0d73a70d0c358
Author: Ronaldo Nunez <rnunez@baylibre.com>

pwm: imx27: Fix variable truncation in .apply()

Fix a variable truncation when calculating period in microseconds as
part of the solution for the ERR051198 in .apply() callback.

> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 3d34cdc4a3a51..c8b801fcb5251 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
[ ... ]
> @@ -208,6 +208,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int val;
>  	int ret;
>  	u32 cr;
> +	u64 tmp;
>  
>  	clkrate = clk_get_rate(imx->clks[PWM_IMX27_PER].clk);
>  	c = clkrate * state->period;

[Severity: High]
This is a pre-existing issue, but does this calculation handle excessively
large periods correctly?

Userspace can request an arbitrarily large state->period. This can cause a
cascading failure:

1. c = clkrate * state->period can overflow a 64-bit integer.
2. The 64-bit result is assigned to unsigned long period_cycles, which
truncates it on 32-bit ARM architectures.
3. The calculation prescale = period_cycles / 0x10000 + 1 can produce a value
exceeding the hardware's 12-bit limit (4096).
4. MX3_PWMCR_PRESCALER_SET(prescale) uses FIELD_PREP, which masks out higher
bits, writing incorrect configurations to the hardware instead of rejecting
unachievable periods with -EINVAL.

Could this lead to silent hardware misconfiguration?

[ ... ]
> @@ -249,6 +250,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	val = readl(imx->mmio_base + MX3_PWMPR);
>  	val = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
>  	cr = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	/*
> +	 * tmp stores period in nanoseconds. Result fits in u64 since
> +	 * val <= 0xfffe and prescaler in [1, 0x1000].
> +	 */
>  	tmp = NSEC_PER_SEC * (u64)(val + 2) * MX3_PWMCR_PRESCALER_GET(cr);
>  	tmp = DIV_ROUND_UP_ULL(tmp, clkrate);

[Severity: High]
This is a pre-existing issue, but could this lead to a division by zero panic?

If the clock is disabled, unparented, or misconfigured, clk_get_rate() will
return 0.

Since clkrate is directly used as a divisor here, the kernel would panic.
Should the driver validate that clkrate > 0 before proceeding?

>  	period_us = DIV_ROUND_UP_ULL(tmp, 1000);

[Severity: High]
This is a pre-existing issue, but is the 1.5us safety margin mixing
unprescaled and prescaled clock units?

Later in pwm_imx27_apply(), the safety margin is calculated as:

	c = clkrate * 1500;
	do_div(c, NSEC_PER_SEC);

This yields unprescaled clock cycles. However, val is read from MX3_PWMCNR,
which increments in prescaled units:

			if ((val + c >= duty_cycles && val < imx->duty_cycle) ||
			    val + c >= period_cycles)

Adding c to val directly adds unprescaled ticks to prescaled ticks. When the
active prescaler is large (e.g., 4096), does this overestimate the safety
margin by a factor of 4096 (e.g., ~6.1ms instead of 1.5us)?

Could this artificially force the workaround to trigger and unnecessarily
delay valid duty cycle updates by a full PWM period?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522191348.6227-1-rnunez@baylibre.com?part=1