| Message ID | 49705E87.7080405@visionsystems.de |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Fri, 16 Jan 2009 11:16:39 +0100 Yegor Yefremov <yegor_sub1@visionsystems.de> wrote: > The script will set the date to default value. But the problem with > rtc_hctosys() still remains. If the value returned from the RTC driver > (those returning rtc_valid_tm() and not 0 if there were no communication > errors) is invalid you'll see the following error message: > > "hctosys: unable to read the hardware clock\n" I will probably change this message to something different. > because err != 0 and you don't reach the rtc_valid_tm() in rtc_hctosys() > itself. This is only functioning if read call returns 0, then > rtc_valid_tm() will be called and provided the value is invalid you get > the right message: > > "hctosys: invalid date/time\n" that's a fallback to cope with badly written drivers. > So in the case of ds1307 you cannot rely on the error message. And if it > is the desired behavior, that the value has to be checked by all > drivers, why not doing so on the central place in > drivers/rtc/interface.c->rtc_read_time()? (see interface.diff) It's done that way in order to make driver authors conscious on what they are doing.
Yegor Yefremov wrote: > Jüri Reitel schrieb: >> Yegor Yefremov wrote: >>> David Brownell wrote: >>>> On Thursday 15 January 2009, Yegor Yefremov wrote: >>>> >>>>> we are maintaining Linux kernel for our OpenRISC device family >>>>> like Alekto and Alena. Both devices are using RTC DS1337 chip from >>>>> Maxim. Sometimes it happens that the RTC provides invalid values >>>>> such as 32 days or 13th month etc. The RTC is still working >>>>> properly, but due to this check rtc_valid_tm() the time in the >>>>> RTC cannot be changed, >>>> >>>> That would be a bug in whatever version of hwclock you're using. >>>> I recall such a bug getting discussed, and fixed, a while back. >>>> >>>> Basically, if you're setting the clock, you don't need to read >>>> it first... >>>> >>>> - Dave >>>> >>> I see your point, but then the drivers/rtc/hctosys.c should be >>> changed not to check the correctness of returned time value but to >>> parse the returned err value. See the diff file to 2.6.29-rc1. >>> >>> - Yegor >>>> >>>> >>>>> because ds1307_get_time() comes with error. I think it is a matter >>>>> of application such as hwclock to check if the time value returned >>>>> is valid or not, but as long as I2C communication is working >>>>> without errors the read call should be successful. For example >>>>> rtc_hctosys() from /driver/rtc/hctosys.c is making this check >>>>> itself. To throw a kernel warning were also a solution. What do >>>>> you think about this? >>>>> >>>> >>>> >>>> >>> >> My opinion is that nothing should be changed in the kernel. Instead >> use user space script during boot >> >> this script is tested with BusyBox v1.10.1 >> >> #!/bin/sh >> year=`date +"%Y"` >> default_year=2009 >> if [ "$year" -lt "$default_year" ] ; then >> date -D "%Y.%m.%d" $default_year.01.01 > /dev/null >> hwclock -w >> echo "setting date to default `date`" >> fi >> >> you can change default_year to 1970 if you like >> because when rtc_hctosys function fails system default time is set to >> 1970 >> >> To me it appears that if `hwclock` returns error (because of >> rtc_read_time error) the `hwclock -w` sill works (so that >> rtc_write_time works) >> >> JR >> > The script will set the date to default value. But the problem with > rtc_hctosys() still remains. If the value returned from the RTC driver > (those returning rtc_valid_tm() and not 0 if there were no > communication errors) is invalid you'll see the following error message: > > "hctosys: unable to read the hardware clock\n" > > because err != 0 and you don't reach the rtc_valid_tm() in > rtc_hctosys() itself. This is only functioning if read call returns 0, > then rtc_valid_tm() will be called and provided the value is invalid > you get the right message: > > "hctosys: invalid date/time\n" > > So in the case of ds1307 you cannot rely on the error message. And if > it is the desired behavior, that the value has to be checked by all > drivers, why not doing so on the central place in > drivers/rtc/interface.c->rtc_read_time()? (see interface.diff) > > Yegor Sure current kernel error on rtc_hctosys is misleading or incomplete, and sure that rtc_valid_tm usage in kernel code is not so clean (rtc_valid_tm can be put to more central location) but if done so then all rtc code must be revisited and all calls to rtc_valid_tm should be removed from concrete rtc drivers like in function ds1307_get_time, and other places that use rtc_read_tme. If ds1307_get_time function does not call rtc_valid_tm any more you must find all possible usages of ds1307_get_time function, but that is not easy because this function is assigned to field read_time in structure rtc_class_ops so you must search for the "->read_time(" and see whether the caller assumed that rtc_valid_tm was called or not. Very good if somebody is willing to make linux kernel better however i can not see your intent, are you just pointing that something could be done better or are you really willing to make things better? JR --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. -~----------~----~----~----~------~----~------~--~---
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index 4348c4b..7328505 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -29,6 +29,8 @@ int rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) else { memset(tm, 0, sizeof(struct rtc_time)); err = rtc->ops->read_time(rtc->dev.parent, tm); + if(!err) + err = rtc_valid_tm(tm); } mutex_unlock(&rtc->ops_lock);