[PATCH v2 0/3] rtc: ds1685: power-off fixes

[PATCH v2 0/3] rtc: ds1685: power-off fixes

[Imre Kaloz]

Three fixes for ds1685_rtc_poweroff(), on SGI O2 and Octane machines.

Patch 1 arms the wake-up alarm only if an alarm is set, so that stale
alarm registers no longer power the machine back on.

Patch 2 replaces the sleep before the PAB write with a busy wait, as
power-off handlers must not sleep.

Patch 3 repeats the PAB write, clearing the kickstart and wake-up
flags with it, until power is removed.

Patch 1 was tested on an O2, and the whole series on an Octane.

Changes in v2:
- New patch 2: use mdelay() instead of msleep() before the PAB write.
- Patch 3 (v1 patch 2): use mdelay() instead of msleep() in the
  repeat loop.

v1: https://lore.kernel.org/all/20261013094930.574-1-kaloz@kernel.org/

Imre Kaloz (3):
  rtc: ds1685: arm the wake-up alarm at power-off only if an alarm is
    set
  rtc: ds1685: do not sleep in the power-off handler
  rtc: ds1685: keep setting PAB until power is removed

 drivers/rtc/rtc-ds1685.c | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.47.3

[PATCH v2 1/3] rtc: ds1685: arm the wake-up alarm at power-off only if an alarm is set

[Imre Kaloz]

ds1685_rtc_poweroff() always sets WIE. The wake-up alarm then compares
the time-of-day and date alarm registers, which hold whatever was last
written to them, and a match powers the machine straight back on.

Set WIE only when AIE is set in Control B, and clear it otherwise.

Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
Signed-off-by: Imre Kaloz <kaloz@kernel.org>
---
 drivers/rtc/rtc-ds1685.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 5fc8e36b1abf..94c89333c5d5 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1367,7 +1367,7 @@ module_platform_driver(ds1685_rtc_driver);
 void __noreturn
 ds1685_rtc_poweroff(struct platform_device *pdev)
 {
-	u8 ctrla, ctrl4a, ctrl4b;
+	u8 ctrla, ctrlb, ctrl4a, ctrl4b;
 	struct ds1685_priv *rtc;
 
 	/* Check for valid RTC data, else, spin forever. */
@@ -1394,6 +1394,8 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
 		ctrla &= ~(RTC_CTRL_A_DV2);
 		rtc->write(rtc, RTC_CTRL_A, ctrla);
 
+		ctrlb = rtc->read(rtc, RTC_CTRL_B);
+
 		/*
 		 * Read Control 4A and check the status of the auxillary
 		 * battery.  This must be present and working (VRT2 = 1)
@@ -1408,13 +1410,17 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
 
 			/*
 			 * The auxillary battery is present and working.
-			 * Enable extended functions (ABE=1), enable
-			 * wake-up (WIE=1), and enable kickstart (KSE=1)
-			 * in Control 4B.
+			 * Enable extended functions (ABE=1) and kickstart
+			 * (KSE=1) in Control 4B.  Enable wake-up (WIE=1) only
+			 * if an alarm is set, else stale alarm registers
+			 * can power the system back on.
 			 */
 			ctrl4b = rtc->read(rtc, RTC_EXT_CTRL_4B);
-			ctrl4b |= (RTC_CTRL_4B_ABE | RTC_CTRL_4B_WIE |
-				   RTC_CTRL_4B_KSE);
+			ctrl4b |= (RTC_CTRL_4B_ABE | RTC_CTRL_4B_KSE);
+			if (ctrlb & RTC_CTRL_B_AIE)
+				ctrl4b |= RTC_CTRL_4B_WIE;
+			else
+				ctrl4b &= ~(RTC_CTRL_4B_WIE);
 			rtc->write(rtc, RTC_EXT_CTRL_4B, ctrl4b);
 		}
 
-- 
2.47.3

[PATCH v2 2/3] rtc: ds1685: do not sleep in the power-off handler

[Imre Kaloz]

Power-off handlers run from an atomic notifier chain and must not
sleep. Use mdelay() for the delay before setting PAB.

Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/sashiko-outbox-169477@kernel.org/
Signed-off-by: Imre Kaloz <kaloz@kernel.org>
---
 drivers/rtc/rtc-ds1685.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 94c89333c5d5..123ab9fc9c8d 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1426,7 +1426,7 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
 
 		/* Set PAB to 1 in Control 4A to power the system down. */
 		dev_warn(&pdev->dev, "Powerdown.\n");
-		msleep(20);
+		mdelay(20);
 		rtc->write(rtc, RTC_EXT_CTRL_4A,
 			   (ctrl4a | RTC_CTRL_4A_PAB));
 
-- 
2.47.3

[PATCH v2 3/3] rtc: ds1685: keep setting PAB until power is removed

[Imre Kaloz]

A kickstart or wake-up flag set while its interrupt is enabled (KF with
KSE, WF with WIE) clears PAB, and the system stays powered. Clear the
event flags together with setting PAB, and repeat the write until power
is gone.

Signed-off-by: Imre Kaloz <kaloz@kernel.org>
---
 drivers/rtc/rtc-ds1685.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 123ab9fc9c8d..421936e3bd74 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1424,15 +1424,21 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
 			rtc->write(rtc, RTC_EXT_CTRL_4B, ctrl4b);
 		}
 
-		/* Set PAB to 1 in Control 4A to power the system down. */
+		/*
+		 * Set PAB to 1 in Control 4A to power the system down.  PAB
+		 * is cleared again while KF and KSE, or WF and WIE, are set,
+		 * so keep clearing the flags and setting PAB until power is
+		 * gone.  We do not switch back to bank0.
+		 */
 		dev_warn(&pdev->dev, "Powerdown.\n");
 		mdelay(20);
-		rtc->write(rtc, RTC_EXT_CTRL_4A,
-			   (ctrl4a | RTC_CTRL_4A_PAB));
-
-		/* Spin ... we do not switch back to bank0. */
-		while(1);
-		unreachable();
+		while (1) {
+			ctrl4a = rtc->read(rtc, RTC_EXT_CTRL_4A);
+			rtc->write(rtc, RTC_EXT_CTRL_4A,
+				   (ctrl4a & ~RTC_CTRL_4A_RWK_MASK) |
+				   RTC_CTRL_4A_PAB);
+			mdelay(100);
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(ds1685_rtc_poweroff);
-- 
2.47.3

Re: [PATCH v2 1/3] rtc: ds1685: arm the wake-up alarm at power-off only if an alarm is set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: ds1685: NULL pointer deref in poweroff handler if unbound
--

commit e81b71e9197dea1b3b5991740fdeeb891e509522
Author: Imre Kaloz <kaloz@kernel.org>

rtc: ds1685: arm the wake-up alarm at power-off only if an alarm is set

This commit modifies the DS1685 RTC driver to only arm the wake-up alarm
during power-off if an alarm is actually set. It prevents stale alarm
registers from inadvertently powering the machine back on.

> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 5fc8e36b1abf8..94c89333c5d5c 100644
> --- a/drivers/rtc/rtc-ds1685.c
> +++ b/drivers/rtc/rtc-ds1685.c
> @@ -1367,7 +1367,7 @@ module_platform_driver(ds1685_rtc_driver);
>  void __noreturn
>  ds1685_rtc_poweroff(struct platform_device *pdev)
>  {
> -	u8 ctrla, ctrl4a, ctrl4b;
> +	u8 ctrla, ctrlb, ctrl4a, ctrl4b;
>  	struct ds1685_priv *rtc;
>  
>  	/* Check for valid RTC data, else, spin forever. */

[ ... ]

> @@ -1394,6 +1394,8 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
>  		ctrla &= ~(RTC_CTRL_A_DV2);
>  		rtc->write(rtc, RTC_CTRL_A, ctrla);
>  
> +		ctrlb = rtc->read(rtc, RTC_CTRL_B);

[Severity: High]
Can this lead to a NULL pointer dereference during the system poweroff
sequence?

If the driver failed to probe or was explicitly unbound via sysfs,
platform_get_drvdata(pdev) will return NULL. When the global shutdown handler
calls ds1685_rtc_poweroff(), we fetch the driver data and dereference it
without checking:

        /* Get the rtc data. */
        rtc = platform_get_drvdata(pdev);

        /* ... */
        if (rtc->irq_num)
                disable_irq_nosync(rtc->irq_num);

        ctrla = rtc->read(rtc, RTC_CTRL_A);

Could this hang the system instead of powering off by causing a panic when rtc
is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120208.11268-1-kaloz@kernel.org?part=1

Re: [PATCH v2 2/3] rtc: ds1685: do not sleep in the power-off handler

[sashiko-bot]

> Power-off handlers run from an atomic notifier chain and must not
> sleep. Use mdelay() for the delay before setting PAB.
> 
> Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/sashiko-outbox-169477@kernel.org/
> Signed-off-by: Imre Kaloz <kaloz@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120208.11268-1-kaloz@kernel.org?part=2

Re: [PATCH v2 3/3] rtc: ds1685: keep setting PAB until power is removed

[sashiko-bot]

> A kickstart or wake-up flag set while its interrupt is enabled (KF with
> KSE, WF with WIE) clears PAB, and the system stays powered. Clear the
> event flags together with setting PAB, and repeat the write until power
> is gone.
> 
> Signed-off-by: Imre Kaloz <kaloz@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120208.11268-1-kaloz@kernel.org?part=3