RTC_DRV_CMOS can break userspace interface
Subsystems (2)
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)
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 3 lines · Matthew Garrett
(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=yCONFIG_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...
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 4 lines · Matthew Garrett
but will disable
/proc/acpi/wakeup. It'll also be impossible to loadCONFIG_RTC_CMOSbecauseCONFIG_RTChas 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?
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 7 lines · Matthew Garrett
Quoted 4 lines · Matthew Garrett
but will disable
/proc/acpi/wakeup. It'll also be impossible to loadCONFIG_RTC_CMOSbecauseCONFIG_RTChas 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...
Quoted 6 lines
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/alarminterface.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
Quoted 11 lines · David Brownell
Quoted 6 lines · Matthew Garrett
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/alarminterface.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.
Signature · 1 line
Matthew Garrett | [email protected]
Quoted 10 lines · Matthew Garrett <mjg59 <at> srcf.ucam.org> writes:
Quoted 8 lines · David Brownell
Quoted 6 lines · Matthew Garrett
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/alarminterface.David, would you be happy with hardcoding the rtc-cmos IRQ to 8 on PCs if there's inadequate PNP information available?
Quoted 9 lines
Quoted 3 lines
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
Quoted 2 lines · Matthieu CASTET
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.
Signature · 1 line
Matthew Garrett | [email protected]
PATCH 1/1 RTC: Use fallback IRQ if PNP tables don't provide one
In reply to David BrownellFrom: Matthew Garrett <[email protected]>
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.
---
drivers/rtc/rtc-cmos.c +10 -3
@@ -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)
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one
Hi,
Quoted 6 lines · Matthew Garrett
From: Matthew Garrett <[email protected]>
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
Quoted 13 lines · On Mon, 28 May 2007 18:50:22 +0000 (UTC) Matthieu CASTET…
Hi,
Quoted 6 lines · Matthew Garrett
From: Matthew Garrett <[email protected]>
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 :
Quoted 9 lines
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
Quoted 2 lines · Andrew Morton
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.
Signature · 1 line
Matthew Garrett | [email protected]
Re: [PATCH] RTC: Use fallback IRQ if PNP tables don't provide one
In reply to Matthew GarrettQuoted 8 lines · Matthew Garrett
From: Matthew Garrett <[email protected]>
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 <[email protected]>
Quoted 31 lines
---
drivers/rtc/rtc-cmos.c +10
-3@@ -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)Signature · 2 lines
-- Matthew Garrett | [email protected]
Quoted 3 lines · Matthew Garrett
f5f72b46c349fefcfd4421b2213c6ffb324c5e56appears 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.
Quoted 2 lines
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.
Quoted 3 lines
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