Hello, I applied the patch and reviewed it in my editor. Here is the resulting diff: diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c index b944ecb456d5..4818d0170d53 100644 --- a/drivers/pwm/pwm-ipq.c +++ b/drivers/pwm/pwm-ipq.c @@ -97,9 +97,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; - if (!ipq_chip->clk_rate) - return -EINVAL; - + /* + * XXX Why? A comment please. (Is this already covered by the checks + * below?) + */ if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC, ipq_chip->clk_rate)) return -ERANGE; @@ -107,18 +108,29 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS); duty_ns = min(state->duty_cycle, period_ns); + /* + * Pick the maximal value for PWM_DIV that still allows a + * 100% relative duty cycle. This allows a fine grained + * selection of duty cycles. + */ pwm_div = IPQ_PWM_MAX_DIV - 1; + + /* + * XXX mul_u64_u64_div_u64 returns an u64, this might overflow the + * unsigned int pre_div. + */ pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate, (u64)NSEC_PER_SEC * (pwm_div + 1)); - pre_div = (pre_div > 0) ? pre_div - 1 : 0; + + if (!pre_div) + return -ERANGE; + + pre_div -= 1; if (pre_div > IPQ_PWM_MAX_DIV) pre_div = IPQ_PWM_MAX_DIV; - /* - * high duration = pwm duty * (pwm div + 1) - * pwm duty = duty_ns / period_ns - */ + /* pwm duty = HI_DUR * (PRE_DIV + 1) / clk_rate */ hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate, (u64)(pre_div + 1) * NSEC_PER_SEC); @@ -161,6 +173,10 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1); effective_div = (u64)(pre_div + 1) * (pwm_div + 1); + + /* + * effective_div <= 0x100000000, so the multiplication doesn't overflow. + */ state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC, ipq_chip->clk_rate); @@ -210,6 +226,8 @@ static int ipq_pwm_probe(struct platform_device *pdev) return dev_err_probe(dev, ret, "Failed to lock clock rate\n"); pwm->clk_rate = clk_get_rate(clk); + if (!pwm->clk_rate) + return dev_err_probe(dev, -EINVAL, "Failed due to clock rate being zero\n"); chip->ops = &ipq_pwm_ops; Comments with XXX need more code adaptions (or a comment why my concern isn't justified). Best regards Uwe