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

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

[Imre Kaloz]

Two 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 repeats the PAB write, clearing the kickstart and wake-up
flags with it, until power is removed.

Patch 1 was tested on an O2, patch 2 on an Octane.

Imre Kaloz (2):
  rtc: ds1685: arm the wake-up alarm at power-off only if an alarm is
    set
  rtc: ds1685: keep setting PAB until power is removed

 drivers/rtc/rtc-ds1685.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.47.3

[PATCH 1/2] 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

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

[sashiko-bot]

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

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

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

[PATCH 2/2] 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 94c89333c5d5..a7828f0d6c09 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");
 		msleep(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);
+			msleep(100);
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(ds1685_rtc_poweroff);
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rtc: ds1685: sleep in atomic/poweroff context causes deadlock
--

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

rtc: ds1685: keep setting PAB until power is removed

This commit alters the ds1685_rtc_poweroff function to continuously clear
event flags and set the PAB bit until power is gone, replacing the previous
busy spin.

> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 94c89333c5d5c..a7828f0d6c09e 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");
>  		msleep(20);

[Severity: High]
This problem wasn't introduced by this patch, but ds1685_rtc_poweroff() is
used as a system power-off handler which executes at the end of the shutdown
sequence, often with preemption or local interrupts disabled.

Does this msleep() trigger a scheduling while atomic bug by delegating to
schedule_timeout_uninterruptible() in this context?

If local interrupts are disabled, could this also block indefinitely without
making progress because the scheduler timer tick will not be serviced?

> -		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);
> +			msleep(100);

[Severity: High]
This patch changes the original busy-spin loop into an infinite loop that
calls msleep(100).

Since this is executing in an atomic power-off context, won't this trigger
the same scheduling while atomic issue or system deadlock as mentioned
above?

Hardware power-off routines typically require busy-waits like mdelay()
instead of msleep() to avoid yielding the CPU while interrupts are disabled.

> +		}
>  	}
>  }
>  EXPORT_SYMBOL_GPL(ds1685_rtc_poweroff);

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