[PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

Make sure to disable the alarm before updating the four alarm time
registers to avoid spurious alarms during the update.

Note that the disable needs to be done outside of the ctrl_reg_lock
section to prevent a racing alarm interrupt from disabling the newly set
alarm when the lock is released.

Fixes: 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
Cc: stable@vger.kernel.org      # 3.1
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/rtc/rtc-pm8xxx.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 716e5d9ad74d..d114f0da537d 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -221,7 +221,6 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
 	int rc, i;
 	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned int ctrl_reg;
 	unsigned long secs, irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
@@ -233,6 +232,11 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		secs >>= 8;
 	}
 
+	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+				regs->alarm_en, 0);
+	if (rc)
+		return rc;
+
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
@@ -242,19 +246,11 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		goto rtc_rw_fail;
 	}
 
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-	if (rc)
-		goto rtc_rw_fail;
-
-	if (alarm->enabled)
-		ctrl_reg |= regs->alarm_en;
-	else
-		ctrl_reg &= ~regs->alarm_en;
-
-	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-	if (rc) {
-		dev_err(dev, "Write to RTC alarm control register failed\n");
-		goto rtc_rw_fail;
+	if (alarm->enabled) {
+		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+					regs->alarm_en, regs->alarm_en);
+		if (rc)
+			goto rtc_rw_fail;
 	}
 
 	dev_dbg(dev, "Alarm Set for h:m:s=%ptRt, y-m-d=%ptRdr\n",
-- 
2.39.1

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> Make sure to disable the alarm before updating the four alarm time
> registers to avoid spurious alarms during the update.

What scenario can encounter a spurious alarm triggering upon writing the
new alarm time inside of pm8xxx_rtc_set_alarm()?

> Note that the disable needs to be done outside of the ctrl_reg_lock
> section to prevent a racing alarm interrupt from disabling the newly set
> alarm when the lock is released.

What scenario shows the IRQ race issue that you mentioned?  How does not
protecting this register write with a lock avoid the race condition?

> Fixes: 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
> Cc: stable@vger.kernel.org      # 3.1
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)

Note that since locking is removed later in the patch series, my
questions above are mainly for the sake of curiosity.


Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Thanks,
David

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:12:43PM -0800, David Collins wrote:
> On 2/2/23 07:54, Johan Hovold wrote:
> > Make sure to disable the alarm before updating the four alarm time
> > registers to avoid spurious alarms during the update.
> 
> What scenario can encounter a spurious alarm triggering upon writing the
> new alarm time inside of pm8xxx_rtc_set_alarm()?

The alarm is stored in four bytes in little-endian order. Consider
having had an alarm set and expired at:

	00 01 00 00

and now you want to set an alarm at

	01 02 00 00

Unless the alarm is disabled before the update the alarm could go off at

	01 01 00 00

after updating the first byte.
 
> > Note that the disable needs to be done outside of the ctrl_reg_lock
> > section to prevent a racing alarm interrupt from disabling the newly set
> > alarm when the lock is released.
> 
> What scenario shows the IRQ race issue that you mentioned?  How does not
> protecting this register write with a lock avoid the race condition?

If a previously set alarm goes off after disabling interrupts but before
disabling the alarm inside the critical section, then that interrupt
could be serviced as soon as interrupts are re-enabled and the handler
would disable the newly set alarm.

> > Fixes: 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
> > Cc: stable@vger.kernel.org      # 3.1
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/rtc/rtc-pm8xxx.c | 24 ++++++++++--------------
> >  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> Note that since locking is removed later in the patch series, my
> questions above are mainly for the sake of curiosity.

Johan

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

On Tue, Feb 07, 2023 at 04:13:36PM +0100, Johan Hovold wrote:
> On Mon, Feb 06, 2023 at 07:12:43PM -0800, David Collins wrote:
> > On 2/2/23 07:54, Johan Hovold wrote:
> > > Make sure to disable the alarm before updating the four alarm time
> > > registers to avoid spurious alarms during the update.
> > 
> > What scenario can encounter a spurious alarm triggering upon writing the
> > new alarm time inside of pm8xxx_rtc_set_alarm()?
> 
> The alarm is stored in four bytes in little-endian order. Consider
> having had an alarm set and expired at:

This was just supposed to say "Consider having an alarm set at:" as the
alarm must still be enabled. Let me update the example I gave:

Consider having an alarm set at
 
 	10 01 00 00

and now you want to set an alarm at

 	01 02 00 00
 
Unless the alarm is disabled before the update the alarm could go off at
 
 	01 01 00 00
 
after updating the first byte.

Johan