2020-03-23 09:25:16

by Rayagonda Kokatanur

[permalink] [raw]
Subject: [PATCH v2 0/2] Handle return value and remove unnecessary check

This patch series contains following changes,
1. Handle clk_get_rate() return
2. remove unnecessary check of 'duty'

Changes from v1:
-Address code review comments from Uwe Kleine-König,
added more details to commit message

Rayagonda Kokatanur (2):
pwm: bcm-iproc: handle clk_get_rate() return
pwm: bcm-iproc: remove unnecessary check of 'duty'

drivers/pwm/pwm-bcm-iproc.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)

--
2.17.1


2020-03-23 09:25:52

by Rayagonda Kokatanur

[permalink] [raw]
Subject: [PATCH v2 2/2] pwm: bcm-iproc: remove unnecessary check of 'duty'

Variable 'duty' is u32 and IPROC_PWM_DUTY_CYCLE_MIN is zero.
Hence the less-than zero comparison is never true,remove the check.

Fixes: daa5abc41c80 ("pwm: Add support for Broadcom iProc PWM controller")
Signed-off-by: Rayagonda Kokatanur <[email protected]>
---
drivers/pwm/pwm-bcm-iproc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
index 8bbd2a04fead..1bb66721f985 100644
--- a/drivers/pwm/pwm-bcm-iproc.c
+++ b/drivers/pwm/pwm-bcm-iproc.c
@@ -149,8 +149,7 @@ static int iproc_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
value = rate * state->duty_cycle;
duty = div64_u64(value, div);

- if (period < IPROC_PWM_PERIOD_MIN ||
- duty < IPROC_PWM_DUTY_CYCLE_MIN)
+ if (period < IPROC_PWM_PERIOD_MIN)
return -EINVAL;

if (period <= IPROC_PWM_PERIOD_MAX &&
--
2.17.1

2020-03-23 09:25:56

by Rayagonda Kokatanur

[permalink] [raw]
Subject: [PATCH v2 1/2] pwm: bcm-iproc: handle clk_get_rate() return

Handle clk_get_rate() returning <= 0 condition to avoid
possible division by zero.

Fixes: daa5abc41c80 ("pwm: Add support for Broadcom iProc PWM controller")
Signed-off-by: Rayagonda Kokatanur <[email protected]>
---
drivers/pwm/pwm-bcm-iproc.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
index 1f829edd8ee7..8bbd2a04fead 100644
--- a/drivers/pwm/pwm-bcm-iproc.c
+++ b/drivers/pwm/pwm-bcm-iproc.c
@@ -99,19 +99,25 @@ static void iproc_pwmc_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
else
state->polarity = PWM_POLARITY_INVERSED;

- value = readl(ip->base + IPROC_PWM_PRESCALE_OFFSET);
- prescale = value >> IPROC_PWM_PRESCALE_SHIFT(pwm->hwpwm);
- prescale &= IPROC_PWM_PRESCALE_MAX;
-
- multi = NSEC_PER_SEC * (prescale + 1);
-
- value = readl(ip->base + IPROC_PWM_PERIOD_OFFSET(pwm->hwpwm));
- tmp = (value & IPROC_PWM_PERIOD_MAX) * multi;
- state->period = div64_u64(tmp, rate);
-
- value = readl(ip->base + IPROC_PWM_DUTY_CYCLE_OFFSET(pwm->hwpwm));
- tmp = (value & IPROC_PWM_PERIOD_MAX) * multi;
- state->duty_cycle = div64_u64(tmp, rate);
+ if (rate == 0) {
+ state->period = 0;
+ state->duty_cycle = 0;
+ } else {
+ value = readl(ip->base + IPROC_PWM_PRESCALE_OFFSET);
+ prescale = value >> IPROC_PWM_PRESCALE_SHIFT(pwm->hwpwm);
+ prescale &= IPROC_PWM_PRESCALE_MAX;
+
+ multi = NSEC_PER_SEC * (prescale + 1);
+
+ value = readl(ip->base + IPROC_PWM_PERIOD_OFFSET(pwm->hwpwm));
+ tmp = (value & IPROC_PWM_PERIOD_MAX) * multi;
+ state->period = div64_u64(tmp, rate);
+
+ value = readl(ip->base +
+ IPROC_PWM_DUTY_CYCLE_OFFSET(pwm->hwpwm));
+ tmp = (value & IPROC_PWM_PERIOD_MAX) * multi;
+ state->duty_cycle = div64_u64(tmp, rate);
+ }
}

static int iproc_pwmc_apply(struct pwm_chip *chip, struct pwm_device *pwm,
--
2.17.1

2020-03-23 09:36:30

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] pwm: bcm-iproc: handle clk_get_rate() return

On Mon, Mar 23, 2020 at 02:54:23PM +0530, Rayagonda Kokatanur wrote:
> Handle clk_get_rate() returning <= 0 condition to avoid
> possible division by zero.

The idea I wanted to transport with my question about how this problem
was found is that the commit log is amended with this information. This
is important information as it helps people having to decide if this
change should be backported. Also it would be great to know if this can
really make the kernel crash or if (e.g.) said clock cannot be off in
practise.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |