[RFC] [PATCH 1/3] Allow to override the RTC alarm time

[RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhao Yakui]

Subject: ACPI: Allow to override the RTC alarm time 
>From : Zhao Yakui <yakui.zhao@intel.com>

In current kernel when RTC alarm time is set and not fired,
it is impossible to set RTC alarm again. But it is more
reasonable that the RTC alarm time can be overrided.

At the same time there is a bug in the function of 
cmos_set_alarm. RTC alarm time for October can't be set 
correctly. 
For October: 0x0A will be written into the RTC region
(MONTH_ALARM). But in fact 0x10 should be written.

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Zhang Rui  <rui.zhang@intel.com>

---
 drivers/rtc/rtc-cmos.c  |    5 ++---
 drivers/rtc/rtc-sysfs.c |    2 --
 2 files changed, 2 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/rtc/rtc-cmos.c
===================================================================
--- linux-2.6.orig/drivers/rtc/rtc-cmos.c
+++ linux-2.6/drivers/rtc/rtc-cmos.c
@@ -198,9 +198,8 @@ static int cmos_set_alarm(struct device 
 
 	/* Writing 0xff means "don't care" or "match all".  */
 
-	mon = t->time.tm_mon;
-	mon = (mon < 12) ? BIN2BCD(mon) : 0xff;
-	mon++;
+	mon = t->time.tm_mon + 1;
+	mon = (mon <= 12) ? BIN2BCD(mon) : 0xff;
 
 	mday = t->time.tm_mday;
 	mday = (mday >= 1 && mday <= 31) ? BIN2BCD(mday) : 0xff;
Index: linux-2.6/drivers/rtc/rtc-sysfs.c
===================================================================
--- linux-2.6.orig/drivers/rtc/rtc-sysfs.c
+++ linux-2.6/drivers/rtc/rtc-sysfs.c
@@ -163,8 +163,6 @@ rtc_sysfs_set_wakealarm(struct device *d
 		retval = rtc_read_alarm(rtc, &alm);
 		if (retval < 0)
 			return retval;
-		if (alm.enabled)
-			return -EBUSY;
 
 		alm.enabled = 1;
 	} else {

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Tuesday 08 April 2008, Zhao Yakui wrote:
> Subject: ACPI: Allow to override the RTC alarm time 
> >From : Zhao Yakui <yakui.zhao@intel.com>
> 
> In current kernel when RTC alarm time is set and not fired,
> it is impossible to set RTC alarm again. But it is more
> reasonable that the RTC alarm time can be overrided.

I'll disagree.  The problem is that if some task is waiting
for the alarm at the specified time, you've just trashed the
alarm it was relying on.


> --- linux-2.6.orig/drivers/rtc/rtc-sysfs.c
> +++ linux-2.6/drivers/rtc/rtc-sysfs.c
> @@ -163,8 +163,6 @@ rtc_sysfs_set_wakealarm(struct device *d

The comment explains much of this ... and if this patch
were deemed to be good, you should have changed that comment
rather than making it a lie:

                /* Avoid accidentally clobbering active alarms; we can't
                 * entirely prevent that here, without even the minimal
                 * locking from the /dev/rtcN api.
                 */
                retval = rtc_read_alarm(rtc, &alm);


>  		retval = rtc_read_alarm(rtc, &alm);
>  		if (retval < 0)
>  			return retval;
> -		if (alm.enabled)
> -			return -EBUSY;
>  
>  		alm.enabled = 1;
>  	} else {
> 
> 

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhang, Rui]

On Wed, 2008-04-09 at 15:34 +0800, David Brownell wrote:
> On Tuesday 08 April 2008, Zhao Yakui wrote:
> > Subject: ACPI: Allow to override the RTC alarm time
> > >From : Zhao Yakui <yakui.zhao@intel.com>
> >
> > In current kernel when RTC alarm time is set and not fired,
> > it is impossible to set RTC alarm again. But it is more
> > reasonable that the RTC alarm time can be overrided.
> 
> I'll disagree.  The problem is that if some task is waiting
> for the alarm at the specified time, you've just trashed the
> alarm it was relying on.
Hmm, what if the wakealarm is set by mistake?
is there any chance that user can fix it?
If not, we can add another prefix so that user can override
the wakealarm file if really needed, just like what we do in patch 3.

thanks,
rui
> 
> 
> > --- linux-2.6.orig/drivers/rtc/rtc-sysfs.c
> > +++ linux-2.6/drivers/rtc/rtc-sysfs.c
> > @@ -163,8 +163,6 @@ rtc_sysfs_set_wakealarm(struct device *d
> 
> The comment explains much of this ... and if this patch
> were deemed to be good, you should have changed that comment
> rather than making it a lie:
> 
>                 /* Avoid accidentally clobbering active alarms; we
> can't
>                  * entirely prevent that here, without even the
> minimal
>                  * locking from the /dev/rtcN api.
>                  */
>                 retval = rtc_read_alarm(rtc, &alm);
> 
> 
> >               retval = rtc_read_alarm(rtc, &alm);
> >               if (retval < 0)
> >                       return retval;
> > -             if (alm.enabled)
> > -                     return -EBUSY;
> > 
> >               alm.enabled = 1;
> >       } else {
> >
> >
> 
> 
> 
> 

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Wednesday 09 April 2008, Zhang, Rui wrote:
> > > In current kernel when RTC alarm time is set and not fired,
> > > it is impossible to set RTC alarm again. But it is more
> > > reasonable that the RTC alarm time can be overrided.
> > 
> > I'll disagree.  The problem is that if some task is waiting
> > for the alarm at the specified time, you've just trashed the
> > alarm it was relying on.
>  
> Hmm, what if the wakealarm is set by mistake?
> is there any chance that user can fix it?

Certainly; there are several ways to turn an alarm off.
With sysfs, just write a time in the past, as I recall...

If you're concerned about accidents, think of it this way:
the proposed patch would make it *REALLY EASY* to have
accidents with sysfs that bork the alarm state, which can
have been set by some task that's relying on it.

The current behavior doesn't prevent such borkage; it
can't, really, because of the alarm model.  What it does
instead is force whatever shell script is doing that to be
explicit about *intending* such borkage ... requiring it
to disable existing alarms, instead of allowing accidents.

- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Wednesday 09 April 2008, Zhao Yakui wrote:
> > > Hmm, what if the wakealarm is set by mistake?
> > > is there any chance that user can fix it?
> > 
> > Certainly; there are several ways to turn an alarm off.
> > With sysfs, just write a time in the past, as I recall...
> > 
> > If you're concerned about accidents, think of it this way:
> > the proposed patch would make it *REALLY EASY* to have
> > accidents with sysfs that bork the alarm state, which can
> > have been set by some task that's relying on it.
> 
> But how to solve the following case?
> 
> It is assumed that the RTC alarm is set by some task and not fired (It
> will be fired after 20 minutes). The system enters the sleeping state
> and is required to be resumed after some time(For example: ten minutes).
> 
> How to set the RTC alarm?

You mean, how to decide which alarm setting should "win"?
If sysfs accidents aren't going to trump everything else.

That's a fair question.  I don't think there's a good answer
to that with today's infrastructure.  Arguably, there should
be the notion of a number of clients, each of which get told
when the alarm they request fires.  But today, there's only
a single alarm, and a single client.

- Dave

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhao Yakui]

On Wed, 2008-04-09 at 01:27 -0700, David Brownell wrote:
> On Wednesday 09 April 2008, Zhang, Rui wrote:
> > > > In current kernel when RTC alarm time is set and not fired,
> > > > it is impossible to set RTC alarm again. But it is more
> > > > reasonable that the RTC alarm time can be overrided.
> > > 
> > > I'll disagree.  The problem is that if some task is waiting
> > > for the alarm at the specified time, you've just trashed the
> > > alarm it was relying on.
> >  
> > Hmm, what if the wakealarm is set by mistake?
> > is there any chance that user can fix it?
> 
> Certainly; there are several ways to turn an alarm off.
> With sysfs, just write a time in the past, as I recall...
> If you're concerned about accidents, think of it this way:
> the proposed patch would make it *REALLY EASY* to have
> accidents with sysfs that bork the alarm state, which can
> have been set by some task that's relying on it.
But how to solve the following case?
It is assumed that the RTC alarm is set by some task and not fired (It
will be fired after 20 minutes). The system enters the sleeping state
and is required to be resumed after some time(For example: ten minutes).
How to set the RTC alarm?
> The current behavior doesn't prevent such borkage; it
> can't, really, because of the alarm model.  What it does
> instead is force whatever shell script is doing that to be
> explicit about *intending* such borkage ... requiring it
> to disable existing alarms, instead of allowing accidents.
> - Dave
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhao Yakui]

On Wed, 2008-04-09 at 03:13 -0700, David Brownell wrote:
> On Wednesday 09 April 2008, Zhao Yakui wrote:
> > > > Hmm, what if the wakealarm is set by mistake?
> > > > is there any chance that user can fix it?
> > > 
> > > Certainly; there are several ways to turn an alarm off.
> > > With sysfs, just write a time in the past, as I recall...
> > > 
> > > If you're concerned about accidents, think of it this way:
> > > the proposed patch would make it *REALLY EASY* to have
> > > accidents with sysfs that bork the alarm state, which can
> > > have been set by some task that's relying on it.
> > 
> > But how to solve the following case?
> > 
> > It is assumed that the RTC alarm is set by some task and not fired (It
> > will be fired after 20 minutes). The system enters the sleeping state
> > and is required to be resumed after some time(For example: ten minutes).
> > 
> > How to set the RTC alarm?
> 
> You mean, how to decide which alarm setting should "win"?
> If sysfs accidents aren't going to trump everything else.
Yes. IMO Sysfs I/F should have more priority than other.

At the same time there is another question about this.
alm.enabled is related with AIE flag. When alarm is fired, the AIE flag
won't be cleared automatically. If we want to set alarm again, should we
turn off the alarm firstly?  

> That's a fair question.  I don't think there's a good answer
> to that with today's infrastructure.  Arguably, there should
> be the notion of a number of clients, each of which get told
> when the alarm they request fires.  But today, there's only
> a single alarm, and a single client.
Agree. Now there is only one alarm and current infrastruture can't
support more than two clients.
> 
> - Dave
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Friday 11 April 2008, Zhao Yakui wrote:
> But The following is reasonable. IMO.
>  For October: 0x0A will be written into the RTC region
> (MONTH_ALARM). But in fact 0x10 should be written.
>        /* Writing 0xff means "don't care" or "match all".  */
>  
> -       mon = t->time.tm_mon;
> -       mon = (mon < 12) ? BIN2BCD(mon) : 0xff;
> -       mon++;
> +       mon = t->time.tm_mon + 1;
> +       mon = (mon <= 12) ? BIN2BCD(mon) : 0xff;
> 
> Is there an opportunity to merge it?

Sure.  Submit that as a patch by itself.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Friday 11 April 2008, Zhao Yakui wrote:
> 
>   There is another question about the driver rtc and rtc-cmos.
> rtc driver is in drivers/char/ and rtc-cmos is in drivers/rtc/

A patch that's pending for 2.6.26 (in MM) makes sure that only
one of those can ever be configured.


> On some laptops the two drivers will share the same memory/irq resource,
> which will cause that rtc-cmos driver can't work well after the rtc
> driver is already loaded.(Of course RTC sys I/F isn't created in rtc
> driver.)

That's a configuration error.  If the new RTC framework is in
use, legacy drivers should not be used.

- Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[David Brownell]

On Thursday 10 April 2008, Zhao Yakui wrote:
> At the same time there is another question about this.
> alm.enabled is related with AIE flag. When alarm is fired, the AIE flag
> won't be cleared automatically.

Are you sure?  I thought that bug was fixed... alarms
should be one-shot, not periodic.


> If we want to set alarm again, should we 
> turn off the alarm firstly?  

As a rule, yes.  We want to present a portable model for
RTC alarms, which fire exactly once.  The PC/AT model,
with wildcarding for hh:mm:ss (and maybe MM:DD, allowed by
ACPI extensions) is not very portable.

(By the way, if someone wants to take rtc-cmos off my
hands, I'd be glad to offload it ... my interest was
just to try to get the PC platform to play in the same
RTC space the other Linux platforms do!)


> > That's a fair question.  I don't think there's a good answer
> > to that with today's infrastructure.  Arguably, there should
> > be the notion of a number of clients, each of which get told
> > when the alarm they request fires.  But today, there's only
> > a single alarm, and a single client.
> 
> Agree. Now there is only one alarm and current infrastruture can't
> support more than two clients.

I don't see how it could be claimed to support two
clients.  The second one clobbers the first ...

Now, there *ARE* some RTCs that support multiple
alarms.  It's common in discrete I2C and SPI chips.
But that's a capability that's not currently exposed
by Linux; one of many, as noted in the current rtc(4)
man page (dated 2006-11-26 in my copy).

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Alessandro Zummo]

On Fri, 11 Apr 2008 20:44:21 +0800
Zhao Yakui <yakui.zhao@intel.com> wrote:

> How about the following issue? 
>   There is another question about the driver rtc and rtc-cmos.
> rtc driver is in drivers/char/ and rtc-cmos is in drivers/rtc/
> On some laptops the two drivers will share the same memory/irq resource,
> which will cause that rtc-cmos driver can't work well after the rtc
> driver is already loaded.(Of course RTC sys I/F isn't created in rtc
> driver.)
>   How to solve this issue?  

The driver in drivers/char/ should not be loaded at all. the most recent
 Kconfig  will check and avoid letting you select both of them.

--

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhao Yakui]

On Wed, 2008-04-09 at 03:13 -0700, David Brownell wrote:
> On Wednesday 09 April 2008, Zhao Yakui wrote:
> > > > Hmm, what if the wakealarm is set by mistake?
> > > > is there any chance that user can fix it?
> > > 
> > > Certainly; there are several ways to turn an alarm off.
> > > With sysfs, just write a time in the past, as I recall...
> > > 
> > > If you're concerned about accidents, think of it this way:
> > > the proposed patch would make it *REALLY EASY* to have
> > > accidents with sysfs that bork the alarm state, which can
> > > have been set by some task that's relying on it.
> > 
> > But how to solve the following case?
> > 
> > It is assumed that the RTC alarm is set by some task and not fired (It
> > will be fired after 20 minutes). The system enters the sleeping state
> > and is required to be resumed after some time(For example: ten minutes).
> > 
> > How to set the RTC alarm?
> 
> You mean, how to decide which alarm setting should "win"?
> If sysfs accidents aren't going to trump everything else.
What David said is right. There is a problem that settling the alarm unconditionally will
trash the alarm time on which some tasks are relying on. 
It is OK to drop this patch.

But The following is reasonable. IMO.
 For October: 0x0A will be written into the RTC region
(MONTH_ALARM). But in fact 0x10 should be written.
       /* Writing 0xff means "don't care" or "match all".  */
 
-       mon = t->time.tm_mon;
-       mon = (mon < 12) ? BIN2BCD(mon) : 0xff;
-       mon++;
+       mon = t->time.tm_mon + 1;
+       mon = (mon <= 12) ? BIN2BCD(mon) : 0xff;

Is there an opportunity to merge it?
Thanks.
   Yakui
> That's a fair question.  I don't think there's a good answer
> to that with today's infrastructure.  Arguably, there should
> be the notion of a number of clients, each of which get told
> when the alarm they request fires.  But today, there's only
> a single alarm, and a single client.
> 
> - Dave
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] [PATCH 1/3] Allow to override the RTC alarm time

[Zhao Yakui]

On Thu, 2008-04-10 at 20:46 -0700, David Brownell wrote:
> On Friday 11 April 2008, Zhao Yakui wrote:
> > But The following is reasonable. IMO.
> >  For October: 0x0A will be written into the RTC region
> > (MONTH_ALARM). But in fact 0x10 should be written.
> >        /* Writing 0xff means "don't care" or "match all".  */
> >  
> > -       mon = t->time.tm_mon;
> > -       mon = (mon < 12) ? BIN2BCD(mon) : 0xff;
> > -       mon++;
> > +       mon = t->time.tm_mon + 1;
> > +       mon = (mon <= 12) ? BIN2BCD(mon) : 0xff;
> > 
> > Is there an opportunity to merge it?
> 
> Sure.  Submit that as a patch by itself.
> 
How about the following issue? 
  There is another question about the driver rtc and rtc-cmos.
rtc driver is in drivers/char/ and rtc-cmos is in drivers/rtc/
On some laptops the two drivers will share the same memory/irq resource,
which will cause that rtc-cmos driver can't work well after the rtc
driver is already loaded.(Of course RTC sys I/F isn't created in rtc
driver.)
  How to solve this issue?  

Best regards
   Yakui

> - Dave
>