RTC_DRV_CMOS can break userspace interface

RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

f5f72b46c349fefcfd4421b2213c6ffb324c5e56 appears to break the userspace 
interface to the CMOS alarm. This could previously be accessed via 
/proc/acpi/alarm, but if RTC_DRV_CMOS is enabled that vanishes. The help 
text for the module doesn't mention this, which makes tracking it down a 
touch irritating.

I'm not actually sure why this is the case. It doesn't look like the two 
interfaces are fundamentally incompatible. I agree that removing the 
proc code is a good long-term aim, but it'd be nice to be able to test 
the new RTC code without removing existing functionality.

(It doesn't really help that rtc-cmos doesn't load on this machine, but 
I'll try to track that down later - right now I suspect some sort of PNP 
issue)
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Sun, May 27, 2007 at 08:03:51PM +0100, Matthew Garrett wrote:

> (It doesn't really help that rtc-cmos doesn't load on this machine, but 
> I'll try to track that down later - right now I suspect some sort of PNP 
> issue)

Ah, no, it's because the ioports for the rtc-cmos driver are already 
claimed by the old driver from CONFIG_RTC. The following configuration 
is valid in Kconfig:

CONFIG_RTC=y
CONFIG_RTC_CMOS=m

but will disable /proc/acpi/wakeup. It'll also be impossible to load 
CONFIG_RTC_CMOS because CONFIG_RTC has grabbed the io ports, so it's not 
possible to use the new interface. This situation doesn't appear to be 
documented, which is less than ideal...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Mon, May 28, 2007 at 12:39:11AM +0100, Matthew Garrett wrote:

> but will disable /proc/acpi/wakeup. It'll also be impossible to load 
> CONFIG_RTC_CMOS because CONFIG_RTC has grabbed the io ports, so it's not 
> possible to use the new interface. This situation doesn't appear to be 
> documented, which is less than ideal...

Actually, it seems to be worse than that - the PNP entry for my cmos 
clock doesn't appear to mention an irq, so the wakealarm entry doesn't 
work. I can happily wake it using the /proc/acpi/alarm interface.

David, would you be happy with hardcoding the rtc-cmos IRQ to 8 on PCs 
if there's inadequate PNP information available?
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[David Brownell]

On Sunday 27 May 2007, Matthew Garrett wrote:
> On Mon, May 28, 2007 at 12:39:11AM +0100, Matthew Garrett wrote:
> 
> > but will disable /proc/acpi/wakeup. It'll also be impossible to load 
> > CONFIG_RTC_CMOS because CONFIG_RTC has grabbed the io ports, so it's not 
> > possible to use the new interface. This situation doesn't appear to be 
> > documented, which is less than ideal...
> 
> Actually, it seems to be worse than that - the PNP entry for my cmos 
> clock doesn't appear to mention an irq, so the wakealarm entry doesn't 
> work. I can happily wake it using the /proc/acpi/alarm interface.
> 
> David, would you be happy with hardcoding the rtc-cmos IRQ to 8 on PCs 
> if there's inadequate PNP information available?

That would seem to naturally belong in the PNP code, yes?

Agreed that it seems like it needs to be hardcoded somewhere.

- Dave

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Sun, May 27, 2007 at 06:44:49PM -0700, David Brownell wrote:
> On Sunday 27 May 2007, Matthew Garrett wrote:
> > Actually, it seems to be worse than that - the PNP entry for my cmos 
> > clock doesn't appear to mention an irq, so the wakealarm entry doesn't 
> > work. I can happily wake it using the /proc/acpi/alarm interface.
> > 
> > David, would you be happy with hardcoding the rtc-cmos IRQ to 8 on PCs 
> > if there's inadequate PNP information available?
> 
> That would seem to naturally belong in the PNP code, yes?
> 
> Agreed that it seems like it needs to be hardcoded somewhere.

The PNP code is reporting what's in the tables - I'd be a bit surprised 
if it special-cased specific devices, but I guess there's an argument 
for that. All the other machines I've checked report an IRQ, so I guess 
Apple just didn't take much care in getting this right.

As far as sanity checking goes - how about we check that the reported 
io ports are the legacy range, and if so hardcode the irq if the 
hardware hasn't reported one? I'd /hope/ that nobody has produced any 
hardware that that would break, but then, well. The strongest argument 
for it being safe is probably that the legacy RTC driver seems to 
hardcode this and hasn't obviously been breaking things.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH] RTC: Use fallback IRQ if PNP tables don't provide one

[Matthew Garrett]

From: Matthew Garrett <mjg59@srcf.ucam.org>

Intel Macs (and possibly other machines) provide a PNP entry for the 
RTC, but provide no IRQ. As a result the rtc-cmos driver doesn't allow 
wakeup alarms. If the RTC is located at the legacy ioport range, assume 
that it's on IRQ 8 unless the tables say otherwise.

Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org>

---

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 6085261..e24ea82 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -641,9 +641,16 @@ cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	 * drivers can't provide shutdown() methods to disable IRQs.
 	 * Or better yet, fix PNP to allow those methods...
 	 */
-	return cmos_do_probe(&pnp->dev,
-			&pnp->res.port_resource[0],
-			pnp->res.irq_resource[0].start);
+	if (pnp_port_start(pnp,0) == 0x70 && !pnp_irq_valid(pnp,0))
+		/* Some machines contain a PNP entry for the RTC, but
+		 * don't define the IRQ. It should always be safe to
+		 * hardcode it in these cases
+		 */
+		return cmos_do_probe(&pnp->dev, &pnp->res.port_resource[0], 8);
+	else
+		return cmos_do_probe(&pnp->dev,
+				     &pnp->res.port_resource[0],
+				     pnp->res.irq_resource[0].start);
 }
 
 static void __exit cmos_pnp_remove(struct pnp_dev *pnp)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one

[Matthieu CASTET]

Hi,


On Mon, 28 May 2007 18:24:18 +0100, Matthew Garrett wrote:

> From: Matthew Garrett <mjg59@srcf.ucam.org>
> 
> Intel Macs (and possibly other machines) provide a PNP entry for the
> RTC, but provide no IRQ. As a result the rtc-cmos driver doesn't allow
> wakeup alarms. If the RTC is located at the legacy ioport range, assume
> that it's on IRQ 8 unless the tables say otherwise.
I post something via gmane this morning, but it seems it was lost :

Did you check if there aren't multiple configuration for rtc (one with 
irq, and
one without it) ?

What's the ouput of 
$ for i in /sys/bus/pnp/devices/*; do if [ "$(cat $i/id)" = PNP0b00 ]; 
then cat
$i/resources; echo options; cat $i/options; fi; done

Matthieu

Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one

[Andrew Morton]

On Mon, 28 May 2007 18:50:22 +0000 (UTC)
Matthieu CASTET <castet.matthieu@free.fr> wrote:

> Hi,
> 
> 
> On Mon, 28 May 2007 18:24:18 +0100, Matthew Garrett wrote:
> 
> > From: Matthew Garrett <mjg59@srcf.ucam.org>
> > 
> > Intel Macs (and possibly other machines) provide a PNP entry for the
> > RTC, but provide no IRQ. As a result the rtc-cmos driver doesn't allow
> > wakeup alarms. If the RTC is located at the legacy ioport range, assume
> > that it's on IRQ 8 unless the tables say otherwise.
> I post something via gmane this morning, but it seems it was lost :
> 
> Did you check if there aren't multiple configuration for rtc (one with 
> irq, and
> one without it) ?
> 
> What's the ouput of 
> $ for i in /sys/bus/pnp/devices/*; do if [ "$(cat $i/id)" = PNP0b00 ]; 
> then cat
> $i/resources; echo options; cat $i/options; fi; done
> 

Matthew didn't reply to this, almost surely because you removed him
(and David) from the cc.  Please don't ever do that.

Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one

[Matthew Garrett]

On Tue, May 29, 2007 at 05:30:58PM -0700, Andrew Morton wrote:

> Matthew didn't reply to this, almost surely because you removed him
> (and David) from the cc.  Please don't ever do that.

Ah, I saw this on linux-acpi and replied to it there. There's no entries 
in the options file and the resources one lists no IRQ.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one

[David Brownell]

On Monday 28 May 2007, Matthew Garrett wrote:
> From: Matthew Garrett <mjg59@srcf.ucam.org>
> 
> Intel Macs (and possibly other machines) provide a PNP entry for the 
> RTC, but provide no IRQ. As a result the rtc-cmos driver doesn't allow 
> wakeup alarms. If the RTC is located at the legacy ioport range, assume 
> that it's on IRQ 8 unless the tables say otherwise.
> 
> Signed-off-by: Matthew Garrett <mjg59@srcf.ucam.org>

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

> 
> ---
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 6085261..e24ea82 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -641,9 +641,16 @@ cmos_pnp_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	 * drivers can't provide shutdown() methods to disable IRQs.
>  	 * Or better yet, fix PNP to allow those methods...
>  	 */
> -	return cmos_do_probe(&pnp->dev,
> -			&pnp->res.port_resource[0],
> -			pnp->res.irq_resource[0].start);
> +	if (pnp_port_start(pnp,0) == 0x70 && !pnp_irq_valid(pnp,0))
> +		/* Some machines contain a PNP entry for the RTC, but
> +		 * don't define the IRQ. It should always be safe to
> +		 * hardcode it in these cases
> +		 */
> +		return cmos_do_probe(&pnp->dev, &pnp->res.port_resource[0], 8);
> +	else
> +		return cmos_do_probe(&pnp->dev,
> +				     &pnp->res.port_resource[0],
> +				     pnp->res.irq_resource[0].start);
>  }
>  
>  static void __exit cmos_pnp_remove(struct pnp_dev *pnp)
> 
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org
> 

Re: RTC_DRV_CMOS can break userspace interface

[David Brownell]

On Sunday 27 May 2007, Matthew Garrett wrote:
> f5f72b46c349fefcfd4421b2213c6ffb324c5e56 appears to break the userspace 
> interface to the CMOS alarm. This could previously be accessed via 
> /proc/acpi/alarm ...

I was a bit surprised the ACPI team didn't have more comments on
that issue, myself.  Thing is, all of /proc/acpi/* is deprecated
(scheduled for removal in barely over one month!) and nobody had
found any actual users of that "alarm" file when they searched for
them a while ago.  I suppose the conclusion then was that there
are no applications using it.


> I'm not actually sure why this is the case. It doesn't look like the two 
> interfaces are fundamentally incompatible.

ISTR the issue is that ACPI only allows one chunk of code to hook
into the relevant notifications.  So:  either /proc/acpi/wakeup;
or /sys/class/rtc/rtc0/wakealarm; but not both.


> I agree that removing the  
> proc code is a good long-term aim, but it'd be nice to be able to test 
> the new RTC code without removing existing functionality.

Coexistence is unfortunately problematic here.  And with "long term"
documented to be a bit over a month ... I guess all I can say is
that if you can come up with a good patch to make both available,
please do so.

- Dave

Re: RTC_DRV_CMOS can break userspace interface

[David Brownell]

On Monday 28 May 2007, frank paulsen wrote:
> David Brownell <david-b@pacbell.net> writes:
> 
> > On Sunday 27 May 2007, Matthew Garrett wrote:
> >> f5f72b46c349fefcfd4421b2213c6ffb324c5e56 appears to break the userspace 
> >> interface to the CMOS alarm. This could previously be accessed via 
> >> /proc/acpi/alarm ...
> >
> > I was a bit surprised the ACPI team didn't have more comments on
> > that issue, myself.  Thing is, all of /proc/acpi/* is deprecated
> > (scheduled for removal in barely over one month!) and nobody had
> > found any actual users of that "alarm" file when they searched for
> > them a while ago.  I suppose the conclusion then was that there
> > are no applications using it.
> 
> sorry for breaking the CC:, but i am currently not subscribed to lkml.

I restored both lkml and linux-acpi to the CC list.  :)


> please have a look at at http://www.mythtv.org/wiki/index.php/ACPI_Wakeup
> 
> i think there is quite a number of users, but most of them won't ever
> look into LKML :)

That seems to be true.  And those particular users should learn the
portable /sys/class/rtc/rtc0/wakealarm syntax ... e.g. using numeric
seconds-since-epoch ("date '+%s'") instead of strings the kernel needs
to parse.  That way, they can start converting usage sooner.

Someone with edit privs on that wiki should update those docs, and
tell folk to switch to that interface since the other one is going...


I'm suspecting that there should be a CONFIG_ACPI_SLEEP_PROC_ALARM
not the current internal (acpi/sleep/proc.c) HAVE_ACPI_LEGACY_ALARM,
and some way should be found that the two interfaces can coexist for
a while.  But I can't quite make sense of the strategy for getting
rid of the /proc/acpi/* files ... for example, I see one entry
saying the ACPI procfs as a whole vanishes July 2007, and the next
one says that /proc/acpi/button stays until August 2007.

- Dave
 

Re: RTC_DRV_CMOS can break userspace interface

[Tino Keitel]

On Mon, May 28, 2007 at 14:06:52 -0700, David Brownell wrote:

[...]

> That seems to be true.  And those particular users should learn the
> portable /sys/class/rtc/rtc0/wakealarm syntax ... e.g. using numeric
> seconds-since-epoch ("date '+%s'") instead of strings the kernel needs
> to parse.  That way, they can start converting usage sooner.

Hi,

I use /proc/acpi/alarm a lot and tried to learn how to use
/sys/class/rtc/rtc0/wakealarm. However, I have no /sys/class/rtc/rtc0
until I load the rtc-test driver. I tried all RTC drivers that are
available in 2.6.21, without success:

$ grep RTC /boot/config-2.6.21
CONFIG_RTC=y
# CONFIG_HPET_RTC_IRQ is not set
# CONFIG_SND_RTCTIMER is not set
CONFIG_RTC_LIB=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_HCTOSYS=y
CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
# CONFIG_RTC_DEBUG is not set
# RTC interfaces
CONFIG_RTC_INTF_SYSFS=y
# CONFIG_RTC_INTF_PROC is not set
CONFIG_RTC_INTF_DEV=y
# CONFIG_RTC_INTF_DEV_UIE_EMUL is not set
# RTC drivers
CONFIG_RTC_DRV_CMOS=y
CONFIG_RTC_DRV_DS1553=m
CONFIG_RTC_DRV_DS1742=m
CONFIG_RTC_DRV_M48T86=m
CONFIG_RTC_DRV_TEST=m
CONFIG_RTC_DRV_V3020=m

In dmesg, I see these messages from RTC_DRV_CMOS:

rtc_cmos 00:09: rtc core: registered rtc_cmos as rtc0
rtc_cmos: probe of 00:09 failed with error -16
drivers/rtc/hctosys.c: unable to open rtc device (rtc0)

So all I can do is to continue to use /proc/acpi/alarm.

Regards,
Tino

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Thu, May 31, 2007 at 06:32:29AM +0200, Tino Keitel wrote:

> rtc_cmos 00:09: rtc core: registered rtc_cmos as rtc0
> rtc_cmos: probe of 00:09 failed with error -16
> drivers/rtc/hctosys.c: unable to open rtc device (rtc0)

Try unsetting CONFIG_RTC - you can't use the legacy driver at the same 
time as the new one.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[Tino Keitel]

On Thu, May 31, 2007 at 11:26:04 +0100, Matthew Garrett wrote:
> On Thu, May 31, 2007 at 06:32:29AM +0200, Tino Keitel wrote:
> 
> > rtc_cmos 00:09: rtc core: registered rtc_cmos as rtc0
> > rtc_cmos: probe of 00:09 failed with error -16
> > drivers/rtc/hctosys.c: unable to open rtc device (rtc0)
> 
> Try unsetting CONFIG_RTC - you can't use the legacy driver at the same 
> time as the new one.

Yes, you are right. I think this issue should be covered by Kconfig.

However:

$ cat wakealarm 
cat: wakealarm: Input/output error

It worked with /proc/acpi/alarm before.

Regards,
Tino

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Fri, Jun 01, 2007 at 09:46:06AM +0200, Tino Keitel wrote:
> Yes, you are right. I think this issue should be covered by Kconfig.
> 
> However:
> 
> $ cat wakealarm 
> cat: wakealarm: Input/output error
> 
> It worked with /proc/acpi/alarm before.

Can you do 

for i in /sys/bus/pnp/devices/*; do if [ "$(cat $i/id)" = PNP0b00 ]; 
then cat $i/resources; echo options; cat $i/options; fi; done

and provide the output? It sounds like you have the same problem I do, 
that is that you have no IRQ listed in the PNP table.

Though, actually, on reading the code:

The code for checking for a platform device only gets included if 
CONFIG_PNP isn't set! David, surely this should be a runtime thing 
rather than a compile-time one? Right now building a kernel with PNP 
support will break horribly if it's then run on a non-PNP system...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[David Brownell]

On Friday 01 June 2007, Matthew Garrett wrote:

> The code for checking for a platform device only gets included if 
> CONFIG_PNP isn't set! David, surely this should be a runtime thing 
> rather than a compile-time one? Right now building a kernel with PNP 
> support will break horribly if it's then run on a non-PNP system...

Maybe it should be a runtime thing.  But then, on x86 there's
nothing to create the platform device for this driver either...

I had posted code to create that platform device at one point,
adjacent to what creates the pcspeaker device, but after ACPI
changed to require PNPACPI it wasn't needed on any system I
could test with.  There are probably a bunch of platform_device
nodes that should be created automagically on old PNP-less PCs.

- Dave

Re: RTC_DRV_CMOS can break userspace interface

[Tino Keitel]

On Fri, Jun 01, 2007 at 13:54:23 +0100, Matthew Garrett wrote:
> On Fri, Jun 01, 2007 at 09:46:06AM +0200, Tino Keitel wrote:
> > Yes, you are right. I think this issue should be covered by Kconfig.
> > 
> > However:
> > 
> > $ cat wakealarm 
> > cat: wakealarm: Input/output error
> > 
> > It worked with /proc/acpi/alarm before.
> 
> Can you do 
> 
> for i in /sys/bus/pnp/devices/*; do if [ "$(cat $i/id)" = PNP0b00 ]; 
> then cat $i/resources; echo options; cat $i/options; fi; done

Here it is:

state = active
io 0x70-0x77
options

Regards,
Tino

Re: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Mon, Jun 04, 2007 at 11:35:18AM +0200, Tino Keitel wrote:

(By the way, it helps if you Cc: me - it's easy to lose track of things 
in the LKML noise)

> Here it is:
> 
> state = active
> io 0x70-0x77
> options

Yes, there's no IRQ listed. Can you try current git?
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: RTC_DRV_CMOS can break userspace interface

[Tino Keitel]

On Mon, Jun 04, 2007 at 13:14:58 +0100, Matthew Garrett wrote:
> On Mon, Jun 04, 2007 at 11:35:18AM +0200, Tino Keitel wrote:
>
> (By the way, it helps if you Cc: me - it's easy to lose track of
> things
> in the LKML noise)
>
> > Here it is:
> >
> > state = active
> > io 0x70-0x77
> > options
>
> Yes, there's no IRQ listed. Can you try current git?

OK, it works with git 99f9f3d49cbc7d944476f6fde53a77ec789ab2aa
(something after -rc4). I can echo something into
/sys/class/rtc/rtc0/wakealarm and the machine will wake up from suspend
to RAM.

However, a minor glitch remains:

$ echo 1181672013 > /sys/class/rtc/rtc0/wakealarm

- suspend

- wait for wakeup

After wakeup:

$ cat /sys/class/rtc/rtc0/wakealarm
2051656909

Regards,
Tino