fix RTC_CLASS regression with PARISC
PATCH 1/1 fix RTC_CLASS regression with PARISC
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 <[email protected]> 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.
---
drivers/rtc/Kconfig +1 -1
@@ -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
Quoted 2 lines · James Bottomley
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
Quoted 5 lines · David Brownell
Quoted 2 lines · James Bottomley
Put it back again by making
RTC_CLASS
unselectable if the architecture is parisc.Easier if those distros just wouldn't select
RTC_CLASSthen. :)
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.
Quoted 8 lines
And long term, better to work with
RTC_CLASS. Eliminate that cruftyasm-parisc/rtc.hfile and one moreGEN_RTCuser; 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
Quoted 11 lines · James Bottomley
Quoted 5 lines · David Brownell
Quoted 2 lines · James Bottomley
Put it back again by making
RTC_CLASS
unselectable if the architecture is parisc.Easier if those distros just wouldn't select
RTC_CLASSthen. :)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_CLASSon parisc, so we should make that clear in theKconfigfile.
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...
Quoted 11 lines
Quoted 8 lines
And long term, better to work with
RTC_CLASS. Eliminate that cruftyasm-parisc/rtc.hfile and one moreGEN_RTCuser; 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.
Quoted 3 lines
This is technically a regression
because before your patchGEN_RTCwould overrideRTC_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
Quoted 30 lines · David Brownell
Quoted 11 lines · James Bottomley
Quoted 5 lines · David Brownell
Quoted 2 lines · James Bottomley
Put it back again by making
RTC_CLASS
unselectable if the architecture is parisc.Easier if those distros just wouldn't select
RTC_CLASSthen. :)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_CLASSon parisc, so we should make that clear in theKconfigfile.Preventing them from doing stupid stuff is exactly why we have
Kconfigprevent 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...
Quoted 9 lines
Quoted 8 lines
And long term, better to work with
RTC_CLASS. Eliminate that cruftyasm-parisc/rtc.hfile and one moreGEN_RTCuser; 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.
Quoted 5 lines
Quoted 2 lines
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?
Quoted 7 lines
Quoted 3 lines
This is technically a regression
because before your patchGEN_RTCwould overrideRTC_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
Quoted 2 lines · James Bottomley
=20
Quoted 1 line
Quoted 1 line
OK, I can look at that, but in the mean time could we make the op=
tion
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
that causes the damage unselectable?
=20
I'd worry if "the mean time" takes too long. =A0But lacking a PARISC laptop to fix this on, I'm unlikely to complain much.=20
What is the expectation? =A0If you're expecting all the architectures=
to
Quoted 2 lines
migrate over to
RTC_CLASS, actually telling linux-arch and saying why its a good idea would have been helpful.
=46olk 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.)
Quoted 1 line
All the PDC real time clock calls can do are read and set, nothing el=
se,
Quoted 2 lines
so it's idealy suited to the
GEN_RTCinfrastructure ... what's the benefit in moving it toRTC_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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
=A0=A0=A0=A0=A0 This is technically a regression=20 because before your patch
GEN_RTCwould overrideRTC_CLASS, now i=
t's the
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
other way around.
=20
Well, previously there was no override ... I think you mean that parisc just completely ignoredRTC_CLASS, treating it like junk DNA.=20
No, it's a regression. =A0... =A0This turns off the ability to selectGEN_RTCif you've said yes toRTC_CLASS. =A0SinceRTC_CLASSis curren=
tly
Quoted 2 lines
unsupported on parisc, we need to fix that by making the
RTC_CLASSoption 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
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 14:29:57 -0700
Quoted 9 lines · James Bottomley
Quoted 3 lines
All the PDC real time clock calls can do are read and set, nothing else, so it's idealy suited to the
GEN_RTCinfrastructure ... what's the benefit in moving it toRTC_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.
Quoted 13 lines · David Miller
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 14:29:57 -0700Quoted 9 lines · James Bottomley
Quoted 3 lines
All the PDC real time clock calls can do are read and set, nothing else, so it's idealy suited to the
GEN_RTCinfrastructure ... what's the benefit in moving it toRTC_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.
Quoted 8 lines
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
From: James Bottomley <[email protected]>
Date: Mon, 08 Sep 2008 18:00:47 -0500
Quoted 6 lines · David Miller
Quoted 3 lines
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
Quoted 3 lines
However, if you based them on
rtc-ppc.cthen yes, I agree, it looks reasonably easy: it's just a matter of converting over theGEN_RTCPDT_TODhelpers.
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.
Quoted 23 lines · David Miller
From: James Bottomley <[email protected]>
Date: Mon, 08 Sep 2008 18:00:47 -0500Quoted 6 lines · David Miller
Quoted 3 lines
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
Quoted 3 lines
However, if you based them on
rtc-ppc.cthen yes, I agree, it looks reasonably easy: it's just a matter of converting over theGEN_RTCPDT_TODhelpers.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.
Quoted 7 lines
I think the powerpc folks did the wrong thing and should just register generic
platform_deviceobjects 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
Quoted 3 lines · James Bottomley
=20
Quoted 2 lines
All the powerpc folks are doing is providing a dummy shim into the RTC layer using their machine description vector, and not really us=
ing
Quoted 4 lines
Quoted 1 line
the RTC layer drivers at all.
=20
But realistically that's all we need. =A0Our RTC is controlled by two calls into firmware: a get and a set; nothing else. =A0We don't have =
the
Quoted 1 line
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
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: James Bottomley <[email protected]>
Date: Mon, 08 Sep 2008 18:23:06 -0500
Quoted 3 lines
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.
Quoted 3 lines · David Miller
I think the powerpc folks did the wrong thing and should just register generic
platform_deviceobjects in their platform code, and let the RTC layer drive the individual devices in response.
I kind of thought that was a migration aid ...
Quoted 3 lines
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
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 16:29:20 -0700
Quoted 7 lines
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?
Quoted 11 lines · David Miller
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 16:29:20 -0700Quoted 7 lines
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. ;)
Quoted 5 lines
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...
Quoted 7 lines
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" <[email protected]>
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.
--- kernel/time/ntp.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/time/ntp.c +6 -6
@@ -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
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 17:55:25 -0700
Quoted 12 lines · David Miller
Quoted 11 lines
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 16:29:20 -0700Quoted 7 lines
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. ;)
Quoted 8 lines
Quoted 5 lines
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 :-)
Quoted 3 lines
=============== CUT ON THE DOTTED LINE ==================
Subject: ntp: letupdate_persistent_clock()sleep
From: "Maciej W. Rozycki" <[email protected]>
I see, as Paul mentioned this is needed for stuff like RTCs behind I2C.
This change isn't in Linus's tree yet.
Quoted 4 lines · David Miller
=20
Quoted 3 lines
Quoted 3 lines
int
update_persistent_clock(struct timespec now)
{
=A0=A0=A0=A0structrtc_device*rtc =3Drtc_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. ;)
Quoted 6 lines
Quoted 3 lines
=20
I'd be tempted to cache that ... notice how you never close it, too. =A0That will goof lots of refcounts...=20
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.
Quoted 5 lines
I'm going to put the missing
rtc_close()in there for now to fix the leak.
=20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
=46rom: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 20:17:23 -0700
Quoted 10 lines · David Miller
Quoted 4 lines
=20
Quoted 3 lines
Quoted 3 lines
int
update_persistent_clock(struct timespec now)
{
=A0=A0=A0=A0structrtc_device*rtc =3Drtc_class_open("rtc0");=20
One more point: that should probably useCONFIG_RTC_HCTOSYS_DEVICEinstead 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.
Quoted 11 lines
Quoted 6 lines
Quoted 3 lines
=20
I'd be tempted to cache that ... notice how you never close it, too. =A0That will goof lots of refcounts...=20
Well if I cache it then we'll hold it forever and that's not so nice right?=20
Why wouldn't it be, so long as it's eventually closed to prevent leakage? Other code canrtc_class_open()too; unlike a userspaceopen("/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 :)
Quoted 5 lines
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
-ENODEVreturn from yourrtc_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line · David Miller
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
=A0=A0=A0=A0struct
rtc_device*rtc =3D rtc_class_open("rtc0")=
;
Quoted 5 lines
Quoted 5 lines
=20
One more point: that should probably useCONFIG_RTC_HCTOSYS_DEVICEinstead 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. ;)
Quoted 10 lines
=20
Can you be more specific? Oh, you want me to use the string defined by that config option. Ok :-)
=20
But as far as I can tell this will only be set ofRTC_HCTOSYSand users currently are allowed to not set that.
=20
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.
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 2 lines
I'd be tempted to cache that ... notice how you never close it, too. =A0That will goof lots of refcounts...
=20
Well if I cache it then we'll hold it forever and that's not so nice right?=20
Why wouldn't it be, so long as it's eventually closed to prevent leakage? Other code canrtc_class_open()too; unlike a userspaceopen("/dev/rtc0", ...)this isn't an exclusive operation.
Quoted 6 lines
=20
When would be "eventually closed" if I open it here and remember the pointer in a static local variable, and don't close it? =20
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.
Quoted 8 lines
Quoted 5 lines
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
-ENODEVreturn from yourrtc_set_mmss()call, and then you close and null your cached handle to free up its memory.=20
I see... god that's ugly. If you want to do this in the generic RTC layer helper routines,
=2E.. when they get created ...
Quoted 6 lines
that's fine, but I don't feel like=20
adding all sorts of stuff like that to the sparc specific routine at the moment.
=20
I'm trying to do things that are practical and that I can check into sparc-next-2.6 right now.
OK by me.
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 33 lines · On Mon, 08 Sep 2008 19:52:35 -0700 (PDT) David Miller <[email protected]>…
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 17:55:25 -0700Quoted 12 lines · David Miller
Quoted 11 lines
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 16:29:20 -0700Quoted 7 lines
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. ;)
Quoted 8 lines
Quoted 5 lines
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 :-)
Quoted 8 lines
Quoted 3 lines
=============== CUT ON THE DOTTED LINE ==================
Subject: ntp: letupdate_persistent_clock()sleep
From: "Maciej W. Rozycki" <[email protected]>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 <[email protected]>
Subject: [GIT pull] timer updates for 2.6.27
Date: Tue, 9 Sep 2008 22:32:39 +0200 (CEST)
Quoted 43 lines · Andrew Morton
Quoted 40 lines · On Mon, 08 Sep 2008 19:52:35 -0700 (PDT) David Miller <[email protected]>…
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 17:55:25 -0700Quoted 12 lines · David Miller
Quoted 11 lines
From: David Brownell <[email protected]>
Date: Mon, 8 Sep 2008 16:29:20 -0700Quoted 7 lines
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. ;)
Quoted 8 lines
Quoted 5 lines
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 :-)
Quoted 3 lines
=============== CUT ON THE DOTTED LINE ==================
Subject: ntp: letupdate_persistent_clock()sleep
From: "Maciej W. Rozycki" <[email protected]>I see, as Paul mentioned this is needed for stuff like RTCs behind I2C.
Quoted 7 lines
Quoted 1 line
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).
Quoted 5 lines
It was not included in Thomas's recent mainline pull request:
From: Thomas Gleixner <[email protected]>
Subject: [GIT pull] timer updates for 2.6.27
Date: Tue, 9 Sep 2008 22:32:39 +0200 (CEST)
Signature · 1 line
~Randy
Quoted 2 lines · Randy.Dunlap
=20
Quoted 1 line
Quoted 1 line
Quoted 1 line
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D CUT ON THE DOTTED=
LINE =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 2 lines
Subject: ntp: let
update_persistent_clock()sleep
From: "Maciej W. Rozycki" <[email protected]>=20
I see, as Paul mentioned this is needed for stuff like RTCs behind I2C.
=20
This change isn't in Linus's tree yet.=20
Should it be?
IMO if it doesn't make 2.6.27, it should merge for 2.6.28 early-ish.
=20
Quoted 1 line
Quoted 1 line
Its current status is: stuck in -mm. =A0I've sent it to Thomas a co=
uple
Quoted 1 line
Quoted 1 line
of times marked "for 2.6.27?" and he might have applied it now (I'm=
a
Quoted 3 lines
Quoted 1 line
few days behind, waiting for linux-next to start up again).
=20
This is something that you can attempt (again) to address next week a=
long
Quoted 1 line
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
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
=46rom: David Brownell <[email protected]>
Date: Wed, 10 Sep 2008 14:19:04 -0700
Quoted 3 lines · Randy.Dunlap
Quoted 2 lines
=20
Quoted 1 line
Quoted 1 line
Quoted 1 line
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D CUT ON THE DOTT=
ED LINE =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Subject: ntp: let
update_persistent_clock()sleep
From: "Maciej W. Rozycki" <[email protected]>=20
I see, as Paul mentioned this is needed for stuff like RTCs behind I2C.
=20
This change isn't in Linus's tree yet.
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
=20
Should it be?=20
IMO if it doesn't make 2.6.27, it should merge for 2.6.28 early-ish. =20
=20Quoted 1 line
Quoted 1 line
Its current status is: stuck in -mm. =A0I've sent it to Thomas a =
couple
Quoted 1 line
Quoted 1 line
Quoted 1 line
of times marked "for 2.6.27?" and he might have applied it now (I=
'm a
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
few days behind, waiting for linux-next to start up again).
=20
This is something that you can attempt (again) to address next week=
along
Quoted 3 lines
Quoted 1 line
with other process issues...
=20
I don't think anyone in particular has been _pushing_ to resolve thes=
e
Quoted 3 lines
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! an=
d
Quoted 1 line
(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? --
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 2 lines · David Miller
Quoted 2 lines
=20
I don't think anyone in particular has been _pushing_ to resolve th=
ese
Quoted 1 line
Quoted 1 line
NTP related issues ... lack of urgency. =A0Maybe it's fair to think=
of
Quoted 1 line
Quoted 1 line
that as a process issue. =A0If DaveM wants SPARC64 to completely re=
move
Quoted 1 line
Quoted 1 line
its legacy RTC support for some release (which?) then: =A0(a) great=
! and
Quoted 3 lines
Quoted 1 line
(b) that should be sufficient urgency.
=20
Well, firstly my RTC sparc work is 2.6.28 targetted.
=46ine; Andrew and Thomas won't need to worry about this patch for now, then.
Quoted 1 line
Secondly, that sleeping capability is only really needed for I2C base=
d
Quoted 1 line
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
--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Brownell <[email protected]>
Date: Wed, 10 Sep 2008 14:36:25 -0700
Quoted 4 lines
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 :)
Quoted 11 lines · David Miller
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.
Quoted 8 lines · David Brownell
Quoted 4 lines
No, it's a regression. ... This turns off the ability to select
GEN_RTCif you've said yes toRTC_CLASS. SinceRTC_CLASSis currently unsupported on parisc, we need to fix that by making theRTC_CLASSoption 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.
Quoted 4 lines
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.
Quoted 5 lines
Kconfigis 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