* [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
@ 2018-09-04 18:31 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-09-04 18:31 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner, linux-renesas-soc
Cc: Vladimir Barinov, Andrey Gusakov, Mikhail Ulyanov, linux-sh
When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
/proc/timer_list. It turned out that when calculating it, the driver did
1 << 32 (causing what I think was undefined behavior) resulting in a zero
delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
to be that the driver abused *unsigned long* for the CMT register values
(which are 16/32-bit), so that the calculation of 'ch->max_match_value'
in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
instead fixed 'max_delta_ns' and even fixed the switching an active
clocksource to CMT (which caused the system to turn non-interactive
before).
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
This patch is against the 'tip.git' repo's 'timers/core' branch.
The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
enabling building of this driver now, so not sure how urgent is this...
Changes in version 2:
- completely redid the patch, fixing abuse of *unsigned long* for the CMT
register values.
drivers/clocksource/sh_cmt.c | 56 +++++++++++++++++++------------------------
1 file changed, 26 insertions(+), 30 deletions(-)
Index: tip/drivers/clocksource/sh_cmt.c
=================================--- tip.orig/drivers/clocksource/sh_cmt.c
+++ tip/drivers/clocksource/sh_cmt.c
@@ -82,14 +82,13 @@ struct sh_cmt_info {
unsigned long clear_bits;
/* callbacks for CMSTR and CMCSR access */
- unsigned long (*read_control)(void __iomem *base, unsigned long offs);
+ u32 (*read_control)(void __iomem *base, unsigned long offs);
void (*write_control)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 value);
/* callbacks for CMCNT and CMCOR access */
- unsigned long (*read_count)(void __iomem *base, unsigned long offs);
- void (*write_count)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 (*read_count)(void __iomem *base, unsigned long offs);
+ void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
};
struct sh_cmt_channel {
@@ -103,9 +102,9 @@ struct sh_cmt_channel {
unsigned int timer_bit;
unsigned long flags;
- unsigned long match_value;
- unsigned long next_match_value;
- unsigned long max_match_value;
+ u32 match_value;
+ u32 next_match_value;
+ u32 max_match_value;
raw_spinlock_t lock;
struct clock_event_device ced;
struct clocksource cs;
@@ -160,24 +159,22 @@ struct sh_cmt_device {
#define SH_CMT32_CMCSR_CKS_RCLK1 (7 << 0)
#define SH_CMT32_CMCSR_CKS_MASK (7 << 0)
-static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read16(void __iomem *base, unsigned long offs)
{
return ioread16(base + (offs << 1));
}
-static unsigned long sh_cmt_read32(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read32(void __iomem *base, unsigned long offs)
{
return ioread32(base + (offs << 2));
}
-static void sh_cmt_write16(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write16(void __iomem *base, unsigned long offs, u32 value)
{
iowrite16(value, base + (offs << 1));
}
-static void sh_cmt_write32(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write32(void __iomem *base, unsigned long offs, u32 value)
{
iowrite32(value, base + (offs << 2));
}
@@ -242,7 +239,7 @@ static const struct sh_cmt_info sh_cmt_i
#define CMCNT 1 /* channel register */
#define CMCOR 2 /* channel register */
-static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
{
if (ch->iostart)
return ch->cmt->info->read_control(ch->iostart, 0);
@@ -259,30 +256,27 @@ static inline void sh_cmt_write_cmstr(st
ch->cmt->info->write_control(ch->cmt->mapbase, 0, value);
}
-static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
{
return ch->cmt->info->read_control(ch->ioctrl, CMCSR);
}
-static inline void sh_cmt_write_cmcsr(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcsr(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_control(ch->ioctrl, CMCSR, value);
}
-static inline unsigned long sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
{
return ch->cmt->info->read_count(ch->ioctrl, CMCNT);
}
-static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_count(ch->ioctrl, CMCNT, value);
}
-static inline void sh_cmt_write_cmcor(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcor(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_count(ch->ioctrl, CMCOR, value);
}
@@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
int *has_wrapped)
{
- unsigned long v1, v2, v3;
+ u32 v1, v2, v3;
int o1, o2;
o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
@@ -311,7 +305,8 @@ static unsigned long sh_cmt_get_counter(
static void sh_cmt_start_stop_ch(struct sh_cmt_channel *ch, int start)
{
- unsigned long flags, value;
+ unsigned long flags;
+ u32 value;
/* start stop register shared by multiple timer channels */
raw_spin_lock_irqsave(&ch->cmt->lock, flags);
@@ -418,10 +413,10 @@ static void sh_cmt_disable(struct sh_cmt
static void sh_cmt_clock_event_program_verify(struct sh_cmt_channel *ch,
int absolute)
{
- unsigned long new_match;
- unsigned long value = ch->next_match_value;
- unsigned long delay = 0;
- unsigned long now = 0;
+ u32 value = ch->next_match_value;
+ u32 new_match;
+ u32 delay = 0;
+ u32 now = 0;
int has_wrapped;
now = sh_cmt_get_counter(ch, &has_wrapped);
@@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
static u64 sh_cmt_clocksource_read(struct clocksource *cs)
{
struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
- unsigned long flags, raw;
+ unsigned long flags;
unsigned long value;
int has_wrapped;
+ u32 raw;
raw_spin_lock_irqsave(&ch->lock, flags);
value = ch->total_cycles;
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
@ 2018-09-04 18:31 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-09-04 18:31 UTC (permalink / raw)
To: Daniel Lezcano, Thomas Gleixner, linux-renesas-soc
Cc: Vladimir Barinov, Andrey Gusakov, Mikhail Ulyanov, linux-sh
When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
/proc/timer_list. It turned out that when calculating it, the driver did
1 << 32 (causing what I think was undefined behavior) resulting in a zero
delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
to be that the driver abused *unsigned long* for the CMT register values
(which are 16/32-bit), so that the calculation of 'ch->max_match_value'
in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
instead fixed 'max_delta_ns' and even fixed the switching an active
clocksource to CMT (which caused the system to turn non-interactive
before).
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
This patch is against the 'tip.git' repo's 'timers/core' branch.
The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
enabling building of this driver now, so not sure how urgent is this...
Changes in version 2:
- completely redid the patch, fixing abuse of *unsigned long* for the CMT
register values.
drivers/clocksource/sh_cmt.c | 56 +++++++++++++++++++------------------------
1 file changed, 26 insertions(+), 30 deletions(-)
Index: tip/drivers/clocksource/sh_cmt.c
===================================================================
--- tip.orig/drivers/clocksource/sh_cmt.c
+++ tip/drivers/clocksource/sh_cmt.c
@@ -82,14 +82,13 @@ struct sh_cmt_info {
unsigned long clear_bits;
/* callbacks for CMSTR and CMCSR access */
- unsigned long (*read_control)(void __iomem *base, unsigned long offs);
+ u32 (*read_control)(void __iomem *base, unsigned long offs);
void (*write_control)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 value);
/* callbacks for CMCNT and CMCOR access */
- unsigned long (*read_count)(void __iomem *base, unsigned long offs);
- void (*write_count)(void __iomem *base, unsigned long offs,
- unsigned long value);
+ u32 (*read_count)(void __iomem *base, unsigned long offs);
+ void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
};
struct sh_cmt_channel {
@@ -103,9 +102,9 @@ struct sh_cmt_channel {
unsigned int timer_bit;
unsigned long flags;
- unsigned long match_value;
- unsigned long next_match_value;
- unsigned long max_match_value;
+ u32 match_value;
+ u32 next_match_value;
+ u32 max_match_value;
raw_spinlock_t lock;
struct clock_event_device ced;
struct clocksource cs;
@@ -160,24 +159,22 @@ struct sh_cmt_device {
#define SH_CMT32_CMCSR_CKS_RCLK1 (7 << 0)
#define SH_CMT32_CMCSR_CKS_MASK (7 << 0)
-static unsigned long sh_cmt_read16(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read16(void __iomem *base, unsigned long offs)
{
return ioread16(base + (offs << 1));
}
-static unsigned long sh_cmt_read32(void __iomem *base, unsigned long offs)
+static u32 sh_cmt_read32(void __iomem *base, unsigned long offs)
{
return ioread32(base + (offs << 2));
}
-static void sh_cmt_write16(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write16(void __iomem *base, unsigned long offs, u32 value)
{
iowrite16(value, base + (offs << 1));
}
-static void sh_cmt_write32(void __iomem *base, unsigned long offs,
- unsigned long value)
+static void sh_cmt_write32(void __iomem *base, unsigned long offs, u32 value)
{
iowrite32(value, base + (offs << 2));
}
@@ -242,7 +239,7 @@ static const struct sh_cmt_info sh_cmt_i
#define CMCNT 1 /* channel register */
#define CMCOR 2 /* channel register */
-static inline unsigned long sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmstr(struct sh_cmt_channel *ch)
{
if (ch->iostart)
return ch->cmt->info->read_control(ch->iostart, 0);
@@ -259,30 +256,27 @@ static inline void sh_cmt_write_cmstr(st
ch->cmt->info->write_control(ch->cmt->mapbase, 0, value);
}
-static inline unsigned long sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmcsr(struct sh_cmt_channel *ch)
{
return ch->cmt->info->read_control(ch->ioctrl, CMCSR);
}
-static inline void sh_cmt_write_cmcsr(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcsr(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_control(ch->ioctrl, CMCSR, value);
}
-static inline unsigned long sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
+static inline u32 sh_cmt_read_cmcnt(struct sh_cmt_channel *ch)
{
return ch->cmt->info->read_count(ch->ioctrl, CMCNT);
}
-static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcnt(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_count(ch->ioctrl, CMCNT, value);
}
-static inline void sh_cmt_write_cmcor(struct sh_cmt_channel *ch,
- unsigned long value)
+static inline void sh_cmt_write_cmcor(struct sh_cmt_channel *ch, u32 value)
{
ch->cmt->info->write_count(ch->ioctrl, CMCOR, value);
}
@@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
int *has_wrapped)
{
- unsigned long v1, v2, v3;
+ u32 v1, v2, v3;
int o1, o2;
o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
@@ -311,7 +305,8 @@ static unsigned long sh_cmt_get_counter(
static void sh_cmt_start_stop_ch(struct sh_cmt_channel *ch, int start)
{
- unsigned long flags, value;
+ unsigned long flags;
+ u32 value;
/* start stop register shared by multiple timer channels */
raw_spin_lock_irqsave(&ch->cmt->lock, flags);
@@ -418,10 +413,10 @@ static void sh_cmt_disable(struct sh_cmt
static void sh_cmt_clock_event_program_verify(struct sh_cmt_channel *ch,
int absolute)
{
- unsigned long new_match;
- unsigned long value = ch->next_match_value;
- unsigned long delay = 0;
- unsigned long now = 0;
+ u32 value = ch->next_match_value;
+ u32 new_match;
+ u32 delay = 0;
+ u32 now = 0;
int has_wrapped;
now = sh_cmt_get_counter(ch, &has_wrapped);
@@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
static u64 sh_cmt_clocksource_read(struct clocksource *cs)
{
struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
- unsigned long flags, raw;
+ unsigned long flags;
unsigned long value;
int has_wrapped;
+ u32 raw;
raw_spin_lock_irqsave(&ch->lock, flags);
value = ch->total_cycles;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
2018-09-04 18:31 ` Sergei Shtylyov
@ 2018-09-06 12:18 ` Geert Uytterhoeven
-1 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2018-09-06 12:18 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Daniel Lezcano, Thomas Gleixner, Linux-Renesas, Vladimir Barinov,
andrey.gusakov, Mikhail Ulyanov, Linux-sh list
Hi Sergei,
On Tue, Sep 4, 2018 at 8:32 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
> /proc/timer_list. It turned out that when calculating it, the driver did
> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
> to be that the driver abused *unsigned long* for the CMT register values
> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
> in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
> instead fixed 'max_delta_ns' and even fixed the switching an active
> clocksource to CMT (which caused the system to turn non-interactive
> before).
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> This patch is against the 'tip.git' repo's 'timers/core' branch.
> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
> enabling building of this driver now, so not sure how urgent is this...
>
> Changes in version 2:
> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
> register values.
Thanks for the update!
> --- tip.orig/drivers/clocksource/sh_cmt.c
> +++ tip/drivers/clocksource/sh_cmt.c
> @@ -82,14 +82,13 @@ struct sh_cmt_info {
> unsigned long clear_bits;
"clear_bits" should be u32, as it corresponds to a register value.
>
> /* callbacks for CMSTR and CMCSR access */
> - unsigned long (*read_control)(void __iomem *base, unsigned long offs);
> + u32 (*read_control)(void __iomem *base, unsigned long offs);
> void (*write_control)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 value);
>
> /* callbacks for CMCNT and CMCOR access */
> - unsigned long (*read_count)(void __iomem *base, unsigned long offs);
> - void (*write_count)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 (*read_count)(void __iomem *base, unsigned long offs);
> + void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
> };
>
> struct sh_cmt_channel {
> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>
> unsigned int timer_bit;
> unsigned long flags;
While no register value, "flags" can be unsigned int, too.
> - unsigned long match_value;
> - unsigned long next_match_value;
> - unsigned long max_match_value;
> + u32 match_value;
> + u32 next_match_value;
> + u32 max_match_value;
> raw_spinlock_t lock;
> struct clock_event_device ced;
> struct clocksource cs;
> @@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
> static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
Return type should be "u32".
> int *has_wrapped)
While not "unsigned long", "has_wrapped" is used to store a register value,
so I think it should be changed to "u32 *".
> {
> - unsigned long v1, v2, v3;
> + u32 v1, v2, v3;
> int o1, o2;
While not "unsigned long", "o1" and "o2" contain register values, so I think
they should be "u32".
> o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
"overflow_bit" should become "u32", too.
> @@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
> static u64 sh_cmt_clocksource_read(struct clocksource *cs)
> {
> struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
> - unsigned long flags, raw;
> + unsigned long flags;
> unsigned long value;
> int has_wrapped;
> + u32 raw;
>
> raw_spin_lock_irqsave(&ch->lock, flags);
> value = ch->total_cycles;
"value" and "total_cycles" should be "u64". But that's a separate bug fix.
Not in the context, so I cannot comment to it, but "sh_cmt_info.width" should
be "unsigned int", too.
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
@ 2018-09-06 12:18 ` Geert Uytterhoeven
0 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2018-09-06 12:18 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Daniel Lezcano, Thomas Gleixner, Linux-Renesas, Vladimir Barinov,
andrey.gusakov, Mikhail Ulyanov, Linux-sh list
Hi Sergei,
On Tue, Sep 4, 2018 at 8:32 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
> /proc/timer_list. It turned out that when calculating it, the driver did
> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
> to be that the driver abused *unsigned long* for the CMT register values
> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
> in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
> instead fixed 'max_delta_ns' and even fixed the switching an active
> clocksource to CMT (which caused the system to turn non-interactive
> before).
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> This patch is against the 'tip.git' repo's 'timers/core' branch.
> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
> enabling building of this driver now, so not sure how urgent is this...
>
> Changes in version 2:
> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
> register values.
Thanks for the update!
> --- tip.orig/drivers/clocksource/sh_cmt.c
> +++ tip/drivers/clocksource/sh_cmt.c
> @@ -82,14 +82,13 @@ struct sh_cmt_info {
> unsigned long clear_bits;
"clear_bits" should be u32, as it corresponds to a register value.
>
> /* callbacks for CMSTR and CMCSR access */
> - unsigned long (*read_control)(void __iomem *base, unsigned long offs);
> + u32 (*read_control)(void __iomem *base, unsigned long offs);
> void (*write_control)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 value);
>
> /* callbacks for CMCNT and CMCOR access */
> - unsigned long (*read_count)(void __iomem *base, unsigned long offs);
> - void (*write_count)(void __iomem *base, unsigned long offs,
> - unsigned long value);
> + u32 (*read_count)(void __iomem *base, unsigned long offs);
> + void (*write_count)(void __iomem *base, unsigned long offs, u32 value);
> };
>
> struct sh_cmt_channel {
> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>
> unsigned int timer_bit;
> unsigned long flags;
While no register value, "flags" can be unsigned int, too.
> - unsigned long match_value;
> - unsigned long next_match_value;
> - unsigned long max_match_value;
> + u32 match_value;
> + u32 next_match_value;
> + u32 max_match_value;
> raw_spinlock_t lock;
> struct clock_event_device ced;
> struct clocksource cs;
> @@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
> static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
Return type should be "u32".
> int *has_wrapped)
While not "unsigned long", "has_wrapped" is used to store a register value,
so I think it should be changed to "u32 *".
> {
> - unsigned long v1, v2, v3;
> + u32 v1, v2, v3;
> int o1, o2;
While not "unsigned long", "o1" and "o2" contain register values, so I think
they should be "u32".
> o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
"overflow_bit" should become "u32", too.
> @@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
> static u64 sh_cmt_clocksource_read(struct clocksource *cs)
> {
> struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
> - unsigned long flags, raw;
> + unsigned long flags;
> unsigned long value;
> int has_wrapped;
> + u32 raw;
>
> raw_spin_lock_irqsave(&ch->lock, flags);
> value = ch->total_cycles;
"value" and "total_cycles" should be "u64". But that's a separate bug fix.
Not in the context, so I cannot comment to it, but "sh_cmt_info.width" should
be "unsigned int", too.
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
2018-09-06 12:18 ` Geert Uytterhoeven
@ 2018-09-08 17:20 ` Sergei Shtylyov
-1 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-09-08 17:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Daniel Lezcano, Thomas Gleixner, Linux-Renesas, Vladimir Barinov,
andrey.gusakov, Mikhail Ulyanov, Linux-sh list
Hello!
On 09/06/2018 03:18 PM, Geert Uytterhoeven wrote:
>> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
>> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
>> /proc/timer_list. It turned out that when calculating it, the driver did
>> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
>> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
>> to be that the driver abused *unsigned long* for the CMT register values
>> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
>> in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
>> instead fixed 'max_delta_ns' and even fixed the switching an active
>> clocksource to CMT (which caused the system to turn non-interactive
>> before).
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> This patch is against the 'tip.git' repo's 'timers/core' branch.
>> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
>> enabling building of this driver now, so not sure how urgent is this...
>>
>> Changes in version 2:
>> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
>> register values.
>
> Thanks for the update!
>
>> --- tip.orig/drivers/clocksource/sh_cmt.c
>> +++ tip/drivers/clocksource/sh_cmt.c
>> @@ -82,14 +82,13 @@ struct sh_cmt_info {
>> unsigned long clear_bits;
>
> "clear_bits" should be u32, as it corresponds to a register value.
OK.
>> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>>
>> unsigned int timer_bit;
>> unsigned long flags;
>
> While no register value, "flags" can be unsigned int, too.
OK to leave that for another patch?
>> - unsigned long match_value;
>> - unsigned long next_match_value;
>> - unsigned long max_match_value;
>> + u32 match_value;
>> + u32 next_match_value;
>> + u32 max_match_value;
>> raw_spinlock_t lock;
>> struct clock_event_device ced;
>> struct clocksource cs;
>
>> @@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
>> static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
>
> Return type should be "u32".
Thanks, missed it...
>> int *has_wrapped)
>
> While not "unsigned long", "has_wrapped" is used to store a register value,
> so I think it should be changed to "u32 *".
Hm, indeed...
>> {
>> - unsigned long v1, v2, v3;
>> + u32 v1, v2, v3;
>> int o1, o2;
>
> While not "unsigned long", "o1" and "o2" contain register values, so I think
> they should be "u32".
OK, lemme try that...
>> o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
>
> "overflow_bit" should become "u32", too.
I've figured. :-)
>
>> @@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
>> static u64 sh_cmt_clocksource_read(struct clocksource *cs)
>> {
>> struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
>> - unsigned long flags, raw;
>> + unsigned long flags;
>> unsigned long value;
>> int has_wrapped;
>> + u32 raw;
>>
>> raw_spin_lock_irqsave(&ch->lock, flags);
>> value = ch->total_cycles;
>
> "value" and "total_cycles" should be "u64". But that's a separate bug fix.
And for 32-bit CPUs, I guess? Will look into it...
> Not in the context, so I cannot comment to it, but "sh_cmt_info.width" should
> be "unsigned int", too.
Separate patch?
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines
@ 2018-09-08 17:20 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2018-09-08 17:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Daniel Lezcano, Thomas Gleixner, Linux-Renesas, Vladimir Barinov,
andrey.gusakov, Mikhail Ulyanov, Linux-sh list
Hello!
On 09/06/2018 03:18 PM, Geert Uytterhoeven wrote:
>> When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed
>> that 'max_delta_ns' for the broadcast timer (CMT) was shown as 1000 in
>> /proc/timer_list. It turned out that when calculating it, the driver did
>> 1 << 32 (causing what I think was undefined behavior) resulting in a zero
>> delta, later clamped to 1000 by cev_delta2ns(). The root cause turned out
>> to be that the driver abused *unsigned long* for the CMT register values
>> (which are 16/32-bit), so that the calculation of 'ch->max_match_value'
>> in sh_cmt_setup_channel() used the worng branch. Using more proper 'u32'
>> instead fixed 'max_delta_ns' and even fixed the switching an active
>> clocksource to CMT (which caused the system to turn non-interactive
>> before).
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> This patch is against the 'tip.git' repo's 'timers/core' branch.
>> The CMT driver wasn't ever used on SH64; on R-Car gen3 (ARM64) I'm only
>> enabling building of this driver now, so not sure how urgent is this...
>>
>> Changes in version 2:
>> - completely redid the patch, fixing abuse of *unsigned long* for the CMT
>> register values.
>
> Thanks for the update!
>
>> --- tip.orig/drivers/clocksource/sh_cmt.c
>> +++ tip/drivers/clocksource/sh_cmt.c
>> @@ -82,14 +82,13 @@ struct sh_cmt_info {
>> unsigned long clear_bits;
>
> "clear_bits" should be u32, as it corresponds to a register value.
OK.
>> @@ -103,9 +102,9 @@ struct sh_cmt_channel {
>>
>> unsigned int timer_bit;
>> unsigned long flags;
>
> While no register value, "flags" can be unsigned int, too.
OK to leave that for another patch?
>> - unsigned long match_value;
>> - unsigned long next_match_value;
>> - unsigned long max_match_value;
>> + u32 match_value;
>> + u32 next_match_value;
>> + u32 max_match_value;
>> raw_spinlock_t lock;
>> struct clock_event_device ced;
>> struct clocksource cs;
>
>> @@ -290,7 +284,7 @@ static inline void sh_cmt_write_cmcor(st
>> static unsigned long sh_cmt_get_counter(struct sh_cmt_channel *ch,
>
> Return type should be "u32".
Thanks, missed it...
>> int *has_wrapped)
>
> While not "unsigned long", "has_wrapped" is used to store a register value,
> so I think it should be changed to "u32 *".
Hm, indeed...
>> {
>> - unsigned long v1, v2, v3;
>> + u32 v1, v2, v3;
>> int o1, o2;
>
> While not "unsigned long", "o1" and "o2" contain register values, so I think
> they should be "u32".
OK, lemme try that...
>> o1 = sh_cmt_read_cmcsr(ch) & ch->cmt->info->overflow_bit;
>
> "overflow_bit" should become "u32", too.
I've figured. :-)
>
>> @@ -619,9 +614,10 @@ static struct sh_cmt_channel *cs_to_sh_c
>> static u64 sh_cmt_clocksource_read(struct clocksource *cs)
>> {
>> struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
>> - unsigned long flags, raw;
>> + unsigned long flags;
>> unsigned long value;
>> int has_wrapped;
>> + u32 raw;
>>
>> raw_spin_lock_irqsave(&ch->lock, flags);
>> value = ch->total_cycles;
>
> "value" and "total_cycles" should be "u64". But that's a separate bug fix.
And for 32-bit CPUs, I guess? Will look into it...
> Not in the context, so I cannot comment to it, but "sh_cmt_info.width" should
> be "unsigned int", too.
Separate patch?
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-08 22:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 18:31 [PATCH v2] clocksource: sh_cmt: fixup for 64-bit machines Sergei Shtylyov
2018-09-04 18:31 ` Sergei Shtylyov
2018-09-06 12:18 ` Geert Uytterhoeven
2018-09-06 12:18 ` Geert Uytterhoeven
2018-09-08 17:20 ` Sergei Shtylyov
2018-09-08 17:20 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.