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

[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