Re: [patch 2.6.20-rc3 1/3] rtc-cmos driver

From: David Brownell <david-b@pacbell.net>
To: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Alessandro Zummo <alessandro.zummo@towertech.it>,
	rtc-linux@googlegroups.com,
	Linux Kernel list <linux-kernel@vger.kernel.org>
Subject: Re: [patch 2.6.20-rc3 1/3] rtc-cmos driver
Date: Mon, 28 May 2007 14:07:04 -0700	[thread overview]
Message-ID: <200705281407.05250.david-b@pacbell.net> (raw)
In-Reply-To: <20070528183513.GA2815@srcf.ucam.org>

On Monday 28 May 2007, Matthew Garrett wrote:
> On Fri, Jan 05, 2007 at 10:01:57AM -0800, David Brownell wrote:
> > This is an "RTC framework" driver for the "CMOS" RTCs which are standard
> > on PCs and some other platforms.  That's MC146818 compatible silicon.
> > ...
> > +static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
> 
> This is awkward. At the very least, year will be set to -1. This then 
> gets passed through to rtc_tm_to_time, which results in reading 
> wakealarm providing very odd feedback. I guess the "right" fix is for 
> rtc_tm_to_time to use the current values for anything that's -1?

Well ... legacy APIs allow "-1" there; /proc/driver/rtc certainly
interprets wildcards consistently too.  That is, historically it's
not been legit to call rtc_tm_to_time(&t->time).

A counter-argument could be made that rtc_read_alarm() should get
rid of wildcards.  It's got the right RTC in hand; rtc_tm_to_time()
doesn't.  Other RTCs can have this same type of "wildcard" issue.

Me, I'd prefer to make the API treat alarms as purely oneshot.
That whole "wildcard" model is, as you noted, awkward ... even
though it's got hardware support in cases like MC146818 and clones,
it's not very portable.  But I don't want to push that issue either
way just now.

> > +	rtc_control = CMOS_READ(RTC_CONTROL);
> > +	rtc_control &= ~(RTC_PIE | RTC_AIE | RTC_UIE);
> 
> Do you really want to clobber RTC_AIE on probe? If an alarm has been set 
> by the BIOS, it seems a little unfair to disable it on boot.

I was wondering about that in the context of a different RTC, which
happens to run on BIOS-less hardware.  Which, curiously, may be the
opposite of BIOS-impaired hardware ... :)

In general, I suspect the alarm should stay active ... unless its
time has already passed.  That was the original intent of that
tweak, but of course PCs don't actually *have* oneshot alarms, so
there's no way to tell if a given alarm is in the past.  Leading to
the conclusion that AIE should probably stay enabled.

- Dave