| Message ID | 1417089760-26848-4-git-send-email-pang.xunlei@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
On Thu, 27 Nov 2014, Xunlei Pang wrote: > +/* Rtc epoch year, can be overrided by command line */ > +static unsigned int rtc_epoch = 1970; > +time64_t rtc_epoch_secs_since_1970; > + > +static int __init > +set_rtc_epoch(char *str) One line please > +{ > + unsigned int epoch; > + > + if (kstrtou32(str, 0, &epoch) != 0) { > + printk(KERN_WARNING "Invalid setup epoch %u!\n", epoch); Sure that makes a lot of sense to print epoch if the string conversion failed. epoch is going to be either uninitialized stack value or 0 depending on the kstrtou32 implementation. > + if (rtc_epoch < 1970) > + printk(KERN_WARNING "Epoch %u is smaller than 1970!\n", epoch); pr_warn() .... +__setup("rtc_epoch=", set_rtc_epoch); Not documented in Documentation/kernel-parameters.txt ... > /* > + * Convert seconds since rtc epoch to seconds since Unix epoch. > + */ > +time64_t rtc_hw32_to_time64(u32 hwtime) > +{ > + return (rtc_epoch_secs_since_1970 + hwtime); > +} > +EXPORT_SYMBOL_GPL(rtc_hw32_to_time64); So we need a exported function to add a global variable and a function argument? inline ? > +/* > + * Convert seconds since Unix epoch to seconds since rtc epoch. > + */ > +int rtc_time64_to_hw32(time64_t time, u32 *hwtime) > +{ > + /* time must be after rtc epoch */ > + if (time < rtc_epoch_secs_since_1970) > + return -EINVAL; > + > + /* rtc epoch may be too small? */ > + if (time - rtc_epoch_secs_since_1970 > UINT_MAX) > + return -EOVERFLOW; And what's the caller supposed to do with that information? > + *hwtime = time - rtc_epoch_secs_since_1970; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(rtc_time64_to_hw32); How is that going to be useful w/o a mechanism to adjust the epoch offset at runtime by any means? Thanks, tglx
On 28 November 2014 at 07:16, Thomas Gleixner <tglx@linutronix.de> wrote: > On Thu, 27 Nov 2014, Xunlei Pang wrote: >> +/* Rtc epoch year, can be overrided by command line */ >> +static unsigned int rtc_epoch = 1970; >> +time64_t rtc_epoch_secs_since_1970; >> + >> +static int __init >> +set_rtc_epoch(char *str) > > One line please > >> +{ >> + unsigned int epoch; >> + >> + if (kstrtou32(str, 0, &epoch) != 0) { >> + printk(KERN_WARNING "Invalid setup epoch %u!\n", epoch); > > Sure that makes a lot of sense to print epoch if the string conversion > failed. epoch is going to be either uninitialized stack value or 0 > depending on the kstrtou32 implementation. > >> + if (rtc_epoch < 1970) >> + printk(KERN_WARNING "Epoch %u is smaller than 1970!\n", epoch); > > pr_warn() .... > > +__setup("rtc_epoch=", set_rtc_epoch); > > Not documented in Documentation/kernel-parameters.txt ... > >> /* >> + * Convert seconds since rtc epoch to seconds since Unix epoch. >> + */ >> +time64_t rtc_hw32_to_time64(u32 hwtime) >> +{ >> + return (rtc_epoch_secs_since_1970 + hwtime); >> +} >> +EXPORT_SYMBOL_GPL(rtc_hw32_to_time64); > > So we need a exported function to add a global variable and a function > argument? inline ? > >> +/* >> + * Convert seconds since Unix epoch to seconds since rtc epoch. >> + */ >> +int rtc_time64_to_hw32(time64_t time, u32 *hwtime) >> +{ >> + /* time must be after rtc epoch */ >> + if (time < rtc_epoch_secs_since_1970) >> + return -EINVAL; >> + >> + /* rtc epoch may be too small? */ >> + if (time - rtc_epoch_secs_since_1970 > UINT_MAX) >> + return -EOVERFLOW; > > And what's the caller supposed to do with that information? This probably means the current rtc hardware has pushed its envelope now, the users should rely on the rtc_epoch command line to continue use it. > >> + *hwtime = time - rtc_epoch_secs_since_1970; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(rtc_time64_to_hw32); > > How is that going to be useful w/o a mechanism to adjust the epoch > offset at runtime by any means? If the rtc epoch changes at runtime, we'll not get the right time once the system is rebooting. So, it can only be altered throught command line, and must stay invariable ever since unless it uses NTP or something. Thanks, Xunlei > > Thanks, > > tglx > >
On Sat, 29 Nov 2014, pang.xunlei wrote: > On 28 November 2014 at 07:16, Thomas Gleixner <tglx@linutronix.de> wrote: > > How is that going to be useful w/o a mechanism to adjust the epoch > > offset at runtime by any means? > > If the rtc epoch changes at runtime, we'll not get the right time once > the system is rebooting. > So, it can only be altered throught command line, and must stay > invariable ever since unless it uses NTP or something. You are imposing that any machine which needs to handle this must be rebooted in order to get support for the HW wrap around of the 32bit seconds counter register. That's wrong to begin with, simply because you would have to update the command line and reboot all those machines a few milliseconds before the wrap around occurs. Not very practicable, right? Your argument that we wont get the proper time once the system is rebooting is moot, as in any case there needs to be some persistant storage which must be updated either to hold the offset itself or the kernel command line. So we want a mechanism to apply that change at runtime by whatever mechanism. And that can be an ad hoc change where we update the offset and the counter value in one go or a simple store operation which gets effective when the counter actually wraps around. That needs a lot more thought so I think we should drop that whole offset magic for now and concentrate on the far more pressing problem of time_t which affects all machines in about 24 years. Once we have done that we can revisit the 2106 problem and design a well thought out mechanism to handle it and not some cobbled together ad hoc solution. That said, I'm not too happy about your comments copied all over the place. We rather have something which can be easily grepped for, does not lose context and can be in the best case runtime evaluated. The simplest solution for this is to do the following: struct rtc_class_ops { ... int (*set_mmss)(struct device *, unsigned long secs); + int (*set_mmss64)(struct device *, time64_t secs); So any RTC which is capable of handling time past 2106 will be converted to use the new interface and leave the set_mmss callback NULL. The device drivers which cannot be converted to handle anything past 2106 due to hardware limitations do not need to be touched at all. Though we might to consider changing set_mmss() to - int (*set_mmss)(struct device *, unsigned long secs); + int (*set_mmss32(struct device *, u32 secs); but that can be done when the bulk has been converted to set_mmss64. That's a single coccinelle patch w/o adding randomly typoed comments to the wrong places. You can find that nicely with grep, you can detect it compile time by removing the set_mmss callback and you can evaluate it at runtime. Once we have a proper solution for the 2106 issue we have a clear indicator which of the drivers needs that treatment and which ones do not. We just can deduce it from the set_mmss callback assignment. Thanks, tglx
On 2 December 2014 at 05:12, Thomas Gleixner <tglx@linutronix.de> wrote: > On Sat, 29 Nov 2014, pang.xunlei wrote: > > On 28 November 2014 at 07:16, Thomas Gleixner <tglx@linutronix.de> > wrote: > > > How is that going to be useful w/o a mechanism to adjust the epoch > > > offset at runtime by any means? > > > > If the rtc epoch changes at runtime, we'll not get the right time once > > the system is rebooting. > > So, it can only be altered throught command line, and must stay > > invariable ever since unless it uses NTP or something. > > You are imposing that any machine which needs to handle this must be > rebooted in order to get support for the HW wrap around of the 32bit > seconds counter register. > > That's wrong to begin with, simply because you would have to update > the command line and reboot all those machines a few milliseconds > before the wrap around occurs. Not very practicable, right? > > Your argument that we wont get the proper time once the > system is rebooting is moot, as in any case there needs to be some > persistant storage which must be updated either to hold the offset > itself or the kernel command line. > > So we want a mechanism to apply that change at runtime by whatever > mechanism. And that can be an ad hoc change where we update the offset > and the counter value in one go or a simple store operation which gets > effective when the counter actually wraps around. > > That needs a lot more thought so I think we should drop that whole > offset magic for now and concentrate on the far more pressing problem > of time_t which affects all machines in about 24 years. > > Once we have done that we can revisit the 2106 problem and design a > well thought out mechanism to handle it and not some cobbled together > ad hoc solution. > > That said, I'm not too happy about your comments copied all over the > place. We rather have something which can be easily grepped for, does > not lose context and can be in the best case runtime evaluated. > > The simplest solution for this is to do the following: > > struct rtc_class_ops { > ... > int (*set_mmss)(struct device *, unsigned long secs); > + int (*set_mmss64)(struct device *, time64_t secs); > > So any RTC which is capable of handling time past 2106 will be > converted to use the new interface and leave the set_mmss callback > NULL. The device drivers which cannot be converted to handle anything > past 2106 due to hardware limitations do not need to be touched at > all. > > Though we might to consider changing set_mmss() to > - int (*set_mmss)(struct device *, unsigned long secs); > + int (*set_mmss32(struct device *, u32 secs); > > but that can be done when the bulk has been converted to > set_mmss64. That's a single coccinelle patch w/o adding randomly > typoed comments to the wrong places. > > You can find that nicely with grep, you can detect it compile time by > removing the set_mmss callback and you can evaluate it at runtime. > > Once we have a proper solution for the 2106 issue we have a clear > indicator which of the drivers needs that treatment and which ones do > not. We just can deduce it from the set_mmss callback assignment. > > This would be good, I agree. Thanks for your time. Regards, Xunlei > Thanks, > > tglx >
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index 38e26be..7f1950f8 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -358,6 +358,29 @@ void devm_rtc_device_unregister(struct device *dev, struct rtc_device *rtc) } EXPORT_SYMBOL_GPL(devm_rtc_device_unregister); +/* Rtc epoch year, can be overrided by command line */ +static unsigned int rtc_epoch = 1970; +time64_t rtc_epoch_secs_since_1970; + +static int __init +set_rtc_epoch(char *str) +{ + unsigned int epoch; + + if (kstrtou32(str, 0, &epoch) != 0) { + printk(KERN_WARNING "Invalid setup epoch %u!\n", epoch); + return 1; + } + + if (rtc_epoch < 1970) + printk(KERN_WARNING "Epoch %u is smaller than 1970!\n", epoch); + else + rtc_epoch = epoch; + + return 1; +} +__setup("rtc_epoch=", set_rtc_epoch); + static int __init rtc_init(void) { rtc_class = class_create(THIS_MODULE, "rtc"); @@ -366,6 +389,8 @@ static int __init rtc_init(void) return PTR_ERR(rtc_class); } rtc_class->pm = RTC_CLASS_DEV_PM_OPS; + + rtc_epoch_secs_since_1970 = mktime64(rtc_epoch, 1, 1, 0, 0, 0); rtc_dev_init(); rtc_sysfs_init(rtc_class); return 0; diff --git a/drivers/rtc/rtc-lib.c b/drivers/rtc/rtc-lib.c index e6bfb9c..d26814c 100644 --- a/drivers/rtc/rtc-lib.c +++ b/drivers/rtc/rtc-lib.c @@ -124,6 +124,34 @@ time64_t rtc_tm_to_time64(struct rtc_time *tm) EXPORT_SYMBOL(rtc_tm_to_time64); /* + * Convert seconds since rtc epoch to seconds since Unix epoch. + */ +time64_t rtc_hw32_to_time64(u32 hwtime) +{ + return (rtc_epoch_secs_since_1970 + hwtime); +} +EXPORT_SYMBOL_GPL(rtc_hw32_to_time64); + +/* + * Convert seconds since Unix epoch to seconds since rtc epoch. + */ +int rtc_time64_to_hw32(time64_t time, u32 *hwtime) +{ + /* time must be after rtc epoch */ + if (time < rtc_epoch_secs_since_1970) + return -EINVAL; + + /* rtc epoch may be too small? */ + if (time - rtc_epoch_secs_since_1970 > UINT_MAX) + return -EOVERFLOW; + + *hwtime = time - rtc_epoch_secs_since_1970; + + return 0; +} +EXPORT_SYMBOL_GPL(rtc_time64_to_hw32); + +/* * Convert rtc_time to ktime */ ktime_t rtc_tm_to_ktime(struct rtc_time tm) diff --git a/include/linux/rtc.h b/include/linux/rtc.h index d080606..32f7c22 100644 --- a/include/linux/rtc.h +++ b/include/linux/rtc.h @@ -16,11 +16,14 @@ #include <linux/interrupt.h> #include <uapi/linux/rtc.h> +extern time64_t rtc_epoch_secs_since_1970; extern int rtc_month_days(unsigned int month, unsigned int year); extern int rtc_year_days(unsigned int day, unsigned int month, unsigned int year); extern int rtc_valid_tm(struct rtc_time *tm); extern time64_t rtc_tm_to_time64(struct rtc_time *tm); extern void rtc_time64_to_tm(time64_t time, struct rtc_time *tm); +extern int rtc_time64_to_hw32(time64_t time, u32 *hwtime); +extern time64_t rtc_hw32_to_time64(u32 hwtime); ktime_t rtc_tm_to_ktime(struct rtc_time tm); struct rtc_time rtc_ktime_to_tm(ktime_t kt);
To attack the 32bit rtc hardware limitations which will bring about y2106 issues to their rtc drivers as those marked "y2106 issue" tags in the former patch, this patch adds a common rtc epoch offset, and provides two interfaces to map between 32bit rtc hardware and 64bit time using this rtc epoch offset. These two interfaces can be used to write rtc drivers for 32-bit rtc hardware. - Add "rtc_epoch" boot parameter, default offset value is hardcoded to 1970 for consistency with Unix epoch. This default value can be overrided through boot command if you have problematic rtc hardware. Once you are doing this, should be aware of following side effects: a) Can't set time before your rtc epoch. b) Can't boot into different OSes relying on the same rtc hardware. "rtc epoch" must stay invariable once finalized on one machine. - Add rtc_time64_to_hw32(): This interface converts 64-bit seconds since unix 1970 epoch to 32bit seconds since rtc epoch which can be maintained by 32-bit rtc hardware. - Add rtc_hw32_to_time64(): This interface converts 32bit seconds since rtc epoch which can be used by 32-bit rtc hardware to 64bit seconds since unix 1970 epoch. There're also other ways to implement this function: For example, some driver(vr41xx) implements the same logic using RTC_EPOCH_SET ioctl and an rtc epoch offset variable. But the approach in this patch is more flexible and common than that. Cc: John Stultz <john.stultz@linaro.org> Cc: Arnd Bergmann <arnd.bergmann@linaro.org> Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org> --- drivers/rtc/class.c | 25 +++++++++++++++++++++++++ drivers/rtc/rtc-lib.c | 28 ++++++++++++++++++++++++++++ include/linux/rtc.h | 3 +++ 3 files changed, 56 insertions(+)