rtc: how should I handle an invalid state?

rtc: how should I handle an invalid state?

[Uwe Kleine-König]

Hello,

I'm still stuck with my rtc.  My current problem is setting the rtc
using hwclock --systohc.

The problem is that hwclock tries to read the current time before
setting the new one.  And if that fails, it doesn't update the rtc.

If no error occurs the procedure to read the time is:

	fd = open("/dev/rtc", O_RDONLY|O_LARGEFILE)
	ioctl(fd, RTC_UIE_ON, 0);
	select(fd + 1, [fd], NULL, NULL, {5, 0});
	ioctl(fd, RTC_RD_TIME, &rtc_time);

If one of ioctl(fd, RTC_UIE_ON, 0) or select(fd + 1, [fd], NULL, NULL,
{5, 0}) fail, hwclock aborts.

OTOH I don't want to report success in both because then there is no way
to distinguish between a valid and an unvalid date for hwclock.

Looking at some other drivers they don't seem to handle that case.

Maybe returning an invalid date could work?

What do you think?

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962

Re: rtc: how should I handle an invalid state?

[Alessandro Zummo]

On Tue, 29 Apr 2008 14:19:24 +0200
Uwe Kleine-König <Uwe.Kleine-Koenig@digi.com> wrote:

> Hello,
> 
> I'm still stuck with my rtc.  My current problem is setting the rtc
> using hwclock --systohc.
> 
> The problem is that hwclock tries to read the current time before
> setting the new one.  And if that fails, it doesn't update the rtc.

 That's a bug of hwclock. :( The correct behaviour for a driver
 is to return an error if the clock cannot be read.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: rtc: how should I handle an invalid state?

[David Brownell]

On Tuesday 29 April 2008, Uwe Kleine-König wrote:
> The problem is that hwclock tries to read the current time before
> setting the new one.  And if that fails, it doesn't update the rtc.

Which version of hwclock?  That does seem like a bug in that
particular version.  If it's in util-linux-ng or busybox,
please send in a patch.  I don't think anyone should be
deploying *new* systems using *old/unmaintained* util-linux.

I'll suspect this is on some util-linux version, since I'm
quite sure that I've had busybox based systems that don't
show this misbehavior.  Haven't had occasion to use non-PC
RTCs with a util-linux-ng version; I wouldn't call a bug in
such combinations a kernel regression, either...

I think the preferred solution never returns invalid times as
valid.  Some RTC drivers do this themselves:

        int my_rtc_read_time(struct device *dev, struct rtc_time *time)
	{
		... read the hardware into *time ...
		return rtc_valid_tm(time);
	}

Or rtc_valid_tm() might be checked in the RTC framework glue,
to provide more uniform behavior at the framework level.

Either way, it's not unknown that an un-initialized RTC have
undefined state, and thus return invalid times until they've
been set (using wall clock, NTP, or whatever).

- Dave

Re: rtc: how should I handle an invalid state?

[Uwe Kleine-König]

Hi Dave,

David Brownell wrote:
> On Tuesday 29 April 2008, Uwe Kleine-König wrote:
> > The problem is that hwclock tries to read the current time before
> > setting the new one.  And if that fails, it doesn't update the rtc.
> 
> Which version of hwclock?  That does seem like a bug in that
> particular version.  If it's in util-linux-ng or busybox,
> please send in a patch.  I don't think anyone should be
> deploying *new* systems using *old/unmaintained* util-linux.
This is hwclock from Debian/stable (i.e. 2.12r-19etch1).  With
Debian/unstables (2.13.1-5) this happens to:

	sh-3.1# /sbin/hwclock --systohc -D
	hwclock from util-linux-ng 2.13.1
	Using /dev interface to clock.
	Last drift adjustment done at 1209474623 seconds after 1969
	Last calibration done at 1209474623 seconds after 1969
	Hardware clock is on local time
	Assuming hardware clock is kept in local time.
	Waiting for clock tick...
	/dev/rtc does not have interrupt functions. Waiting in loop for time from /dev/rtc to change
	RTC_RD_TIME: Invalid argument
	ioctl() to /dev/rtc to read the time failed.

Oh, there is a new version in Debian/unstable (2.14~rc1-1), nothing new
though:

	sh-3.1# /sbin/hwclock --systohc -D
	hwclock from util-linux-ng 2.14-rc1
	Using /dev interface to clock.
	Last drift adjustment done at 1209474623 seconds after 1969
	Last calibration done at 1209474623 seconds after 1969
	Hardware clock is on local time
	Assuming hardware clock is kept in local time.
	Waiting for clock tick...
	/dev/rtc does not have interrupt functions. Waiting in loop for time from /dev/rtc to change
	RTC_RD_TIME: Invalid argument
	ioctl() to /dev/rtc to read the time failed.


I looked a bit in the source of hwclock (from unstable) and IMHO it does
strange things.  E.g. I wouldn't do

	if (getuid() != 0 && systohc) {
		fprintf(stderr,
			_("Sorry, only the superuser can change "
			  "the Hardware Clock.\n"));
		permitted = FALSE;
	}

I wonder further why do_rtc_read_ioctl has a return type of int if it is
unchecked in read_hardware_clock_rtc which unconditionally returns 0.
(Ah, OK, do_rtc_read_ioctl always returns 0, if there is an error,
exit() is called.  Nice.)

Anyhow ...

Ah, reading the hwclock is necessary to adjust /etc/adjtime.  So the
correct way to set the hwclock is 

	# /sbin/hwclock --systohc -D --noadjfile --utc

But as you already suspect, this doesn't help either.

I'll write a bugreport to Debian as I now don't fancy to work with that
piece of code anymore. :(

> I'll suspect this is on some util-linux version, since I'm
> quite sure that I've had busybox based systems that don't
> show this misbehavior.
Right, the busybox I have here behaves fine.

>                         Haven't had occasion to use non-PC
> RTCs with a util-linux-ng version; I wouldn't call a bug in
> such combinations a kernel regression, either...
> 
> I think the preferred solution never returns invalid times as
> valid.  Some RTC drivers do this themselves:
> 
>         int my_rtc_read_time(struct device *dev, struct rtc_time *time)
> 	{
> 		... read the hardware into *time ...
> 		return rtc_valid_tm(time);
> 	}
> 
> Or rtc_valid_tm() might be checked in the RTC framework glue,
> to provide more uniform behavior at the framework level.
Currently it doesn't, the ioctl function for RTC_RD_TIME simply does:

	err = rtc_read_time(rtc, &tm);
	if (err < 0)
		return err;

	if (copy_to_user(uarg, &tm, sizeof(tm)))
		return -EFAULT;

	...

	return err;

> Either way, it's not unknown that an un-initialized RTC have
> undefined state, and thus return invalid times until they've
> been set (using wall clock, NTP, or whatever).
For my rtc it's even better:  There are two flags that indicate that
the clock is running and the time is valid.  So I can do

	if (!clock_is_running || !time_is_valid)
		return -EINVAL;
	else
		readtime(&tm)
		return 0;

I remember that the rx8025, an rtc I wrote a driver for some time ago,
had something similar.

Best regards
Uwe

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962