Re: [patch 02/23] GTOD: persistent clock support, core

[David Brownell <david-b@pacbell.net>]

On Tuesday 03 October 2006 12:19 pm, john stultz wrote:
> 
> Yea. First, let me apologize for falling off the last thread. I got busy
> with other things and the discussion withered. This patch has been
> re-raised because Thomas finally tripped over the "theoretical" resume
> time ordering bug that I was concerned about.

I recall losing track of what that issue was, and thinking a new explanation
was needed ... no such issues were described in this patch or its summary.


> So, while my first announcement with the patch was something of the
> effect of "Trying to unify the cmos/RTC/whatever interface", due to our
> discussion I'm scaling back that goal.
> 
> Instead the purpose of this is just a continuation of the generic
> timekeeping work. Moving the *existing* arch specific resume timekeeping
> code into generic code,

... which wasn't what that one source code comment implied; quite
the opposite, it assumed all architectures _could_ work that way.


> On arches that have the constraints you list above, a dummy
> read_persistent_clock() that returns zero can be implemented, with a
> comment about why and the RTC_HCTOSYS bits can be used, with no change
> in behavior from what we have now.

Better IMO to use a more traditional run-time function call style
hook, not the link time magic you're now using.  Point is, this
is most accurately a _board_ option not an architectural one.

Even systems that _could_ provide some access to a persistent
clock without relying on IRQs might be designed not to do so.
For example, RTCs integrated into system-on-chip processors
can be less power-efficient than discrete ones, so that when
power-efficient design is essential, that integrated RTC may
not be a viable design option.

And since we expect Linux kernels to be able to run on more
than one board, that includes configurations where one of the
boards uses the integrated register-based RTC, and another
uses a discrete one with serial interface.  But your choice
of link-time binding precludes one kernel handling both boards
correctly...



> > > +/**
> > > + * read_persistent_clock -  Return time in seconds from the persistent clock.
> > > + *
> > > + * Weak dummy function for arches that do not yet support it.
> > > + * Returns seconds from epoch using the battery backed persistent clock.
> > > + * Returns zero if unsupported.
> > > + *
> > > + *  XXX - Do be sure to remove it once all arches implement it.
> > 
> > But not all architectures **CAN** support this notion ...
> 
> That's ok and is the reason why we have a unsupported return option.
> 
> This patch just gives us a path to consolidate what the majority of
> arches do currently.

That XXX comment disagrees ... it assumes all architectures can work
that way ...

And I'm not sure about "majority"; depends how you count.  Certainly
the number of embedded boards that could work differently is large and
growing ... and there are a lot more embedded computers in the world
than desktops, laptops, or servers.


> > > @@ -774,13 +801,23 @@ static int timekeeping_suspended;
> > >  static int timekeeping_resume(struct sys_device *dev)
> > >  {
> > >         unsigned long flags;
> > > +       unsigned long now = read_persistent_clock();
> > 
> > Again: sys_device resume() is called with IRQs disabled, which
> > prevents access to many systems' persistent clocks.  In fact,
> > after posting this example patch
> > 
> >   http://marc.theaimsgroup.com/?l=linux-kernel&m=115600629813751&w=2
> 
> 
> Ok, correct me if I'm wrong, though: The only catch, if I understand
> correctly, is that it requires the system in question to have a proper
> RTC driver, which doesn't cover 100% of the arch/platforms supported.
> No?

It's not a "catch" for the systems which have an RTC class driver,
and your approach doesn't cover 100% either.

The key difference between the two approaches is that yours **CANNOT**
handle cases like an I2C based RTC (call them "message based RTCs"), where
the RTC class drivers **CAN** handle "register based RTCs".


> I don't really have an issue w/ the RTC code above, however it does not
> address the current suspend/resume code in the majority of arches. I
> don't know if we're actually in that much conflict here, since I'm
> trying to remove the arch specific suspend/resume timekeeping changes,
> and (to my understanding) so are you.

It's a question of scope, I guess.  You're limiting your solution to
boards that don't require message based RTCs.  I'm thinking that's not
really sufficient ... and it bothers me to see something be pitched
as a basic architectural feature of the clock/time framework when
it's so clearly not sufficient.


> We just have a difference in where we're trying to re-add the code. I'm
> moving the current code into the generic tod path, and you're moving it
> into the RTC driver. Both efforts are consolidating code, so even with
> the minor duplication we have less code then before. So I'm sure as we
> whittle away at this we can find a way to meet in the middle. I think
> this patch moves us forward in that direction.

So long as this GTOD patch is updated to reflect the reality that
not all _boards_ can (or will) support that style of RTC ... cleanup
is good, but those code comments suggest deeper assumptions.


> > I never heard anything more from you on this issue.  Given this
> > particular patch (in $SUBJECT) should I assume you're going to
> > just ignore the issues whereby RTCs ("persistent clocks") can't
> > always meet the no-IRQs-needed assumptions being made here?  Or
> > address isssues like using pointer-to-function instead of using
> > linker tricks?
> 
> In my head I see it like this:
> 
> Currently here is how the timekeeping resume code breaks down:
> 1) timeofday_resume: reset clocksource, NTP
> 2) arch time resume: [set xtime]
> 3) RTC resume: [set xtime]
> 
> Where the [set xtime]s depend on platform config

That is, three different ways to do it.


> I'm trying to just move us to:
> 1) timeofday_resume: reset clocksource, NTP, [set xtime]
> 2) RTC resume: [set xtime]
> 
> Again, where the [set xtime]s depend on platform config

Getting rid of one is useful, yes.  But why not get rid of two?

 
> Then as the RTC drivers gain  coverage, maybe we can start cutting
> timeofday resume down and have something like:
> 1) timeofday_resume: reset clocksource, NTP
> 2) RTC resume: set xtime
> 
> Does this sound like a way forward?

I'm not sure I see how the last two groups are very different;
are you saying it would get rid of these message-unfriendly
read_persistent_clock() calls?

- Dave

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