| Message ID | 8aa6d2cd-68f9-eae4-cba0-222c1511eb9d@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clocksource: sh_cmt: fixup for 64-bit machines | expand |
Hi Sergei, On Thu, Aug 30, 2018 at 9:19 PM Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed that > the maximum delta (in ns) for the broadcast timer was diplayed as 1000 in > /proc/timer_list. It turned out that when calculating it, the driver did > shift left 1 by 32 (causing what I think was undefined behaviour) getting > 1 as a result. Using 1UL instead fixed the maximum delta and did even fix > switching an active clocksource to a CMT channel (which caused the system > to go non-interactive before). > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'tip.git' repo's 'timers/core' branch. > > I am not sure whether the CMT driver was ever used on SH64; on R-Car gen3 > (ARM64) I'm only enabling this driver now, so not sure how urgent is this... > > drivers/clocksource/sh_cmt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Index: tip/drivers/clocksource/sh_cmt.c > =================================================================== > --- tip.orig/drivers/clocksource/sh_cmt.c > +++ tip/drivers/clocksource/sh_cmt.c > @@ -891,7 +891,7 @@ static int sh_cmt_setup_channel(struct s > if (cmt->info->width == (sizeof(ch->max_match_value) * 8)) > ch->max_match_value = ~0; > else > - ch->max_match_value = (1 << cmt->info->width) - 1; > + ch->max_match_value = (1UL << cmt->info->width) - 1; > > ch->match_value = ch->max_match_value; > raw_spin_lock_init(&ch->lock); Shouldn't all those fields/variables be changed from "unsigned long" to "u32", as they really should match the CMT's register sizes? Gr{oetje,eeting}s, Geert
Hi Sergie, Thanks for the patch. > Subject: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines > > When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed that > the maximum delta (in ns) for the broadcast timer was diplayed as 1000 in %s/ diplayed/displayed Regards, Biju Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hi Sergei, On Fri, Aug 31, 2018 at 11:05 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 30, 2018 at 9:19 PM Sergei Shtylyov > <sergei.shtylyov@cogentembedded.com> wrote: > > When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed that > > the maximum delta (in ns) for the broadcast timer was diplayed as 1000 in > > /proc/timer_list. It turned out that when calculating it, the driver did > > shift left 1 by 32 (causing what I think was undefined behaviour) getting > > 1 as a result. Using 1UL instead fixed the maximum delta and did even fix > > switching an active clocksource to a CMT channel (which caused the system > > to go non-interactive before). > > > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > > > --- > > This patch is against the 'tip.git' repo's 'timers/core' branch. > > > > I am not sure whether the CMT driver was ever used on SH64; on R-Car gen3 > > (ARM64) I'm only enabling this driver now, so not sure how urgent is this... > > > > drivers/clocksource/sh_cmt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > Index: tip/drivers/clocksource/sh_cmt.c > > =================================================================== > > --- tip.orig/drivers/clocksource/sh_cmt.c > > +++ tip/drivers/clocksource/sh_cmt.c > > @@ -891,7 +891,7 @@ static int sh_cmt_setup_channel(struct s > > if (cmt->info->width == (sizeof(ch->max_match_value) * 8)) > > ch->max_match_value = ~0; > > else > > - ch->max_match_value = (1 << cmt->info->width) - 1; > > + ch->max_match_value = (1UL << cmt->info->width) - 1; > > > > ch->match_value = ch->max_match_value; > > raw_spin_lock_init(&ch->lock); > > Shouldn't all those fields/variables be changed from "unsigned long" to "u32", > as they really should match the CMT's register sizes? To clarify: the if-check above is intended to catch a shift by 32. But it fails to do that on 64-bit platforms, as max_match_value is unsigned long, and its size thus depends on the platform, while the CMT's register sizes are fixed to 32-bit. Hence please fix the real bug (abuse of unsigned long for 32-bit values all over the place), instead of papering over it. Thanks! Gr{oetje,eeting}s, Geert
Index: tip/drivers/clocksource/sh_cmt.c =================================================================== --- tip.orig/drivers/clocksource/sh_cmt.c +++ tip/drivers/clocksource/sh_cmt.c @@ -891,7 +891,7 @@ static int sh_cmt_setup_channel(struct s if (cmt->info->width == (sizeof(ch->max_match_value) * 8)) ch->max_match_value = ~0; else - ch->max_match_value = (1 << cmt->info->width) - 1; + ch->max_match_value = (1UL << cmt->info->width) - 1; ch->match_value = ch->max_match_value; raw_spin_lock_init(&ch->lock);
When trying to use CMT for clockevents on R-Car gen3 SoCs, I noticed that the maximum delta (in ns) for the broadcast timer was diplayed as 1000 in /proc/timer_list. It turned out that when calculating it, the driver did shift left 1 by 32 (causing what I think was undefined behaviour) getting 1 as a result. Using 1UL instead fixed the maximum delta and did even fix switching an active clocksource to a CMT channel (which caused the system to go non-interactive before). Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'tip.git' repo's 'timers/core' branch. I am not sure whether the CMT driver was ever used on SH64; on R-Car gen3 (ARM64) I'm only enabling this driver now, so not sure how urgent is this... drivers/clocksource/sh_cmt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)