[PATCH] fix RTC_CLASS regression with PARISC

[PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

As of 2.6.26, most distribution kernels for PARISC are coming with the
wrong RTC infrastructure enabled, meaning that userland can no longer
get at the RTC, so all our parisc clocks are drifting.

The fault is this patch:

commit c750090085f260503d8beec1c73c4d2e4fe93628
Author: David Brownell <david-b@pacbell.net>
Date:   Mon Apr 28 02:11:52 2008 -0700

    rtc: avoid legacy drivers with generic framework

Which makes drivers/rtc take precedence over the generic rtc.  However,
for parisc we only have the generic rtc, so in essence this turns off
our ability to access the rtc.  Put it back again by making RTC_CLASS
unselectable if the architecture is parisc.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 9a9755c..472fb19 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -8,7 +8,7 @@ config RTC_LIB
 menuconfig RTC_CLASS
 	tristate "Real Time Clock"
 	default n
-	depends on !S390
+	depends on !(S390 || PARISC)
 	select RTC_LIB
 	help
 	  Generic RTC class support. If you say yes here, you will

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, James Bottomley wrote:
> Put it back again by making RTC_CLASS 
> unselectable if the architecture is parisc.

Easier if those distros just wouldn't select RTC_CLASS then.  :)

And long term, better to work with RTC_CLASS.  Eliminate that
crufty asm-parisc/rtc.h file and one more GEN_RTC user; and
share more widely-used infrastructure.

If I read things right, that would be easy:  the PARISC RTC is
two firmware calls, ptc_tod_{read,set}(), which would map to
RTC class {read,set}_time() methods of about six lines each.
The RTC framework can do UIE emulation, if needed.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

On Mon, 2008-09-08 at 11:19 -0700, David Brownell wrote:
> On Monday 08 September 2008, James Bottomley wrote:
> > Put it back again by making RTC_CLASS 
> > unselectable if the architecture is parisc.
> 
> Easier if those distros just wouldn't select RTC_CLASS then.  :)

Yes, but think of distro config people rather like users ... if you can
prevent them from doing something stupid, it's a good idea.  In this
case, there's currently no way anyone should ever select RTC_CLASS on
parisc, so we should make that clear in the Kconfig file.

> And long term, better to work with RTC_CLASS.  Eliminate that
> crufty asm-parisc/rtc.h file and one more GEN_RTC user; and
> share more widely-used infrastructure.
> 
> If I read things right, that would be easy:  the PARISC RTC is
> two firmware calls, ptc_tod_{read,set}(), which would map to
> RTC class {read,set}_time() methods of about six lines each.
> The RTC framework can do UIE emulation, if needed.

OK, I can look at that, but in the mean time could we make the option
that causes the damage unselectable?  This is technically a regression
because before your patch GEN_RTC would override RTC_CLASS, now it's the
other way around.

James

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, James Bottomley wrote:
> On Mon, 2008-09-08 at 11:19 -0700, David Brownell wrote:
> > On Monday 08 September 2008, James Bottomley wrote:
> > > Put it back again by making RTC_CLASS 
> > > unselectable if the architecture is parisc.
> > 
> > Easier if those distros just wouldn't select RTC_CLASS then.  :)
> 
> Yes, but think of distro config people rather like users ... if you can
> prevent them from doing something stupid, it's a good idea.  In this
> case, there's currently no way anyone should ever select RTC_CLASS on
> parisc, so we should make that clear in the Kconfig file.

Preventing them from doing stupid stuff is exactly why we
have Kconfig prevent both legacy *AND* framework RTC code
from being selected.  :)

Of course stupidity is infinite, and we didn't know about
this particular instance in advance...


> > And long term, better to work with RTC_CLASS.  Eliminate that
> > crufty asm-parisc/rtc.h file and one more GEN_RTC user; and
> > share more widely-used infrastructure.
> > 
> > If I read things right, that would be easy:  the PARISC RTC is
> > two firmware calls, ptc_tod_{read,set}(), which would map to
> > RTC class {read,set}_time() methods of about six lines each.
> > The RTC framework can do UIE emulation, if needed.
> 
> OK, I can look at that, but in the mean time could we make the option
> that causes the damage unselectable?

I'd worry if "the mean time" takes too long.  But lacking a
PARISC laptop to fix this on, I'm unlikely to complain much.


>	 This is technically a regression 
> because before your patch GEN_RTC would override RTC_CLASS, now it's the
> other way around.

Well, previously there was no override ... I think you mean
that parisc just completely ignored RTC_CLASS, treating it
like junk DNA.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

On Mon, 2008-09-08 at 12:13 -0700, David Brownell wrote:
> On Monday 08 September 2008, James Bottomley wrote:
> > On Mon, 2008-09-08 at 11:19 -0700, David Brownell wrote:
> > > On Monday 08 September 2008, James Bottomley wrote:
> > > > Put it back again by making RTC_CLASS 
> > > > unselectable if the architecture is parisc.
> > > 
> > > Easier if those distros just wouldn't select RTC_CLASS then.  :)
> > 
> > Yes, but think of distro config people rather like users ... if you can
> > prevent them from doing something stupid, it's a good idea.  In this
> > case, there's currently no way anyone should ever select RTC_CLASS on
> > parisc, so we should make that clear in the Kconfig file.
> 
> Preventing them from doing stupid stuff is exactly why we
> have Kconfig prevent both legacy *AND* framework RTC code
> from being selected.  :)
> 
> Of course stupidity is infinite, and we didn't know about
> this particular instance in advance...
> 
> 
> > > And long term, better to work with RTC_CLASS.  Eliminate that
> > > crufty asm-parisc/rtc.h file and one more GEN_RTC user; and
> > > share more widely-used infrastructure.
> > > 
> > > If I read things right, that would be easy:  the PARISC RTC is
> > > two firmware calls, ptc_tod_{read,set}(), which would map to
> > > RTC class {read,set}_time() methods of about six lines each.
> > > The RTC framework can do UIE emulation, if needed.
> > 
> > OK, I can look at that, but in the mean time could we make the option
> > that causes the damage unselectable?
> 
> I'd worry if "the mean time" takes too long.  But lacking a
> PARISC laptop to fix this on, I'm unlikely to complain much.

What is the expectation?  If you're expecting all the architectures to
migrate over to RTC_CLASS, actually telling linux-arch and saying why
its a good idea would have been helpful.

All the PDC real time clock calls can do are read and set, nothing else,
so it's idealy suited to the GEN_RTC infrastructure ... what's the
benefit in moving it to RTC_CLASS?

> >	 This is technically a regression 
> > because before your patch GEN_RTC would override RTC_CLASS, now it's the
> > other way around.
> 
> Well, previously there was no override ... I think you mean
> that parisc just completely ignored RTC_CLASS, treating it
> like junk DNA.

No, it's a regression.  You made it so when you added this

#
# These legacy RTC drivers just cause too many conflicts with the
generic
# RTC framework ... let's not even try to coexist any more.
#
if RTC_LIB=n

Around the GEN_RTC configuration.  This turns off the ability to select
GEN_RTC if you've said yes to RTC_CLASS.  Since RTC_CLASS is currently
unsupported on parisc, we need to fix that by making the RTC_CLASS
option unselectable on parisc.

James

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, James Bottomley wrote:
> 
> > > OK, I can look at that, but in the mean time could we make the option
> > > that causes the damage unselectable?
> > 
> > I'd worry if "the mean time" takes too long.  But lacking a
> > PARISC laptop to fix this on, I'm unlikely to complain much.
> 
> What is the expectation?  If you're expecting all the architectures to
> migrate over to RTC_CLASS, actually telling linux-arch and saying why
> its a good idea would have been helpful.

Folk have been migrating already.  IMO there's no rush ... but
similarly, retrograde motion should be discouraged.  (Same issue
with essentially all legacy code in the tree.)


> All the PDC real time clock calls can do are read and set, nothing else,
> so it's idealy suited to the GEN_RTC infrastructure ... what's the
> benefit in moving it to RTC_CLASS?

The same benefit always found in sharing infrastructure.  Lots
of little differences/bugs go away.  Infrastructure improvements
and bugfixes get leveraged.  Dead and crufticious code can vanish.
And so forth.


> > >      This is technically a regression 
> > > because before your patch GEN_RTC would override RTC_CLASS, now it's the
> > > other way around.
> > 
> > Well, previously there was no override ... I think you mean
> > that parisc just completely ignored RTC_CLASS, treating it
> > like junk DNA.
> 
> No, it's a regression.  ...  This turns off the ability to select
> GEN_RTC if you've said yes to RTC_CLASS.  Since RTC_CLASS is currently
> unsupported on parisc, we need to fix that by making the RTC_CLASS
> option unselectable on parisc.

So you affirmed that there was no override, AND that it was
previously treated as junk DNA (ignored).  So just what were
you disagreeing with me about??

I have a hard time calling something a regression which
was never really a supported configuration.  And which
still *JUST WORKS* in those defconfigs ... given all that,
it's hard to argue that something is actually broken.

Kconfig is not about letting Aunt Tillie configure kernels
without being able to shoot herself in the foot.  That
discussion has been had (at length!) before.  Result, we
have a much better kernel config framework ... but still
don't facilitate "Kconfig-4-dummiez" audiences.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 8 Sep 2008 14:29:57 -0700

> On Monday 08 September 2008, James Bottomley wrote:
> > All the PDC real time clock calls can do are read and set, nothing else,
> > so it's idealy suited to the GEN_RTC infrastructure ... what's the
> > benefit in moving it to RTC_CLASS?
> 
> The same benefit always found in sharing infrastructure.  Lots
> of little differences/bugs go away.  Infrastructure improvements
> and bugfixes get leveraged.  Dead and crufticious code can vanish.
> And so forth.

I absolutely and positively agree with David here.

I just last week converted all of both sparc ports to the generic
RTC layer and what a huge burdon has been moved off of my shoulders.

The RTC layer is very nice and it even allows writing drivers for
very simplistic RTC devices (even ones that cannot be written)
with ease.  I had two such cases to handle on sparc64.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

On Mon, 2008-09-08 at 14:35 -0700, David Miller wrote:
> From: David Brownell <david-b@pacbell.net>
> Date: Mon, 8 Sep 2008 14:29:57 -0700
> 
> > On Monday 08 September 2008, James Bottomley wrote:
> > > All the PDC real time clock calls can do are read and set, nothing else,
> > > so it's idealy suited to the GEN_RTC infrastructure ... what's the
> > > benefit in moving it to RTC_CLASS?
> > 
> > The same benefit always found in sharing infrastructure.  Lots
> > of little differences/bugs go away.  Infrastructure improvements
> > and bugfixes get leveraged.  Dead and crufticious code can vanish.
> > And so forth.
> 
> I absolutely and positively agree with David here.
> 
> I just last week converted all of both sparc ports to the generic
> RTC layer and what a huge burdon has been moved off of my shoulders.
> 
> The RTC layer is very nice and it even allows writing drivers for
> very simplistic RTC devices (even ones that cannot be written)
> with ease.  I had two such cases to handle on sparc64.

I'm guessing they're not upstream yet (since I can't find them)?

However, if you based them on rtc-ppc.c then yes, I agree, it looks
reasonably easy:  it's just a matter of converting over the GEN_RTC
PDT_TOD helpers.

On a related note ... I could do this specifically for parisc, but I
could also do a GEN_RTC conversion to PDC_CLASS ... would that be more
helpful?

James

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 08 Sep 2008 18:00:47 -0500

> On Mon, 2008-09-08 at 14:35 -0700, David Miller wrote:
> > The RTC layer is very nice and it even allows writing drivers for
> > very simplistic RTC devices (even ones that cannot be written)
> > with ease.  I had two such cases to handle on sparc64.
> 
> I'm guessing they're not upstream yet (since I can't find them)?

It's in my sparc next tree:

	master.kernel.org:/pub/scm/linux/kernel/git/davem/sparc-next-2.6.git

> However, if you based them on rtc-ppc.c then yes, I agree, it looks
> reasonably easy:  it's just a matter of converting over the GEN_RTC
> PDT_TOD helpers.

That's not what I do, I use the real RAW chip drivers provided by the
RTC layer.

That's the way to do this.

I think the powerpc folks did the wrong thing and should just register
generic platform_device objects in their platform code, and let the
RTC layer drive the individual devices in response.

All the powerpc folks are doing is providing a dummy shim into the
RTC layer using their machine description vector, and not really using
the RTC layer drivers at all.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

On Mon, 2008-09-08 at 16:04 -0700, David Miller wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date: Mon, 08 Sep 2008 18:00:47 -0500
> 
> > On Mon, 2008-09-08 at 14:35 -0700, David Miller wrote:
> > > The RTC layer is very nice and it even allows writing drivers for
> > > very simplistic RTC devices (even ones that cannot be written)
> > > with ease.  I had two such cases to handle on sparc64.
> > 
> > I'm guessing they're not upstream yet (since I can't find them)?
> 
> It's in my sparc next tree:
> 
> 	master.kernel.org:/pub/scm/linux/kernel/git/davem/sparc-next-2.6.git
> 
> > However, if you based them on rtc-ppc.c then yes, I agree, it looks
> > reasonably easy:  it's just a matter of converting over the GEN_RTC
> > PDT_TOD helpers.
> 
> That's not what I do, I use the real RAW chip drivers provided by the
> RTC layer.
> 
> That's the way to do this.
> 
> I think the powerpc folks did the wrong thing and should just register
> generic platform_device objects in their platform code, and let the
> RTC layer drive the individual devices in response.
> 
> All the powerpc folks are doing is providing a dummy shim into the
> RTC layer using their machine description vector, and not really using
> the RTC layer drivers at all.

But realistically that's all we need.  Our RTC is controlled by two
calls into firmware: a get and a set; nothing else.  We don't have the
docs to get at the clock without the firmware calls.

James

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, James Bottomley wrote:
> 
> > All the powerpc folks are doing is providing a dummy shim into the
> > RTC layer using their machine description vector, and not really using
> > the RTC layer drivers at all.
> 
> But realistically that's all we need.  Our RTC is controlled by two
> calls into firmware: a get and a set; nothing else.  We don't have the
> docs to get at the clock without the firmware calls.

True for PARISC ... but not for PowerPC.  Lots of PowerPC
system boards use off-the-shelf RTCs with more capabilities
than the machine description vector acknowledges.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Mon, 08 Sep 2008 18:23:06 -0500

> But realistically that's all we need.  Our RTC is controlled by two
> calls into firmware: a get and a set; nothing else.  We don't have the
> docs to get at the clock without the firmware calls.

I wrote RTC layer drivers on sparc64 that make the firmware calls in
the Niagara system case, exactly like your case.

Really, this is the way to do this.

Since you're not reading what I implemented, I'll read it for you :-/

See drivers/rtc/rtc-sun4v.c, that's the driver

Code in arch/sparc64/kernel/time.c creates and registers the
appropriate platform_device object once the correct system type is
detected, and then rtc-sun4v.c attaches when such objects have been
registered.

Likewise for all the other RTC types that can be found on Sparc machines.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, David Miller wrote:
> I think the powerpc folks did the wrong thing and should just register
> generic platform_device objects in their platform code, and let the
> RTC layer drive the individual devices in response.

I kind of thought that was a migration aid ... 


> All the powerpc folks are doing is providing a dummy shim into the
> RTC layer using their machine description vector, and not really using
> the RTC layer drivers at all.

I basically agree.  There's functional overlap between those
machine descriptions and the RTC framework, and it should be
removed (by shrinking those descriptions).  The shim gets
/dev/rtcN support, and thus hwclock; also /sys/class/rtc/*
stuff.  But no wake alarms...


That said, there's a bit of unresolved stuff around NTP hooks
in the kernel.  Some patches are pending to let thtem work with
the RTC framework -- where writing an RTC may need to sleep,
for example because the RTC is on an I2C or SPI bus.  And
then there's the discussion of whether that shouldn't all be
handled by NTPD anyway, no special kernel support desired.
Alessandro has opinions there.  ;)

ISTR that was a factor in the powerpc taking that "sideways"
step.  Or if not powerpc, then some other arch.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 8 Sep 2008 16:29:20 -0700

> That said, there's a bit of unresolved stuff around NTP hooks
> in the kernel.  Some patches are pending to let thtem work with
> the RTC framework -- where writing an RTC may need to sleep,
> for example because the RTC is on an I2C or SPI bus.  And
> then there's the discussion of whether that shouldn't all be
> handled by NTPD anyway, no special kernel support desired.
> Alessandro has opinions there.  ;)

My update_persistent_clock() on sparc64 is:

int update_persistent_clock(struct timespec now)
{
	struct rtc_device *rtc = rtc_class_open("rtc0");

	if (rtc)
		return rtc_set_mmss(rtc, now.tv_sec);

	return -1;
}

and that should handle this NTP shouldn't it?

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, David Miller wrote:
> From: David Brownell <david-b@pacbell.net>
> Date: Mon, 8 Sep 2008 16:29:20 -0700
> 
> > That said, there's a bit of unresolved stuff around NTP hooks
> > in the kernel.  Some patches are pending to let thtem work with
> > the RTC framework -- where writing an RTC may need to sleep,
> > for example because the RTC is on an I2C or SPI bus.  And
> > then there's the discussion of whether that shouldn't all be
> > handled by NTPD anyway, no special kernel support desired.
> > Alessandro has opinions there.  ;)
> 
> My update_persistent_clock() on sparc64 is:
> 
> int update_persistent_clock(struct timespec now)
> {
> 	struct rtc_device *rtc = rtc_class_open("rtc0");

I'd be tempted to cache that ... notice how you never
close it, too.  That will goof lots of refcounts...


> 	if (rtc)
> 		return rtc_set_mmss(rtc, now.tv_sec);
> 
> 	return -1;
> }
> 
> and that should handle this NTP shouldn't it?

Depends on what patches have applied; I've lost track
of whether it's now ok for update_persistent_clock()
to sleep.  Previously it was not, without a critical
patch (appended).

Having something like that work is *certainly* a goal,
at least for those who don't want to get rid of those
kernel NTP hooks entirely.

And of course, once that works I'd claim it should live
in drivers/rtc for the benefit of other platforms.  :)

- Dave
 
=============== CUT ON THE DOTTED LINE ==================
Subject: ntp: let update_persistent_clock() sleep
From: "Maciej W. Rozycki" <macro@linux-mips.org>

This is a change that makes the 11-minute RTC update be run in the process
context.  This is so that update_persistent_clock() can sleep, which may
be required for certain types of RTC hardware -- most notably I2C devices.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Roman Zippel <zippel@linux-m68k.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: David Brownell <david-b@pacbell.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/time/ntp.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/kernel/time/ntp.c	2008-06-30 21:20:51.000000000 -0700
+++ b/kernel/time/ntp.c	2008-06-30 21:21:27.000000000 -0700
@@ -10,13 +10,13 @@
 
 #include <linux/mm.h>
 #include <linux/time.h>
-#include <linux/timer.h>
 #include <linux/timex.h>
 #include <linux/jiffies.h>
 #include <linux/hrtimer.h>
 #include <linux/capability.h>
 #include <linux/math64.h>
 #include <linux/clocksource.h>
+#include <linux/workqueue.h>
 #include <asm/timex.h>
 
 /*
@@ -218,11 +218,11 @@ void second_overflow(void)
 /* Disable the cmos update - used by virtualization and embedded */
 int no_sync_cmos_clock  __read_mostly;
 
-static void sync_cmos_clock(unsigned long dummy);
+static void sync_cmos_clock(struct work_struct *work);
 
-static DEFINE_TIMER(sync_cmos_timer, sync_cmos_clock, 0, 0);
+static DECLARE_DELAYED_WORK(sync_cmos_work, sync_cmos_clock);
 
-static void sync_cmos_clock(unsigned long dummy)
+static void sync_cmos_clock(struct work_struct *work)
 {
 	struct timespec now, next;
 	int fail = 1;
@@ -258,13 +258,13 @@ static void sync_cmos_clock(unsigned lon
 		next.tv_sec++;
 		next.tv_nsec -= NSEC_PER_SEC;
 	}
-	mod_timer(&sync_cmos_timer, jiffies + timespec_to_jiffies(&next));
+	schedule_delayed_work(&sync_cmos_work, timespec_to_jiffies(&next));
 }
 
 static void notify_cmos_timer(void)
 {
 	if (!no_sync_cmos_clock)
-		mod_timer(&sync_cmos_timer, jiffies + 1);
+		schedule_delayed_work(&sync_cmos_work, 0);
 }
 
 #else

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 8 Sep 2008 17:55:25 -0700

> On Monday 08 September 2008, David Miller wrote:
> > From: David Brownell <david-b@pacbell.net>
> > Date: Mon, 8 Sep 2008 16:29:20 -0700
> > 
> > > That said, there's a bit of unresolved stuff around NTP hooks
> > > in the kernel.  Some patches are pending to let thtem work with
> > > the RTC framework -- where writing an RTC may need to sleep,
> > > for example because the RTC is on an I2C or SPI bus.  And
> > > then there's the discussion of whether that shouldn't all be
> > > handled by NTPD anyway, no special kernel support desired.
> > > Alessandro has opinions there.  ;)
> > 
> > My update_persistent_clock() on sparc64 is:
> > 
> > int update_persistent_clock(struct timespec now)
> > {
> > 	struct rtc_device *rtc = rtc_class_open("rtc0");
> 
> I'd be tempted to cache that ... notice how you never
> close it, too.  That will goof lots of refcounts...

Well if I cache it then we'll hold it forever and that's not
so nice right?

I'm going to put the missing rtc_close() in there for now to
fix the leak.

I'm happy to cache this if you think it's warranted, but then
this is like saying that the refcount doesn't matter :-)

> =============== CUT ON THE DOTTED LINE ==================
> Subject: ntp: let update_persistent_clock() sleep
> From: "Maciej W. Rozycki" <macro@linux-mips.org>

I see, as Paul mentioned this is needed for stuff like RTCs
behind I2C.

This change isn't in Linus's tree yet.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, David Miller wrote:
> 
> > > int update_persistent_clock(struct timespec now)
> > > {
> > >     struct rtc_device *rtc = rtc_class_open("rtc0");

One more point:  that should probably use CONFIG_RTC_HCTOSYS_DEVICE
instead of hard-wiring to "rtc0".  Yeah, I'm sure your SPARCs have
lots of RTCs to choose from -- not! -- but I'd like to see you end
up with code that many folk can reuse/recycle/pirate.  ;)


> > 
> > I'd be tempted to cache that ... notice how you never
> > close it, too.  That will goof lots of refcounts...
> 
> Well if I cache it then we'll hold it forever and that's not
> so nice right?

Why wouldn't it be, so long as it's eventually closed
to prevent leakage?  Other code can rtc_class_open() too;
unlike a userspace open("/dev/rtc0", ...) this isn't an
exclusive operation.


> I'm going to put the missing rtc_close() in there for now to
> fix the leak.
> 
> I'm happy to cache this if you think it's warranted, but then
> this is like saying that the refcount doesn't matter :-)

If you're concerned about stuff like "rmmod my-i2c-rtc-driver"
losing (or "rmmod my-i2c-rtc-driver's-i2c-adapter") ... what's
supposed to happen is that you start getting an -ENODEV return
from your rtc_set_mmss() call, and then you close and null your
cached handle to free up its memory.

The next time you go through this routine you'd see nothing
is cached and then try to get an RTC.  Maybe it's available
again; maybe not.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Mon, 8 Sep 2008 20:17:23 -0700

> On Monday 08 September 2008, David Miller wrote:
> > 
> > > > int update_persistent_clock(struct timespec now)
> > > > {
> > > >     struct rtc_device *rtc = rtc_class_open("rtc0");
> 
> One more point:  that should probably use CONFIG_RTC_HCTOSYS_DEVICE
> instead of hard-wiring to "rtc0".  Yeah, I'm sure your SPARCs have
> lots of RTCs to choose from -- not! -- but I'd like to see you end
> up with code that many folk can reuse/recycle/pirate.  ;)

Can you be more specific?  Oh, you want me to use the string defined
by that config option.  Ok :-)

But as far as I can tell this will only be set of RTC_HCTOSYS and
users currently are allowed to not set that.

If this code goes somewhere generic you would need to ifdef test on
that, depending upon where you'd want to put it and how it would
be provided generically.

> > > 
> > > I'd be tempted to cache that ... notice how you never
> > > close it, too.  That will goof lots of refcounts...
> > 
> > Well if I cache it then we'll hold it forever and that's not
> > so nice right?
> 
> Why wouldn't it be, so long as it's eventually closed
> to prevent leakage?  Other code can rtc_class_open() too;
> unlike a userspace open("/dev/rtc0", ...) this isn't an
> exclusive operation.

When would be "eventually closed" if I open it here and remember
the pointer in a static local variable, and don't close it?

I guess you need to be more specific about what you mean by
caching :)

> If you're concerned about stuff like "rmmod my-i2c-rtc-driver"
> losing (or "rmmod my-i2c-rtc-driver's-i2c-adapter") ... what's
> supposed to happen is that you start getting an -ENODEV return
> from your rtc_set_mmss() call, and then you close and null your
> cached handle to free up its memory.

I see... god that's ugly.  If you want to do this in the generic
RTC layer helper routines, that's fine, but I don't feel like
adding all sorts of stuff like that to the sparc specific routine
at the moment.

I'm trying to do things that are practical and that I can check
into sparc-next-2.6 right now.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Monday 08 September 2008, David Miller wrote:
> > > > >     struct rtc_device *rtc = rtc_class_open("rtc0");
> > 
> > One more point:  that should probably use CONFIG_RTC_HCTOSYS_DEVICE
> > instead of hard-wiring to "rtc0".  Yeah, I'm sure your SPARCs have
> > lots of RTCs to choose from -- not! -- but I'd like to see you end
> > up with code that many folk can reuse/recycle/pirate.  ;)
> 
> Can you be more specific?  Oh, you want me to use the string defined
> by that config option.  Ok :-)
> 
> But as far as I can tell this will only be set of RTC_HCTOSYS and
> users currently are allowed to not set that.
> 
> If this code goes somewhere generic you would need to ifdef test on
> that, depending upon where you'd want to put it and how it would
> be provided generically.

OK.


> > > > I'd be tempted to cache that ... notice how you never
> > > > close it, too.  That will goof lots of refcounts...
> > > 
> > > Well if I cache it then we'll hold it forever and that's not
> > > so nice right?
> > 
> > Why wouldn't it be, so long as it's eventually closed
> > to prevent leakage?  Other code can rtc_class_open() too;
> > unlike a userspace open("/dev/rtc0", ...) this isn't an
> > exclusive operation.
> 
> When would be "eventually closed" if I open it here and remember
> the pointer in a static local variable, and don't close it?
> 
> I guess you need to be more specific about what you mean by
> caching :)

I'll translate that as "-ENOPATCH".  :)

It'd suffice to fire a timer every 15 minutes or so, and
close it if the NTP logic hasn't refreshed the clock since
last time.  You're right that the simplest scheme is to just
open/close on each call.  The extra work is so infrequent it
may not be measurable.


> > If you're concerned about stuff like "rmmod my-i2c-rtc-driver"
> > losing (or "rmmod my-i2c-rtc-driver's-i2c-adapter") ... what's
> > supposed to happen is that you start getting an -ENODEV return
> > from your rtc_set_mmss() call, and then you close and null your
> > cached handle to free up its memory.
> 
> I see... god that's ugly.  If you want to do this in the generic
> RTC layer helper routines,

... when they get created ...


> 	that's fine, but I don't feel like 
> adding all sorts of stuff like that to the sparc specific routine
> at the moment.
> 
> I'm trying to do things that are practical and that I can check
> into sparc-next-2.6 right now.

OK by me.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[Andrew Morton]

On Mon, 08 Sep 2008 19:52:35 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: David Brownell <david-b@pacbell.net>
> Date: Mon, 8 Sep 2008 17:55:25 -0700
> 
> > On Monday 08 September 2008, David Miller wrote:
> > > From: David Brownell <david-b@pacbell.net>
> > > Date: Mon, 8 Sep 2008 16:29:20 -0700
> > > 
> > > > That said, there's a bit of unresolved stuff around NTP hooks
> > > > in the kernel.  Some patches are pending to let thtem work with
> > > > the RTC framework -- where writing an RTC may need to sleep,
> > > > for example because the RTC is on an I2C or SPI bus.  And
> > > > then there's the discussion of whether that shouldn't all be
> > > > handled by NTPD anyway, no special kernel support desired.
> > > > Alessandro has opinions there.  ;)
> > > 
> > > My update_persistent_clock() on sparc64 is:
> > > 
> > > int update_persistent_clock(struct timespec now)
> > > {
> > > 	struct rtc_device *rtc = rtc_class_open("rtc0");
> > 
> > I'd be tempted to cache that ... notice how you never
> > close it, too.  That will goof lots of refcounts...
> 
> Well if I cache it then we'll hold it forever and that's not
> so nice right?
> 
> I'm going to put the missing rtc_close() in there for now to
> fix the leak.
> 
> I'm happy to cache this if you think it's warranted, but then
> this is like saying that the refcount doesn't matter :-)
> 
> > =============== CUT ON THE DOTTED LINE ==================
> > Subject: ntp: let update_persistent_clock() sleep
> > From: "Maciej W. Rozycki" <macro@linux-mips.org>
> 
> I see, as Paul mentioned this is needed for stuff like RTCs
> behind I2C.
> 
> This change isn't in Linus's tree yet.

Should it be?

Its current status is: stuck in -mm.  I've sent it to Thomas a couple
of times marked "for 2.6.27?" and he might have applied it now (I'm a
few days behind, waiting for linux-next to start up again).

It was not included in Thomas's recent mainline pull request:

From: Thomas Gleixner <tglx@tglx.de>
Subject: [GIT pull] timer updates for 2.6.27
Date: Tue, 9 Sep 2008 22:32:39 +0200 (CEST)

Re: [PATCH] fix RTC_CLASS regression with PARISC

[Randy.Dunlap]

On Wed, 10 Sep 2008, Andrew Morton wrote:

> On Mon, 08 Sep 2008 19:52:35 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: David Brownell <david-b@pacbell.net>
> > Date: Mon, 8 Sep 2008 17:55:25 -0700
> > 
> > > On Monday 08 September 2008, David Miller wrote:
> > > > From: David Brownell <david-b@pacbell.net>
> > > > Date: Mon, 8 Sep 2008 16:29:20 -0700
> > > > 
> > > > > That said, there's a bit of unresolved stuff around NTP hooks
> > > > > in the kernel.  Some patches are pending to let thtem work with
> > > > > the RTC framework -- where writing an RTC may need to sleep,
> > > > > for example because the RTC is on an I2C or SPI bus.  And
> > > > > then there's the discussion of whether that shouldn't all be
> > > > > handled by NTPD anyway, no special kernel support desired.
> > > > > Alessandro has opinions there.  ;)
> > > > 
> > > > My update_persistent_clock() on sparc64 is:
> > > > 
> > > > int update_persistent_clock(struct timespec now)
> > > > {
> > > > 	struct rtc_device *rtc = rtc_class_open("rtc0");
> > > 
> > > I'd be tempted to cache that ... notice how you never
> > > close it, too.  That will goof lots of refcounts...
> > 
> > Well if I cache it then we'll hold it forever and that's not
> > so nice right?
> > 
> > I'm going to put the missing rtc_close() in there for now to
> > fix the leak.
> > 
> > I'm happy to cache this if you think it's warranted, but then
> > this is like saying that the refcount doesn't matter :-)
> > 
> > > =============== CUT ON THE DOTTED LINE ==================
> > > Subject: ntp: let update_persistent_clock() sleep
> > > From: "Maciej W. Rozycki" <macro@linux-mips.org>
> > 
> > I see, as Paul mentioned this is needed for stuff like RTCs
> > behind I2C.
> > 
> > This change isn't in Linus's tree yet.
> 
> Should it be?
> 
> Its current status is: stuck in -mm.  I've sent it to Thomas a couple
> of times marked "for 2.6.27?" and he might have applied it now (I'm a
> few days behind, waiting for linux-next to start up again).

This is something that you can attempt (again) to address next week along
with other process issues...

Maybe travel time/delay is also involved (for people other than Mr. Rothwell).


> It was not included in Thomas's recent mainline pull request:
> 
> From: Thomas Gleixner <tglx@tglx.de>
> Subject: [GIT pull] timer updates for 2.6.27
> Date: Tue, 9 Sep 2008 22:32:39 +0200 (CEST)

-- 
~Randy

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Wednesday 10 September 2008, Randy.Dunlap wrote:
> 
> > > > =============== CUT ON THE DOTTED LINE ==================
> > > > Subject: ntp: let update_persistent_clock() sleep
> > > > From: "Maciej W. Rozycki" <macro@linux-mips.org>
> > > 
> > > I see, as Paul mentioned this is needed for stuff like RTCs
> > > behind I2C.
> > > 
> > > This change isn't in Linus's tree yet.
> > 
> > Should it be?

IMO if it doesn't make 2.6.27, it should merge for 2.6.28 early-ish.

 
> > Its current status is: stuck in -mm.  I've sent it to Thomas a couple
> > of times marked "for 2.6.27?" and he might have applied it now (I'm a
> > few days behind, waiting for linux-next to start up again).
> 
> This is something that you can attempt (again) to address next week along
> with other process issues...

I don't think anyone in particular has been _pushing_ to resolve these
NTP related issues ... lack of urgency.  Maybe it's fair to think of
that as a process issue.  If DaveM wants SPARC64 to completely remove
its legacy RTC support for some release (which?) then:  (a) great! and
(b) that should be sufficient urgency.

There was, I think, another patch related to this one.  Then there may
be a few other loose ends to tie up also.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Wed, 10 Sep 2008 14:19:04 -0700

> On Wednesday 10 September 2008, Randy.Dunlap wrote:
> > 
> > > > > =============== CUT ON THE DOTTED LINE ==================
> > > > > Subject: ntp: let update_persistent_clock() sleep
> > > > > From: "Maciej W. Rozycki" <macro@linux-mips.org>
> > > > 
> > > > I see, as Paul mentioned this is needed for stuff like RTCs
> > > > behind I2C.
> > > > 
> > > > This change isn't in Linus's tree yet.
> > > 
> > > Should it be?
> 
> IMO if it doesn't make 2.6.27, it should merge for 2.6.28 early-ish.
> 
>  
> > > Its current status is: stuck in -mm.  I've sent it to Thomas a couple
> > > of times marked "for 2.6.27?" and he might have applied it now (I'm a
> > > few days behind, waiting for linux-next to start up again).
> > 
> > This is something that you can attempt (again) to address next week along
> > with other process issues...
> 
> I don't think anyone in particular has been _pushing_ to resolve these
> NTP related issues ... lack of urgency.  Maybe it's fair to think of
> that as a process issue.  If DaveM wants SPARC64 to completely remove
> its legacy RTC support for some release (which?) then:  (a) great! and
> (b) that should be sufficient urgency.

Well, firstly my RTC sparc work is 2.6.28 targetted.

Secondly, that sleeping capability is only really needed for I2C based
RTC chips, of which sparc isn't currently making any use of.

PowerPC folks do, however, use I2C based RTC chips right now I thought?

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell]

On Wednesday 10 September 2008, David Miller wrote:
> > 
> > I don't think anyone in particular has been _pushing_ to resolve these
> > NTP related issues ... lack of urgency.  Maybe it's fair to think of
> > that as a process issue.  If DaveM wants SPARC64 to completely remove
> > its legacy RTC support for some release (which?) then:  (a) great! and
> > (b) that should be sufficient urgency.
> 
> Well, firstly my RTC sparc work is 2.6.28 targetted.

Fine; Andrew and Thomas won't need to worry about this patch
for now, then.


> Secondly, that sleeping capability is only really needed for I2C based
> RTC chips, of which sparc isn't currently making any use of.

Not quite right.  The RTC calls you make grabs a mutex; *ALL* calls
into the framework are made in task context.  Even if the underlying
RTC happens to be the type with registers on the CPU bus, where
there's no need to sleep when reading values.

Of course, you're very unlikely to sleep while waiting for that
mutex; or even while holding it, if you stick to Sun hardware.
Someone running Linux on one of the FPGA-based SPARCs might well
use an I2C based RTC though.

- Dave

Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Miller]

From: David Brownell <david-b@pacbell.net>
Date: Wed, 10 Sep 2008 14:36:25 -0700

> Of course, you're very unlikely to sleep while waiting for that
> mutex; or even while holding it, if you stick to Sun hardware.
> Someone running Linux on one of the FPGA-based SPARCs might well
> use an I2C based RTC though.

There are at least one or two sparc workstations and/or servers that
use an I2C based RTC, I just haven't written the driver yet :)

Re: [PATCH] fix RTC_CLASS regression with PARISC

[Paul Mackerras]

David Miller writes:

> int update_persistent_clock(struct timespec now)
> {
> 	struct rtc_device *rtc = rtc_class_open("rtc0");
> 
> 	if (rtc)
> 		return rtc_set_mmss(rtc, now.tv_sec);
> 
> 	return -1;
> }
> 
> and that should handle this NTP shouldn't it?

Yes as long as your rtc set functions don't need to sleep, since
update_persistent_clock is called at interrupt level.  Some powerpc
systems have their RTC at the far end of an I2C bus, and the I2C
access routines can sleep.

Paul.

Re: [PATCH] fix RTC_CLASS regression with PARISC

[James Bottomley]

On Mon, 2008-09-08 at 14:29 -0700, David Brownell wrote:
> > No, it's a regression.  ...  This turns off the ability to select
> > GEN_RTC if you've said yes to RTC_CLASS.  Since RTC_CLASS is currently
> > unsupported on parisc, we need to fix that by making the RTC_CLASS
> > option unselectable on parisc.
> 
> So you affirmed that there was no override, AND that it was
> previously treated as junk DNA (ignored).  So just what were
> you disagreeing with me about??

The fact that the override you put in to disable GEN_RTC is causing a
regression.

> I have a hard time calling something a regression which
> was never really a supported configuration.  And which
> still *JUST WORKS* in those defconfigs ... given all that,
> it's hard to argue that something is actually broken.

On 2.6.25 the parisc users got a working realtime clock.  On 2.6.26 they
can find themselves without one using the same .config file.  This is
the regression.  The fix is to disable RTC_CLASS until we have it
working for parisc.

> Kconfig is not about letting Aunt Tillie configure kernels
> without being able to shoot herself in the foot.  That
> discussion has been had (at length!) before.  Result, we
> have a much better kernel config framework ... but still
> don't facilitate "Kconfig-4-dummiez" audiences.

Kconfig is complex enough that we try not to trip users up by hiding
things, especially when it's the wrong thing to hide on this platform.
If an option specifically doesn't work or causes harm on a platform, it
shouldn't be selectable on that platform.  This isn't Aunt Tillieism,
this is called common sense.

James