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

Re: RTC_DRV_CMOS can break userspace interface

[Matthieu CASTET]

Matthew Garrett <mjg59 <at> srcf.ucam.org> writes:

> 
> 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.
> 
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: RTC_DRV_CMOS can break userspace interface

[Matthew Garrett]

On Mon, May 28, 2007 at 07:45:33AM +0000, Matthieu CASTET wrote:

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

Yup. The options file is empty, and the resources file doesn't contain 
an IRQ.

-- 
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