Re: [PATCH v3] pwm: atmel: Fix arithmetic overflows for long periods

[David Laight <david.laight.linux@gmail.com>]

On Tue, 27 Oct 2026 03:27:55 +0100
Karl Mehltretter <kmehltretter@gmail.com> wrote:

> atmel_pwm_calculate_cprd_and_pres() first multiplies the requested
> period by the clock rate in a u64 and only then divides by
> NSEC_PER_SEC. The product can overflow even when the quotient and the
> resulting hardware configuration are representable.
> 
> For example, at 12 MHz a 1600 s period has 19.2 billion clock cycles
> and fits with prescaler 3, but the intermediate product exceeds U64_MAX.
> It wraps and the driver programs a period of about 62.8 s instead. This
> remains broken after changing fls() to fls64(). The duty-cycle
> calculation has the same overflow.
> 
> Use mul_u64_u64_div_u64() for both conversions and use fls64() when
> deriving the prescaler. The multiply/divide result cannot overflow if
> the clock rate is at most NSEC_PER_SEC, so hold the clock rate exclusive
> and validate it at probe time.
> 
> The reverse conversion in atmel_pwm_get_state() can overflow too: it
> multiplies CPRD or CDTY by NSEC_PER_SEC and then shifts the u64 by the
> prescaler. Convert the scaled clock ticks with the overflow-safe,
> round-up helper instead. Explicitly saturate values that cannot be
> represented by the u64 nanosecond API, rather than relying on the
> architecture-specific helper's quotient-overflow behaviour.

Personally I think mul_u64_u64_div_u64() should return ~0ull on both
overflow and divide by zero.
While a valid result, any code that expects it can pretty certainly
also get 'a bit more' so including ~0ull in the 'overflow' values
doesn't really do any harm.
But someone insisted on in faulting - even though there has been
a patch to x86-64 to recover from the fault.

> 
> Fixes: a9d887dc1c60 ("pwm: Convert period and duty cycle to u64")
> Cc: stable@vger.kernel.org
> Reported-by: Uwe Kleine-König <ukleinek@kernel.org>
> Link: https://lore.kernel.org/r/atnHHaOwTkI78_tc@monoceros
> Suggested-by: Uwe Kleine-König <ukleinek@kernel.org>
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> Changes in v3:
> - Use mul_u64_u64_div_u64() for the period conversion as suggested by
>   Uwe, and for the analogous duty-cycle conversion.
> - Fix the corresponding overflow in the get_state() conversion and
>   explicitly saturate results that cannot be represented in nanoseconds.
> - Hold the clock rate exclusive and validate the rate bound required by
>   the arithmetic.
> - Rebase onto v7.3-rc7.
> 
> Testing:
> - Built an ARM zImage with clang 22.1.8 and W=1.
> - Passed 14 host boundary tests, 53 oracle-checked SAM9X75 QEMU boots,
>   and all 20 PWM device-model qtests.
> - Passed 89/89 checks on a physical SAM9X75, including overflow cases.
>   A 69.175-second period measured 69.210 seconds between interrupts; no
>   kernel errors were found.
> 
>  drivers/pwm/pwm-atmel.c | 46 +++++++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 86918523d8..db7a14c66a 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -23,6 +23,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -183,19 +184,18 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
>  					     unsigned long *cprd, u32 *pres)
>  {
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> -	unsigned long long cycles = state->period;
> +	unsigned long long cycles;
>  	int shift;
>  
>  	/* Calculate the period cycles and prescale value */
> -	cycles *= clkrate;
> -	do_div(cycles, NSEC_PER_SEC);
> +	cycles = mul_u64_u64_div_u64(state->period, clkrate, NSEC_PER_SEC);
>  
>  	/*
>  	 * The register for the period length is cfg.period_bits bits wide.
>  	 * So for each bit the number of clock cycles is wider divide the input
>  	 * clock frequency by two using pres and shift cprd accordingly.
>  	 */
> -	shift = fls(cycles) - atmel_pwm->data->cfg.period_bits;
> +	shift = fls64(cycles) - atmel_pwm->data->cfg.period_bits;
>  
>  	if (shift > PWM_MAX_PRES) {
>  		dev_err(pwmchip_parent(chip), "pres exceeds the maximum value\n");
> @@ -218,12 +218,23 @@ static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
>  {
>  	unsigned long long cycles = state->duty_cycle;
>  
> -	cycles *= clkrate;
> -	do_div(cycles, NSEC_PER_SEC);
> +	cycles = mul_u64_u64_div_u64(cycles, clkrate, NSEC_PER_SEC);
>  	cycles >>= pres;
> +
>  	*cdty = cprd - cycles;
>  }
>  
> +static u64 atmel_pwm_ticks_to_ns(u64 ticks, unsigned long rate)
> +{
> +	u64 max_ticks;
> +
> +	max_ticks = mul_u64_u64_div_u64(U64_MAX, rate, NSEC_PER_SEC);
> +	if (ticks > max_ticks)
> +		return U64_MAX;

I'm not sure how often this is called, but it might be worth caching
max_ticks during the probe.
(and maybe even the clock rate itself - I doubt it can change.)

David

> +
> +	return mul_u64_u64_div_u64_roundup(ticks, NSEC_PER_SEC, rate);
> +}
> +
>  static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm,
>  				  unsigned long cdty)
>  {
> @@ -352,24 +363,22 @@ static int atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (sr & (1 << pwm->hwpwm)) {
>  		unsigned long rate = clk_get_rate(atmel_pwm->clk);
>  		u32 cdty, cprd, pres;
> -		u64 tmp;
> +		u64 ticks;
>  
>  		pres = cmr & PWM_CMR_CPRE_MSK;
>  
>  		cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
>  					  atmel_pwm->data->regs.period);
> -		tmp = (u64)cprd * NSEC_PER_SEC;
> -		tmp <<= pres;
> -		state->period = DIV64_U64_ROUND_UP(tmp, rate);
> +		ticks = (u64)cprd << pres;
> +		state->period = atmel_pwm_ticks_to_ns(ticks, rate);
>  
>  		/* Wait for an updated duty_cycle queued in hardware */
>  		atmel_pwm_wait_nonpending(atmel_pwm, pwm->hwpwm);
>  
>  		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
>  					  atmel_pwm->data->regs.duty);
> -		tmp = (u64)(cprd - cdty) * NSEC_PER_SEC;
> -		tmp <<= pres;
> -		state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate);
> +		ticks = (u64)(cprd - cdty) << pres;
> +		state->duty_cycle = atmel_pwm_ticks_to_ns(ticks, rate);
>  
>  		state->enabled = true;
>  	} else {
> @@ -487,6 +496,7 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  {
>  	struct atmel_pwm_chip *atmel_pwm;
>  	struct pwm_chip *chip;
> +	unsigned long rate;
>  	int ret;
>  
>  	chip = devm_pwmchip_alloc(&pdev->dev, 4, sizeof(*atmel_pwm));
> @@ -507,6 +517,16 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(atmel_pwm->clk),
>  				     "failed to get prepared PWM clock\n");
>  
> +	ret = devm_clk_rate_exclusive_get(&pdev->dev, atmel_pwm->clk);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "failed to get exclusive PWM clock rate\n");
> +
> +	rate = clk_get_rate(atmel_pwm->clk);
> +	if (!rate || rate > NSEC_PER_SEC)
> +		return dev_err_probe(&pdev->dev, -EINVAL,
> +				     "invalid PWM clock rate %lu Hz\n", rate);
> +
>  	chip->ops = &atmel_pwm_ops;
>  
>  	ret = atmel_pwm_enable_clk_if_on(chip, true);
> 
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907