| Message ID | e7324659-7697-7d8a-dcc4-edd8968bcfad@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] clocksource: sh_cmt: fixup for 64-bit machines | expand |
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
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
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;
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(-)