linux-kernel - Re: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 28 Jun 2013 23:20:19 +0200
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Tomasz Figa <tomasz.figa@...il.com>
Cc:	linux-arm-kernel@...ts.infradead.org,
	Thomas Gleixner <tglx@...utronix.de>,
	Emilio Lopez <emilio@...pez.com.ar>,
	linux-kernel@...r.kernel.org, linux-sunxi@...glegroups.com,
	John Stultz <john.stultz@...aro.org>, sunny@...winnertech.com,
	shuge@...winnertech.com, kevin@...winnertech.com
Subject: Re: [PATCHv2 4/8] clocksource: sun4i: Fix the next event code

On Fri, Jun 28, 2013 at 10:35:29PM +0200, Tomasz Figa wrote:
> On Friday 28 of June 2013 22:13:08 Thomas Gleixner wrote:
> > On Fri, 28 Jun 2013, Maxime Ripard wrote:
> > > The next_event logic was setting the next interval to fire in the
> > > current timer value instead of the interval value register, which is
> > > obviously wrong.
> > 
> > Ok.
> > 
> > > Plus the logic to set the actual value was wrong as well, so this
> > > code has always been broken.
> > 
> > This lacks an explanation why the logic is wrong and what the actual
> > fix is.
> > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>
> > > ---
> > > 
> > >  drivers/clocksource/sun4i_timer.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/clocksource/sun4i_timer.c
> > > b/drivers/clocksource/sun4i_timer.c index 84ace76..695c8c8 100644
> > > --- a/drivers/clocksource/sun4i_timer.c
> > > +++ b/drivers/clocksource/sun4i_timer.c
> > > @@ -16,6 +16,7 @@
> > > 
> > >  #include <linux/clk.h>
> > >  #include <linux/clockchips.h>
> > > 
> > > +#include <linux/delay.h>
> > > 
> > >  #include <linux/interrupt.h>
> > >  #include <linux/irq.h>
> > >  #include <linux/irqreturn.h>
> > > 
> > > @@ -61,9 +62,14 @@ static void sun4i_clkevt_mode(enum clock_event_mode
> > > mode,> 
> > >  static int sun4i_clkevt_next_event(unsigned long evt,
> > >  
> > >  				   struct clock_event_device *unused)
> > >  
> > >  {
> > > 
> > > -	u32 u = readl(timer_base + TIMER_CTL_REG(0));
> > > -	writel(evt, timer_base + TIMER_CNTVAL_REG(0));
> > > -	writel(u | TIMER_CTL_ENABLE | TIMER_CTL_AUTORELOAD,
> > > +	u32 val = readl(timer_base + TIMER_CTL_REG(0));
> > > +	writel(val & ~TIMER_CTL_ENABLE, timer_base + TIMER_CTL_REG(0));
> > > +	udelay(1);
> > 
> > That udelay() is more than suspicious.
> 
> Not only it is suspicious, but also delays the event by 1 microsecond. Not 
> much, given usual usage of clock events, but still.
> 
> From what I understand from this code, you keep this timer running and 
> just stop it to set new event. Can you simply disable autoreload and just 
> program this timer to start counting from evt down to 0 when it generates 
> interrupt and just stops itself?

Something like that, but not completely, because the timer actually
stops.

To reprogram a new interval to a running timer, you have to:
  - Disable it
  - Program the new interval
  - Propagates the new interval and start the timer by setting the bits
    ENABLE and (AUTO)RELOAD (AUTORELOAD is probably a bad name here
    actually). That is, wether or not it's a oneshot or periodic timer.

Now, between the time you disable the timer and enable it back, you have
to wait at least 2 timer clock source cycles (which is around 85ns).

It's the ONESHOT (BIT(7)) that actually controls wether or not the timer
is periodic.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)