RTC `wakealarm` is improperly deleted from `sysfs`

seamusdemora commented 1 month ago

Describe the bug

A future time value stored in /sys/class/rtc/rtc0/wakealarm is deleted improperly; e.g. following a halt and re-start of the Raspberry Pi.

Steps to reproduce the behaviour

Set the wakealarm (in sysfs) to a value in the future, let's call it Tf, and let's assume Tf is 24 hours beyond the time at which wakealarm is set; i.e. date '+%s' -d '+ 24 hours' | sudo tee /sys/class/rtc/rtc0/wakealarm;
- IOW: Tf = date '+%s' -d '+ 24 hours'
- Let's assume the value of Tf = 1720585180
Shortly (e.g. 30 minutes) after setting wakealarm as in Step 1, issue a halt from the command line.

NOTE A: I have the gpio-poweroff overlay set in the config.txt file; i.e. gpio-poweroff "fires" after the halt is issued in Step 2. However, this does not appear to be material to this bug report as I have removed the gpio-poweroff overlay from config.txt, and it makes no difference in the result.

NOTE B: A reboot has the same effect on wakealarm as a halt and re-start.

Shortly (e.g. 30 minutes) after issuing the halt in Step 2, power is "re-connected" to the RPi, and it boots.
After booting, check the value stored in wakealarm. You will find there is nothing stored in wakealarm.
This is not a case of the wakealarm value still being retained in the DS3231 chip register; i.e. this is NOT a case of /sys/class/rtc/rtc0/wakealarm = NULL, but the chip register still contains the previously programmed value. I've confirmed that the alarm simply does not go off / no #INTerrupt; i.e. the Tf value has apparently been removed from the DS3231 register by the driver.

Device (s)

Raspberry Pi Zero W / WH

System

cat /etc/rpi-issue ==> Raspberry Pi reference 2022-01-28 Generated using pi-gen, https://github.com/RPi-Distro/pi-gen, f01430c9d8f67a4b9719cc00e74a2079d3834d5d, stage2

uname -a ==> Linux raspberrypi0w 6.1.21+ #1642 Mon Apr 3 17:19:14 BST 2023 armv6l GNU/Linux

Logs

No response

Additional context

What I expected to happen:

In Step 4 (Steps to reproduce behavior), I expected the wakealarm setting (Tf = 1720585180) to be available by reading wakealarm (via cat wakealarm). I initially assumed this was just a case that the driver was not updating the wakealarm value from the DS3231 register. However, further experiments revealed this to not be the case. Apparently the driver "zeros out" the DS3231's register at some time after the halt or subsequent boot. This of course results in no alarm/#INTerrupt.

pelwell commented 1 month ago

This behaviour is not going to be unique to the Raspberry Pi kernel. You should email linux-rtc@vger.kernel.org.

If you think gpio-poweroff may be playing a part, try removing it.

seamusdemora commented 1 month ago

Thanks - Edited bug report.

I've emailed kernel.org; do you agree that this is something that should be fixed?

pelwell commented 1 month ago

I don't have an opinion on it, having not played with wakealarms enough to know what the behaviour should be.

seamusdemora commented 1 month ago

@pelwell

FWIW, I posted twice, but had zero luck raising anyone on the mail list atlinux-rtc@vger.kernel.org. It seems this site is used for submitting patches - no discussion at all. Just FYI.

6by9 commented 1 month ago

Isn't the driver doing exactly what it says it's doing in the comment at https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/rtc/rtc-ds1307.c#L1799-L1804

         * Using IRQ or defined as wakeup-source?
         * Disable the square wave and both alarms.
...
            regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);

The sysfs show hook doesn't display disabled alarms as https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/rtc/sysfs.c#L134-L136

    /* Don't show disabled alarms.  For uniformity, RTC alarms are
     * conceptually one-shot, even though some common RTCs (on PCs)
     * don't actually work that way.

and ds1337_read_alarm won't have set enabled due to the interrupt being disabled.

Are there any other RTC drivers that do retain alarm status over a reboot? I don't know enough about the subsystem to say which is the expected behaviour.

Whenever emailing lists about a specific driver, it's always worth including the driver's maintainers as reported by scripts/get_maintainer.pl (or a quick search in the raw MAINTAINERS file)

seamusdemora commented 3 weeks ago

Isn't the driver doing exactly what it says it's doing in the comment at https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/rtc/rtc-ds1307.c#L1799-L1804
       * Using IRQ or defined as wakeup-source?
       * Disable the square wave and both alarms.
...
          regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);

I've read this comment four times now over two days, and its meaning remains unclear to me ... "IRQ OR wakeup-source..." What does that mean? The pin (on a DS3231) is labeled #INT/SQW - meaning that it either puts out a 1 PPS SQuare Wave - OR - it is pulled low (from a pulled-up state) at the alarm time. That being the case, what does the author mean when he says, "IRQ or defined as wakeup-source"?

That said, I have difficulty in making the connection you propose; i.e. that the comment implies that the alarm time should be/is reset to zero during a reboot. In the first place: I can't see why anyone would want to do that. For example, I might have an alarm set for 3 days from today; why would I want to reset it each time I have to reboot (or halt)?

WRT your question:

Are there any other RTC drivers that do retain alarm status over a reboot?

I can't answer that as this is the only RTC driver I've ever used.

pelwell commented 3 weeks ago

[ Offensive comment above deleted ]

pelwell commented 3 weeks ago

That's quite enough time spent on a question for upstream.

seamusdemora commented 1 week ago

I'd like to thank you for all the help. It's a small thanks - the best I can do.

raspberrypi / linux