These are three improvements / fixes for the arm_global_timer driver.
Changes from v2 at [1]:
- add patches 1 and 2 (which is why this is now a series instead of a
single patch) as in the code review process more issues have been
spotted that need fixing
- remove "psv < 0" check from patch 3 (as this check is now impossible
as patch 2 makes the variable in question an unsigned long with a
check for zero and returning before decrementing psv).
Changes from v1 at [0]:
- use FIELD_FIT() to check whether psv overflows the register
- update the description accordingly
[0] https://lore.kernel.org/lkml/[email protected]/
[1] https://lore.kernel.org/lkml/[email protected]/
Martin Blumenstingl (3):
clocksource/drivers/arm_global_timer: Make gt_target_rate unsigned
long
clocksource/drivers/arm_global_timer: Guard against division by zero
clocksource/drivers/arm_global_timer: Simplify prescaler register
access
drivers/clocksource/arm_global_timer.c | 31 ++++++++++++--------------
1 file changed, 14 insertions(+), 17 deletions(-)
--
2.44.0
Change the data type of gt_target_rate to unsigned long as this is what
we get back from clk_get_rate().
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/clocksource/arm_global_timer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 8dd1e46b7176..fb3ffd54c822 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -52,7 +52,8 @@
*/
static void __iomem *gt_base;
static struct notifier_block gt_clk_rate_change_nb;
-static u32 gt_psv_new, gt_psv_bck, gt_target_rate;
+static u32 gt_psv_new, gt_psv_bck;
+static unsigned long gt_target_rate;
static int gt_ppi;
static struct clock_event_device __percpu *gt_evt;
--
2.44.0
The result of the division of new_rate by gt_target_rate can be zero (if
new_rate is smaller than gt_target_rate). Using that result as divisor
without checking can result in a division by zero error. Guard against
this by checking for a zero value earlier.
While here, also change the psv variable to an unsigned long to make
sure we don't overflow the datatype as all other types involved are also
unsiged long.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/clocksource/arm_global_timer.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index fb3ffd54c822..257599d682f0 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -291,18 +291,17 @@ static int gt_clk_rate_change_cb(struct notifier_block *nb,
switch (event) {
case PRE_RATE_CHANGE:
{
- int psv;
+ unsigned long psv;
- psv = DIV_ROUND_CLOSEST(ndata->new_rate,
- gt_target_rate);
-
- if (abs(gt_target_rate - (ndata->new_rate / psv)) > MAX_F_ERR)
+ psv = DIV_ROUND_CLOSEST(ndata->new_rate, gt_target_rate);
+ if (!psv ||
+ abs(gt_target_rate - (ndata->new_rate / psv)) > MAX_F_ERR)
return NOTIFY_BAD;
psv--;
/* prescaler within legal range? */
- if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
+ if (psv > GT_CONTROL_PRESCALER_MAX)
return NOTIFY_BAD;
/*
--
2.44.0
Use GENMASK() to define the prescaler mask and make the whole driver use
the mask (together with helpers such as FIELD_{GET,PREP,FIT}) instead of
needing an additional shift and max value constant.
Signed-off-by: Martin Blumenstingl <[email protected]>
---
drivers/clocksource/arm_global_timer.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 257599d682f0..8a82b60b467b 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -9,6 +9,7 @@
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/bitfield.h>
#include <linux/clocksource.h>
#include <linux/clockchips.h>
#include <linux/cpu.h>
@@ -31,10 +32,7 @@
#define GT_CONTROL_COMP_ENABLE BIT(1) /* banked */
#define GT_CONTROL_IRQ_ENABLE BIT(2) /* banked */
#define GT_CONTROL_AUTO_INC BIT(3) /* banked */
-#define GT_CONTROL_PRESCALER_SHIFT 8
-#define GT_CONTROL_PRESCALER_MAX 0xFF
-#define GT_CONTROL_PRESCALER_MASK (GT_CONTROL_PRESCALER_MAX << \
- GT_CONTROL_PRESCALER_SHIFT)
+#define GT_CONTROL_PRESCALER_MASK GENMASK(15, 8)
#define GT_INT_STATUS 0x0c
#define GT_INT_STATUS_EVENT_FLAG BIT(0)
@@ -248,7 +246,7 @@ static void gt_write_presc(u32 psv)
reg = readl(gt_base + GT_CONTROL);
reg &= ~GT_CONTROL_PRESCALER_MASK;
- reg |= psv << GT_CONTROL_PRESCALER_SHIFT;
+ reg |= FIELD_PREP(GT_CONTROL_PRESCALER_MASK, psv);
writel(reg, gt_base + GT_CONTROL);
}
@@ -257,8 +255,7 @@ static u32 gt_read_presc(void)
u32 reg;
reg = readl(gt_base + GT_CONTROL);
- reg &= GT_CONTROL_PRESCALER_MASK;
- return reg >> GT_CONTROL_PRESCALER_SHIFT;
+ return FIELD_GET(GT_CONTROL_PRESCALER_MASK, reg);
}
static void __init gt_delay_timer_init(void)
@@ -273,9 +270,9 @@ static int __init gt_clocksource_init(void)
writel(0, gt_base + GT_COUNTER0);
writel(0, gt_base + GT_COUNTER1);
/* set prescaler and enable timer on all the cores */
- writel(((CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) <<
- GT_CONTROL_PRESCALER_SHIFT)
- | GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+ writel(FIELD_PREP(GT_CONTROL_PRESCALER_MASK,
+ CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) |
+ GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
#ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
sched_clock_register(gt_sched_clock_read, 64, gt_target_rate);
@@ -301,7 +298,7 @@ static int gt_clk_rate_change_cb(struct notifier_block *nb,
psv--;
/* prescaler within legal range? */
- if (psv > GT_CONTROL_PRESCALER_MAX)
+ if (!FIELD_FIT(GT_CONTROL_PRESCALER_MASK, psv))
return NOTIFY_BAD;
/*
--
2.44.0
On 25/02/2024 16:13, Martin Blumenstingl wrote:
> These are three improvements / fixes for the arm_global_timer driver.
>
> Changes from v2 at [1]:
> - add patches 1 and 2 (which is why this is now a series instead of a
> single patch) as in the code review process more issues have been
> spotted that need fixing
> - remove "psv < 0" check from patch 3 (as this check is now impossible
> as patch 2 makes the variable in question an unsigned long with a
> check for zero and returning before decrementing psv).
>
> Changes from v1 at [0]:
> - use FIELD_FIT() to check whether psv overflows the register
> - update the description accordingly
Applied, thanks for the improvements
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
The following commit has been merged into the timers/core branch of tip:
Commit-ID: e651f2fae33634175fae956d896277cf916f5d09
Gitweb: https://git.kernel.org/tip/e651f2fae33634175fae956d896277cf916f5d09
Author: Martin Blumenstingl <[email protected]>
AuthorDate: Sun, 25 Feb 2024 16:13:35 +01:00
Committer: Daniel Lezcano <[email protected]>
CommitterDate: Mon, 26 Feb 2024 10:07:25 +01:00
clocksource/drivers/arm_global_timer: Guard against division by zero
The result of the division of new_rate by gt_target_rate can be zero (if
new_rate is smaller than gt_target_rate). Using that result as divisor
without checking can result in a division by zero error. Guard against
this by checking for a zero value earlier.
While here, also change the psv variable to an unsigned long to make
sure we don't overflow the datatype as all other types involved are also
unsiged long.
Signed-off-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/clocksource/arm_global_timer.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index fd39cfa..4726a15 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -291,18 +291,17 @@ static int gt_clk_rate_change_cb(struct notifier_block *nb,
switch (event) {
case PRE_RATE_CHANGE:
{
- int psv;
+ unsigned long psv;
- psv = DIV_ROUND_CLOSEST(ndata->new_rate,
- gt_target_rate);
-
- if (abs(gt_target_rate - (ndata->new_rate / psv)) > MAX_F_ERR)
+ psv = DIV_ROUND_CLOSEST(ndata->new_rate, gt_target_rate);
+ if (!psv ||
+ abs(gt_target_rate - (ndata->new_rate / psv)) > MAX_F_ERR)
return NOTIFY_BAD;
psv--;
/* prescaler within legal range? */
- if (psv < 0 || psv > GT_CONTROL_PRESCALER_MAX)
+ if (psv > GT_CONTROL_PRESCALER_MAX)
return NOTIFY_BAD;
/*
The following commit has been merged into the timers/core branch of tip:
Commit-ID: 755350bcfb4ac8cbbb62bd7ee6be8271d4b2a88a
Gitweb: https://git.kernel.org/tip/755350bcfb4ac8cbbb62bd7ee6be8271d4b2a88a
Author: Martin Blumenstingl <[email protected]>
AuthorDate: Sun, 25 Feb 2024 16:13:36 +01:00
Committer: Daniel Lezcano <[email protected]>
CommitterDate: Mon, 26 Feb 2024 10:07:25 +01:00
clocksource/drivers/arm_global_timer: Simplify prescaler register access
Use GENMASK() to define the prescaler mask and make the whole driver use
the mask (together with helpers such as FIELD_{GET,PREP,FIT}) instead of
needing an additional shift and max value constant.
Signed-off-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/clocksource/arm_global_timer.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index 4726a15..ab1c8c2 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -9,6 +9,7 @@
#include <linux/init.h>
#include <linux/interrupt.h>
+#include <linux/bitfield.h>
#include <linux/clocksource.h>
#include <linux/clockchips.h>
#include <linux/cpu.h>
@@ -31,10 +32,7 @@
#define GT_CONTROL_COMP_ENABLE BIT(1) /* banked */
#define GT_CONTROL_IRQ_ENABLE BIT(2) /* banked */
#define GT_CONTROL_AUTO_INC BIT(3) /* banked */
-#define GT_CONTROL_PRESCALER_SHIFT 8
-#define GT_CONTROL_PRESCALER_MAX 0xFF
-#define GT_CONTROL_PRESCALER_MASK (GT_CONTROL_PRESCALER_MAX << \
- GT_CONTROL_PRESCALER_SHIFT)
+#define GT_CONTROL_PRESCALER_MASK GENMASK(15, 8)
#define GT_INT_STATUS 0x0c
#define GT_INT_STATUS_EVENT_FLAG BIT(0)
@@ -248,7 +246,7 @@ static void gt_write_presc(u32 psv)
reg = readl(gt_base + GT_CONTROL);
reg &= ~GT_CONTROL_PRESCALER_MASK;
- reg |= psv << GT_CONTROL_PRESCALER_SHIFT;
+ reg |= FIELD_PREP(GT_CONTROL_PRESCALER_MASK, psv);
writel(reg, gt_base + GT_CONTROL);
}
@@ -257,8 +255,7 @@ static u32 gt_read_presc(void)
u32 reg;
reg = readl(gt_base + GT_CONTROL);
- reg &= GT_CONTROL_PRESCALER_MASK;
- return reg >> GT_CONTROL_PRESCALER_SHIFT;
+ return FIELD_GET(GT_CONTROL_PRESCALER_MASK, reg);
}
static void __init gt_delay_timer_init(void)
@@ -273,9 +270,9 @@ static int __init gt_clocksource_init(void)
writel(0, gt_base + GT_COUNTER0);
writel(0, gt_base + GT_COUNTER1);
/* set prescaler and enable timer on all the cores */
- writel(((CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) <<
- GT_CONTROL_PRESCALER_SHIFT)
- | GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+ writel(FIELD_PREP(GT_CONTROL_PRESCALER_MASK,
+ CONFIG_ARM_GT_INITIAL_PRESCALER_VAL - 1) |
+ GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
#ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
sched_clock_register(gt_sched_clock_read, 64, gt_target_rate);
@@ -301,7 +298,7 @@ static int gt_clk_rate_change_cb(struct notifier_block *nb,
psv--;
/* prescaler within legal range? */
- if (psv > GT_CONTROL_PRESCALER_MAX)
+ if (!FIELD_FIT(GT_CONTROL_PRESCALER_MASK, psv))
return NOTIFY_BAD;
/*
The following commit has been merged into the timers/core branch of tip:
Commit-ID: f31c204850f9d93906b5ac8c203b2066524ff245
Gitweb: https://git.kernel.org/tip/f31c204850f9d93906b5ac8c203b2066524ff245
Author: Martin Blumenstingl <[email protected]>
AuthorDate: Sun, 25 Feb 2024 16:13:34 +01:00
Committer: Daniel Lezcano <[email protected]>
CommitterDate: Mon, 26 Feb 2024 10:07:25 +01:00
clocksource/drivers/arm_global_timer: Make gt_target_rate unsigned long
Change the data type of gt_target_rate to unsigned long as this is what
we get back from clk_get_rate().
Signed-off-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Daniel Lezcano <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/clocksource/arm_global_timer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index d749dee..fd39cfa 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -52,7 +52,8 @@
*/
static void __iomem *gt_base;
static struct notifier_block gt_clk_rate_change_nb;
-static u32 gt_psv_new, gt_psv_bck, gt_target_rate;
+static u32 gt_psv_new, gt_psv_bck;
+static unsigned long gt_target_rate;
static int gt_ppi;
static struct clock_event_device __percpu *gt_evt;