[PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Sergei Shtylyov]

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);

[PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Sergei Shtylyov]

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);

Re: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Geert Uytterhoeven]

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

-- 
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

RE: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Biju Das]

SGkgU2VyZ2llLA0KDQpUaGFua3MgZm9yIHRoZSBwYXRjaC4NCg0KPiBTdWJqZWN0OiBbUEFUQ0hd
IGNsb2Nrc291cmNlOiBzaF9jbXQ6IGZpeHVwIGZvciA2NC1iaXQgbWFjaGluZXMNCj4NCj4gV2hl
biB0cnlpbmcgdG8gdXNlIENNVCBmb3IgY2xvY2tldmVudHMgb24gUi1DYXIgZ2VuMyBTb0NzLCBJ
IG5vdGljZWQgdGhhdA0KPiB0aGUgbWF4aW11bSBkZWx0YSAoaW4gbnMpIGZvciB0aGUgYnJvYWRj
YXN0IHRpbWVyIHdhcyBkaXBsYXllZCBhcyAxMDAwIGluDQoNCiVzLyBkaXBsYXllZC9kaXNwbGF5
ZWQNCg0KUmVnYXJkcywNCkJpanUNCg0KDQoNCg0KUmVuZXNhcyBFbGVjdHJvbmljcyBFdXJvcGUg
THRkLCBEdWtlcyBNZWFkb3csIE1pbGxib2FyZCBSb2FkLCBCb3VybmUgRW5kLCBCdWNraW5naGFt
c2hpcmUsIFNMOCA1RkgsIFVLLiBSZWdpc3RlcmVkIGluIEVuZ2xhbmQgJiBXYWxlcyB1bmRlciBS
ZWdpc3RlcmVkIE5vLiAwNDU4NjcwOS4NCg=

RE: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Biju Das]

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.

Re: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH] clocksource: sh_cmt: fixup for 64-bit machines

[Geert Uytterhoeven]

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

-- 
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