| Message ID | 20161208171010.29446-3-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 08, 2016 at 06:10:09PM +0100, Gregory CLEMENT wrote: > It is one more step to remove the deprecated functions rtc_time_to_tm > and rtc_tm_to_time. I fail to see any advantage to this patch, or in fact the y2038 patch merged into rtc. Let's first look at the original rtc_time_to_tm(): +void rtc_time_to_tm(unsigned long time, struct rtc_time *tm) +{ + days = time / 86400; + year = 1970 + days / 365; time is an unsigned long, so is 32-bit or 64-bit. Let's assume 32-bit. The maximum value it can have is 2^32-1 - which gives a maximum days value of 49710 (it always fits in an int or unsigned int as a positive value on any arch supported by Linux.) That gives a maximum value of 'year' as 2106. So, actually rtc_time_to_tm() doesn't have a y2038 problem, it has a y2106 problem. So, the commentary in the original commit is actually wrong! Okay, that aside, let's look at what your patch actually achieves. Old code to set the time: unsigned long time; ret = rtc_tm_to_time(tm, &time); if (ret) goto out; /* write time to a 32-bit register */ New code: time64_t time; time = rtc_tm_to_time64(tm); /* write time to a 32-bit register */ I fail to see how this is any kind of improvement, or aids to solve the y2106 problem (not y2038) here. A better approach would have been to also provide a rtc_tm_to_time32() following the function signature of rtc_tm_to_time(), but replacing the unsigned long pointer with a u32 pointer, and (importantly) making it fail if the time was unrepresentable as a u32 value. Why u32 and not time_t? time_t is a signed value, which suffers from the y2038 problem. u32 suffers from a y2106 problem, and is what we have with the original rtc_tm_to_time() on 32-bit platforms. So it's safe _not_ to needlessly chop off a bit there which wasn't done in the original code. This would mean that drivers (such as armada38x) which are only capable of storing a 32-bit time fail gracefully to set times beyond y2106. The problem as I see it right now is that the y2038 patch which deprecates rtc_time_to_tm() etc wasn't thought out thoroughly enough, and is making the situation much worse by pushing the problem in many drivers in a way that results in it being less visible. I think this needs to be reconsidered, and it needs a much better implementation - one which at least tells the user that their hardware has broken when trying to set the time to something the hardware can't support.
diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c index a0859286a4c4..b8a74ffaae80 100644 --- a/drivers/rtc/rtc-armada38x.c +++ b/drivers/rtc/rtc-armada38x.c @@ -128,13 +128,14 @@ static unsigned long read_rtc_register_wa(struct armada38x_rtc *rtc, u8 rtc_reg) static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm) { struct armada38x_rtc *rtc = dev_get_drvdata(dev); - unsigned long time, flags; + unsigned long flags; + time64_t time; spin_lock_irqsave(&rtc->lock, flags); - time = read_rtc_register_wa(rtc, RTC_TIME); + time = (time64_t)read_rtc_register_wa(rtc, RTC_TIME); spin_unlock_irqrestore(&rtc->lock, flags); - rtc_time_to_tm(time, tm); + rtc_time64_to_tm(time, tm); return 0; } @@ -142,37 +143,34 @@ static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm) static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm) { struct armada38x_rtc *rtc = dev_get_drvdata(dev); - int ret = 0; - unsigned long time, flags; - - ret = rtc_tm_to_time(tm, &time); + unsigned long flags; + time64_t time; - if (ret) - goto out; + time = rtc_tm_to_time64(tm); spin_lock_irqsave(&rtc->lock, flags); - rtc_delayed_write(time, rtc, RTC_TIME); + rtc_delayed_write((u32)time, rtc, RTC_TIME); spin_unlock_irqrestore(&rtc->lock, flags); -out: - return ret; + return 0; } static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) { struct armada38x_rtc *rtc = dev_get_drvdata(dev); - unsigned long time, flags; + unsigned long flags; + time64_t time; u32 val; spin_lock_irqsave(&rtc->lock, flags); - time = read_rtc_register_wa(rtc, RTC_ALARM1); + time = (time64_t)read_rtc_register_wa(rtc, RTC_ALARM1); val = read_rtc_register_wa(rtc, RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN; spin_unlock_irqrestore(&rtc->lock, flags); alrm->enabled = val ? 1 : 0; - rtc_time_to_tm(time, &alrm->time); + rtc_time64_to_tm(time, &alrm->time); return 0; } @@ -180,18 +178,15 @@ static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) { struct armada38x_rtc *rtc = dev_get_drvdata(dev); - unsigned long time, flags; - int ret = 0; + unsigned long flags; + time64_t time; u32 val; - ret = rtc_tm_to_time(&alrm->time, &time); - - if (ret) - goto out; + time = rtc_tm_to_time64(&alrm->time); spin_lock_irqsave(&rtc->lock, flags); - rtc_delayed_write(time, rtc, RTC_ALARM1); + rtc_delayed_write((u32)time, rtc, RTC_ALARM1); if (alrm->enabled) { rtc_delayed_write(RTC_IRQ1_AL_EN, rtc, RTC_IRQ1_CONF); @@ -202,8 +197,7 @@ static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) spin_unlock_irqrestore(&rtc->lock, flags); -out: - return ret; + return 0; } static int armada38x_rtc_alarm_irq_enable(struct device *dev,
It is one more step to remove the deprecated functions rtc_time_to_tm and rtc_tm_to_time. And as bonus it simplifies a little the driver. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- drivers/rtc/rtc-armada38x.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)