| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 5 Jan 2022 10:29:08 +0800
From: weidong guan <weidong.guanguan@...il.com>
To: Baolin Wang <baolin.wang7@...il.com>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Orson Zhai <orsonzhai@...il.com>,
Chunyan Zhang <zhang.lyra@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
"weidong.guan" <weidong.guan@...soc.com>
Subject: Re: [PATCH] clocksource: fix counter with 32bit may wrap around
On Tue, Jan 4, 2022 at 9:42 AM Baolin Wang <baolin.wang7@...il.com> wrote:
>
> Hi,
>
> On Fri, Dec 31, 2021 at 2:03 PM weidong guan <weidong.guanguan@...il.com> wrote:
> >
> > From: "weidong.guan" <weidong.guan@...soc.com>
> >
> > When the system sleep time exceeds 30+ hours, counter with 32bit may wrap
>
> I am curious about which condition the system can suspend for more
> than 30+ hours, from my understanding, the phone will be woken up
> periodically.
Yes, it does on mobile phones, but on some non-mobile devices,
it would continue to sleep until you wake it up actively.Just as logs
shown below:
[89438.802502][11-09 09:14:34.802] PM: suspend entry (deep)
[200927.406098][11-10 16:12:43.406] PM: suspend exit
And in my opinion, should we try to avoid counter overflow even if
theoretically?
>
> > around, resulting in insufficient sleep compensation time. So We should
> > enable 64bit counter mode of sprd timer and use the given mult/shift
> > to ensure that sufficient counts can be generated.
> >
> > Signed-off-by: weidong.guan <weidong.guan@...soc.com>
> > ---
> > drivers/clocksource/timer-sprd.c | 31 ++++++++++++++++++++-----------
> > 1 file changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-sprd.c b/drivers/clocksource/timer-sprd.c
> > index 430cb99..7eac059 100644
> > --- a/drivers/clocksource/timer-sprd.c
> > +++ b/drivers/clocksource/timer-sprd.c
> > @@ -30,6 +30,7 @@
> > #define TIMER_VALUE_SHDW_HI 0x1c
> >
> > #define TIMER_VALUE_LO_MASK GENMASK(31, 0)
> > +#define TIMER_VALUE_64BIT_MASK GENMASK_ULL(63, 0)
> >
> > static void sprd_timer_enable(void __iomem *base, u32 flag)
> > {
> > @@ -57,10 +58,12 @@ static void sprd_timer_disable(void __iomem *base)
> > writel_relaxed(val, base + TIMER_CTL);
> > }
> >
> > -static void sprd_timer_update_counter(void __iomem *base, unsigned long cycles)
> > +static void
> > +sprd_timer_update_counter(void __iomem *base, unsigned long long cycles)
> > {
> > writel_relaxed(cycles & TIMER_VALUE_LO_MASK, base + TIMER_LOAD_LO);
> > - writel_relaxed(0, base + TIMER_LOAD_HI);
> > + writel_relaxed((cycles >> 32) & TIMER_VALUE_LO_MASK,
> > + base + TIMER_LOAD_HI);
> > }
> >
> > static void sprd_timer_enable_interrupt(void __iomem *base)
> > @@ -82,7 +85,7 @@ static int sprd_timer_set_next_event(unsigned long cycles,
> > struct timer_of *to = to_timer_of(ce);
> >
> > sprd_timer_disable(timer_of_base(to));
> > - sprd_timer_update_counter(timer_of_base(to), cycles);
> > + sprd_timer_update_counter(timer_of_base(to), (u64)cycles);
> > sprd_timer_enable(timer_of_base(to), 0);
> >
> > return 0;
> > @@ -93,7 +96,8 @@ static int sprd_timer_set_periodic(struct clock_event_device *ce)
> > struct timer_of *to = to_timer_of(ce);
> >
> > sprd_timer_disable(timer_of_base(to));
> > - sprd_timer_update_counter(timer_of_base(to), timer_of_period(to));
> > + sprd_timer_update_counter(timer_of_base(to),
> > + (u64)timer_of_period(to));
> > sprd_timer_enable(timer_of_base(to), TIMER_CTL_PERIOD_MODE);
> >
> > return 0;
> > @@ -162,15 +166,19 @@ static int __init sprd_timer_init(struct device_node *np)
> >
> > static u64 sprd_suspend_timer_read(struct clocksource *cs)
> > {
> > - return ~(u64)readl_relaxed(timer_of_base(&suspend_to) +
> > - TIMER_VALUE_SHDW_LO) & cs->mask;
> > + u64 value_hi, value_lo;
> > +
> > + value_hi = (u64)readl_relaxed(timer_of_base(&suspend_to) + TIMER_VALUE_SHDW_HI);
> > + value_lo = (u64)readl_relaxed(timer_of_base(&suspend_to) + TIMER_VALUE_SHDW_LO);
> > + return ~(u64)((value_hi << 32) + value_lo) & cs->mask;
> > }
> >
> > static int sprd_suspend_timer_enable(struct clocksource *cs)
> > {
> > sprd_timer_update_counter(timer_of_base(&suspend_to),
> > - TIMER_VALUE_LO_MASK);
> > - sprd_timer_enable(timer_of_base(&suspend_to), TIMER_CTL_PERIOD_MODE);
> > + TIMER_VALUE_64BIT_MASK);
> > + sprd_timer_enable(timer_of_base(&suspend_to),
> > + TIMER_CTL_PERIOD_MODE|TIMER_CTL_64BIT_WIDTH);
> >
> > return 0;
> > }
> > @@ -183,10 +191,12 @@ static void sprd_suspend_timer_disable(struct clocksource *cs)
> > static struct clocksource suspend_clocksource = {
> > .name = "sprd_suspend_timer",
> > .rating = 200,
> > + .mult = 0x1DCD650,
> > + .shift = 10,
>
> And why?
Do you mean why we use the given mult/shift? The reason is If we use the
mult/shift automatically calculated by kernel, the mult value is relatively
large, causing the not large enough maximum count value. So under the
precondition of ensuring accuracy, we calculate a relatively small mult/shift
value, aiming to get larger maximum count value.
>
> > .read = sprd_suspend_timer_read,
> > .enable = sprd_suspend_timer_enable,
> > .disable = sprd_suspend_timer_disable,
> > - .mask = CLOCKSOURCE_MASK(32),
> > + .mask = CLOCKSOURCE_MASK(64),
> > .flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,
> > };
> >
> > @@ -198,8 +208,7 @@ static int __init sprd_suspend_timer_init(struct device_node *np)
> > if (ret)
> > return ret;
> >
> > - clocksource_register_hz(&suspend_clocksource,
> > - timer_of_rate(&suspend_to));
> > + clocksource_register_hz(&suspend_clocksource, 0);
>
> --
> Baolin Wang
Powered by blists - more mailing lists