Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

[John Stultz]

On 11/16/2012 06:53 PM, Anton Vorontsov wrote:
> On Fri, Nov 09, 2012 at 05:26:53PM -0800, Kees Cook wrote:
> [....]
> > > > @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
> > > > persistent_ram_zone *prz)
> > > >          struct timeval timestamp;
> > > >          size_t len;
> > > >
> > > > -       do_gettimeofday(&timestamp);
> > > > +       /* Handle dumping before timekeeping has resumed. */
> > > > +       if (unlikely(timekeeping_suspended)) {
> > > > +               timestamp.tv_sec = 0;
> > > > +               timestamp.tv_usec = 0;
> > > > +       } else
> > > > +               do_gettimeofday(&timestamp);
> > > > +
> > > Would nulling out the timestamp be better done in do_gettimeofday()?  That
> > > way we don't have to export timekeeping internals and users would get
> > > something more sane for this corner case.
> > Well... I'm not sure. If we don't want to expose the
> > timekeeping_suspended variable, maybe we need a function to check
> > this? I think it's probably better to find the users of timekeeping
> > that could call it when suspended. That's why I figured the BUG was
> > there. Very very few things should be attempting to call gettimeofday
> > in a place where it might be suspended. As such, it seems like those
> > things should be able to determine how to handle it. Maybe not
> > everything would be sensible to get back 0s.
> >
> > In this particular case, I'm fine with removing the BUG and returning
> > 0 instead, since that's fine for ramoops. :)
> In the lack of agreement on kernel/time/timekeeping.c change, I can't
> apply the patch. And personally I tend to agree that doing this workaround
> in the pstore code is odd. How about introducing ___do_gettimeofday() that
> is safe to call when suspened, and the func would have good kernel doc
> comments explaining the purpose of it?
Yea, I wanted to revisit this, because it is an odd case.

We don't want to call getnstimeofday() while the timekeeping code is 
suspended, since the clocksource cycle_last value may be invalid if the 
hardware was reset during suspend.  Kees is correct,  the WARN_ONs were 
there to make sure no one tries to use the timekeeping core before its 
resumed, so removing them is problematic.

Your sugggestion of having the __do_gettimeofday() internal accessor 
that maybe returns an error if timekeeping has been suspended could work.

The other possibility is depending on the needs for accuracy with the 
timestamp, current_kernel_time() might be a better interface to use, 
since it will return the time at the last tick, and doesn't require 
accessing the clocksource hardware.  Might that be a simpler solution? 
Or is sub-tick granularity necessary?

thanks
-john







--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/