[PATCH 0/3] pwm: Fix division by 0 issues

[PATCH 0/3] pwm: Fix division by 0 issues

[Uwe Kleine-König]

Hello,

triggered by Josh Poimboeuf's patch to fix a warning by objtool due to a
division by zero in the pwm-mediatek driver, I reviewed all pwm drivers
for similar issues.

Josh's patch is resent here, the two additional patches are new. objtool
didn't warn about these even though these are quite similar divisions by
zero. I didn't debug why these were not catched by clang/objtool.

I haven't made up my mind yet if I'll send them for inclusion in
6.15 given that they are new and only build-tested so far. Also the
commits that introduced these problems are already old (v4.12-rc1,
v4.4-rc1 and v5.3-rc1 respectively) and there is no indication this
problem triggers on real hardware.

I'll put them in my for-next branch anyhow for wider exposure. I happily
accept test reports to (hopefully) increase my confidence in these
patches.

Best regards
Uwe

Josh Poimboeuf (1):
  pwm: mediatek: Prevent divide-by-zero in pwm_mediatek_config()

Uwe Kleine-König (2):
  pwm: rcar: Improve register calculation
  pwm: fsl-ftm: Handle clk_get_rate() returning 0

 drivers/pwm/pwm-fsl-ftm.c  |  6 ++++++
 drivers/pwm/pwm-mediatek.c |  8 ++++++--
 drivers/pwm/pwm-rcar.c     | 23 ++++++++++++-----------
 3 files changed, 24 insertions(+), 13 deletions(-)


base-commit: 6df320abbb40654085d7258de33d78481e93ac8d
-- 
2.47.2

[PATCH 1/3] pwm: mediatek: Prevent divide-by-zero in pwm_mediatek_config()

[Uwe Kleine-König]

From: Josh Poimboeuf <jpoimboe@kernel.org>

With CONFIG_COMPILE_TEST && !CONFIG_HAVE_CLK, pwm_mediatek_config() has a
divide-by-zero in the following line:

	do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm]));

due to the fact that the !CONFIG_HAVE_CLK version of clk_get_rate()
returns zero.

This is presumably just a theoretical problem: COMPILE_TEST overrides
the dependency on RALINK which would select COMMON_CLK.  Regardless it's
a good idea to check for the error explicitly to avoid divide-by-zero.

Fixes the following warning:

  drivers/pwm/pwm-mediatek.o: warning: objtool: .text: unexpected end of section

Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Link: https://lore.kernel.org/r/fb56444939325cc173e752ba199abd7aeae3bf12.1742852847.git.jpoimboe@kernel.org
[ukleinek: s/CONFIG_CLK/CONFIG_HAVE_CLK/]
Fixes: caf065f8fd58 ("pwm: Add MediaTek PWM support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 01dfa0fab80a..7eaab5831499 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -121,21 +121,25 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
 	u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
 	    reg_thres = PWMTHRES;
+	unsigned long clk_rate;
 	u64 resolution;
 	int ret;
 
 	ret = pwm_mediatek_clk_enable(chip, pwm);
-
 	if (ret < 0)
 		return ret;
 
+	clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
+	if (!clk_rate)
+		return -EINVAL;
+
 	/* Make sure we use the bus clock and not the 26MHz clock */
 	if (pc->soc->has_ck_26m_sel)
 		writel(0, pc->regs + PWM_CK_26M_SEL);
 
 	/* Using resolution in picosecond gets accuracy higher */
 	resolution = (u64)NSEC_PER_SEC * 1000;
-	do_div(resolution, clk_get_rate(pc->clk_pwms[pwm->hwpwm]));
+	do_div(resolution, clk_rate);
 
 	cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution);
 	while (cnt_period > 8191) {
-- 
2.47.2

[PATCH 2/3] pwm: rcar: Improve register calculation

[Uwe Kleine-König]

There were several issues in the function rcar_pwm_set_counter():

 - The u64 values period_ns and duty_ns were cast to int on function
   call which might loose bits on 32 bit architectures.
   Fix: Make parameters to rcar_pwm_set_counter() u64
 - The algorithm divided by the result of a division which looses
   precision.
   Fix: Make use of mul_u64_u64_div_u64()
 - The calculated values were just masked to fit the respective register
   fields which again might loose bits.
   Fix: Explicitly check for overlow

Implement the respective fixes.

Fixes: ed6c1476bf7f ("pwm: Add support for R-Car PWM Timer")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-rcar.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 2261789cc27d..19e5d0b849a6 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -102,23 +102,24 @@ static void rcar_pwm_set_clock_control(struct rcar_pwm_chip *rp,
 	rcar_pwm_write(rp, value, RCAR_PWMCR);
 }
 
-static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, int duty_ns,
-				int period_ns)
+static int rcar_pwm_set_counter(struct rcar_pwm_chip *rp, int div, u64 duty_ns,
+				u64 period_ns)
 {
-	unsigned long long one_cycle, tmp;	/* 0.01 nanoseconds */
+	unsigned long long tmp;
 	unsigned long clk_rate = clk_get_rate(rp->clk);
 	u32 cyc, ph;
 
-	one_cycle = NSEC_PER_SEC * 100ULL << div;
-	do_div(one_cycle, clk_rate);
+	/* div <= 24 == RCAR_PWM_MAX_DIVISION, so the shift doesn't overflow. */
+	tmp = mul_u64_u64_div_u64(period_ns, clk_rate, (u64)NSEC_PER_SEC << div);
+	if (tmp > FIELD_MAX(RCAR_PWMCNT_CYC0_MASK))
+		tmp = FIELD_MAX(RCAR_PWMCNT_CYC0_MASK);
 
-	tmp = period_ns * 100ULL;
-	do_div(tmp, one_cycle);
-	cyc = (tmp << RCAR_PWMCNT_CYC0_SHIFT) & RCAR_PWMCNT_CYC0_MASK;
+	cyc = FIELD_PREP(RCAR_PWMCNT_CYC0_MASK, tmp);
 
-	tmp = duty_ns * 100ULL;
-	do_div(tmp, one_cycle);
-	ph = tmp & RCAR_PWMCNT_PH0_MASK;
+	tmp = mul_u64_u64_div_u64(duty_ns, clk_rate, (u64)NSEC_PER_SEC << div);
+	if (tmp > FIELD_MAX(RCAR_PWMCNT_PH0_MASK))
+		tmp = FIELD_MAX(RCAR_PWMCNT_PH0_MASK);
+	ph = FIELD_PREP(RCAR_PWMCNT_PH0_MASK, tmp);
 
 	/* Avoid prohibited setting */
 	if (cyc == 0 || ph == 0)
-- 
2.47.2

Re: [PATCH 2/3] pwm: rcar: Improve register calculation

[Geert Uytterhoeven]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On Tue, 1 Apr 2025, Uwe Kleine-König wrote:
> There were several issues in the function rcar_pwm_set_counter():
>
> - The u64 values period_ns and duty_ns were cast to int on function
>   call which might loose bits on 32 bit architectures.
>   Fix: Make parameters to rcar_pwm_set_counter() u64
> - The algorithm divided by the result of a division which looses
>   precision.
>   Fix: Make use of mul_u64_u64_div_u64()
> - The calculated values were just masked to fit the respective register
>   fields which again might loose bits.
>   Fix: Explicitly check for overlow
>
> Implement the respective fixes.
>
> Fixes: ed6c1476bf7f ("pwm: Add support for R-Car PWM Timer")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

This is now commit b41a4921d409c909 ("pwm: rcar: Improve register
calculation"), and has the missing include.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

Re: [PATCH 2/3] pwm: rcar: Improve register calculation

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

Hello Geert,

On Fri, Apr 04, 2025 at 08:48:47AM +0200, Geert Uytterhoeven wrote:
> On Tue, 1 Apr 2025, Uwe Kleine-König wrote:
> > There were several issues in the function rcar_pwm_set_counter():
> > 
> > - The u64 values period_ns and duty_ns were cast to int on function
> >   call which might loose bits on 32 bit architectures.
> >   Fix: Make parameters to rcar_pwm_set_counter() u64
> > - The algorithm divided by the result of a division which looses
> >   precision.
> >   Fix: Make use of mul_u64_u64_div_u64()
> > - The calculated values were just masked to fit the respective register
> >   fields which again might loose bits.
> >   Fix: Explicitly check for overlow
> > 
> > Implement the respective fixes.
> > 
> > Fixes: ed6c1476bf7f ("pwm: Add support for R-Car PWM Timer")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> This is now commit b41a4921d409c909 ("pwm: rcar: Improve register
> calculation"), and has the missing include.
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

I added your tag, the commit is now e7327c193014.

Thanks!
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/3] pwm: fsl-ftm: Handle clk_get_rate() returning 0

[Uwe Kleine-König]

Considering that the driver doesn't enable the used clocks (and also
that clk_get_rate() returns 0 if CONFIG_HAVE_CLK is unset) better check
the return value of clk_get_rate() for being non-zero before dividing by
it.

Fixes: 3479bbd1e1f8 ("pwm: fsl-ftm: More relaxed permissions for updating period")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-fsl-ftm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 2510c10ca473..c45a5fca4cbb 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -118,6 +118,9 @@ static unsigned int fsl_pwm_ticks_to_ns(struct fsl_pwm_chip *fpc,
 	unsigned long long exval;
 
 	rate = clk_get_rate(fpc->clk[fpc->period.clk_select]);
+	if (rate >> fpc->period.clk_ps == 0)
+		return 0;
+
 	exval = ticks;
 	exval *= 1000000000UL;
 	do_div(exval, rate >> fpc->period.clk_ps);
@@ -190,6 +193,9 @@ static unsigned int fsl_pwm_calculate_duty(struct fsl_pwm_chip *fpc,
 	unsigned int period = fpc->period.mod_period + 1;
 	unsigned int period_ns = fsl_pwm_ticks_to_ns(fpc, period);
 
+	if (!period_ns)
+		return 0;
+
 	duty = (unsigned long long)duty_ns * period;
 	do_div(duty, period_ns);
 
-- 
2.47.2

Re: [PATCH 0/3] pwm: Fix division by 0 issues

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Hello,

On Tue, Apr 01, 2025 at 12:28:58PM +0200, Uwe Kleine-König wrote:
> I'll put them in my for-next branch anyhow for wider exposure. I happily
> accept test reports to (hopefully) increase my confidence in these
> patches.

FTR: I did that. The 0day build bot already found a problem with it in the
rcar patch that is already fixed.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]