[PATCH v2 0/2] rtc: rv8803 patches

[PATCH v2 0/2] rtc: rv8803 patches

[Sascha Hauer]

This series has the remainder of
https://lore.kernel.org/all/20220426071056.1187235-1-s.hauer@pengutronix.de/
which was partly applied.

Sascha

Ahmad Fatoum (1):
  include/linux/bcd.h: provide bcd_is_valid() helper

Sascha Hauer (1):
  rtc: rv8803: invalidate date/time if alarm time is invalid

 drivers/rtc/rtc-rv8803.c | 45 +++++++++++++++++++++++++++++++++-------
 include/linux/bcd.h      |  4 ++++
 2 files changed, 42 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCH v2 1/2] include/linux/bcd.h: provide bcd_is_valid() helper

[Sascha Hauer]

From: Ahmad Fatoum <a.fatoum@pengutronix.de>

bcd2bin(0x0A) happily returns 10, despite this being an invalid BCD
value. RTC drivers converting possibly corrupted BCD timestamps might
want to validate their input before calling bcd2bin().

Provide a macro to do so. Unlike bcd2bin and bin2bcd, out-of-line
versions are not implemented. Should the macro experience enough use,
this can be retrofitted.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 include/linux/bcd.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/bcd.h b/include/linux/bcd.h
index 118bea36d7d49..abbc8149178e6 100644
--- a/include/linux/bcd.h
+++ b/include/linux/bcd.h
@@ -14,8 +14,12 @@
 		const_bin2bcd(x) :			\
 		_bin2bcd(x))
 
+#define bcd_is_valid(x)					\
+		const_bcd_is_valid(x)
+
 #define const_bcd2bin(x)	(((x) & 0x0f) + ((x) >> 4) * 10)
 #define const_bin2bcd(x)	((((x) / 10) << 4) + (x) % 10)
+#define const_bcd_is_valid(x)	(((x) & 0x0f) < 10 && ((x) >> 4) < 10)
 
 unsigned _bcd2bin(unsigned char val) __attribute_const__;
 unsigned char _bin2bcd(unsigned val) __attribute_const__;
-- 
2.30.2

[PATCH v2 2/2] rtc: rv8803: invalidate date/time if alarm time is invalid

[Sascha Hauer]

RTC core never calls rv8803_set_alarm with an invalid alarm time,
so if an invalid alarm time > 0 is set, external factors must have
corrupted the RTC's alarm time and possibly other registers.

Play it safe by marking the date/time invalid, so all registers are
reinitialized on a ->set_time.

This may cause existing setups to lose time if they so far set only
date/time, but ignored that the alarm registers had an invalid date
value, e.g.:

  rtc rtc0: invalid alarm value: 2020-3-27 7:82:0

These systems will have their ->get_time return -EINVAL till
->set_time initializes the alarm value (and sets a new time).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---

Notes:
    Changes since v1:
    - set alarm_invalid directly when one of the alarmvals has invalid BCD
    - cast to (unsigned int) rather than (unsigned)

 drivers/rtc/rtc-rv8803.c | 45 +++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
index 3527a0521e9b2..4875728014bed 100644
--- a/drivers/rtc/rtc-rv8803.c
+++ b/drivers/rtc/rtc-rv8803.c
@@ -70,6 +70,7 @@ struct rv8803_data {
 	struct mutex flags_lock;
 	u8 ctrl;
 	u8 backup;
+	u8 alarm_invalid:1;
 	enum rv8803_type type;
 };
 
@@ -165,13 +166,13 @@ static int rv8803_regs_init(struct rv8803_data *rv8803)
 
 static int rv8803_regs_configure(struct rv8803_data *rv8803);
 
-static int rv8803_regs_reset(struct rv8803_data *rv8803)
+static int rv8803_regs_reset(struct rv8803_data *rv8803, bool full)
 {
 	/*
 	 * The RV-8803 resets all registers to POR defaults after voltage-loss,
 	 * the Epson RTCs don't, so we manually reset the remainder here.
 	 */
-	if (rv8803->type == rx_8803 || rv8803->type == rx_8900) {
+	if (full || rv8803->type == rx_8803 || rv8803->type == rx_8900) {
 		int ret = rv8803_regs_init(rv8803);
 		if (ret)
 			return ret;
@@ -238,6 +239,11 @@ static int rv8803_get_time(struct device *dev, struct rtc_time *tm)
 	u8 *date = date1;
 	int ret, flags;
 
+	if (rv8803->alarm_invalid) {
+		dev_warn(dev, "Corruption detected, data may be invalid.\n");
+		return -EINVAL;
+	}
+
 	flags = rv8803_read_reg(rv8803->client, RV8803_FLAG);
 	if (flags < 0)
 		return flags;
@@ -313,12 +319,19 @@ static int rv8803_set_time(struct device *dev, struct rtc_time *tm)
 		return flags;
 	}
 
-	if (flags & RV8803_FLAG_V2F) {
-		ret = rv8803_regs_reset(rv8803);
+	if ((flags & RV8803_FLAG_V2F) || rv8803->alarm_invalid) {
+		/*
+		 * If we sense corruption in the alarm registers, but see no
+		 * voltage loss flag, we can't rely on other registers having
+		 * sensible values. Reset them fully.
+		 */
+		ret = rv8803_regs_reset(rv8803, rv8803->alarm_invalid);
 		if (ret) {
 			mutex_unlock(&rv8803->flags_lock);
 			return ret;
 		}
+
+		rv8803->alarm_invalid = false;
 	}
 
 	ret = rv8803_write_reg(rv8803->client, RV8803_FLAG,
@@ -344,15 +357,33 @@ static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	if (flags < 0)
 		return flags;
 
+	alarmvals[0] &= 0x7f;
+	alarmvals[1] &= 0x3f;
+	alarmvals[2] &= 0x3f;
+
+	if (!bcd_is_valid(alarmvals[0]) ||
+	    !bcd_is_valid(alarmvals[1]) ||
+	    !bcd_is_valid(alarmvals[2]))
+		goto err_invalid;
+
 	alrm->time.tm_sec  = 0;
-	alrm->time.tm_min  = bcd2bin(alarmvals[0] & 0x7f);
-	alrm->time.tm_hour = bcd2bin(alarmvals[1] & 0x3f);
-	alrm->time.tm_mday = bcd2bin(alarmvals[2] & 0x3f);
+	alrm->time.tm_min  = bcd2bin(alarmvals[0]);
+	alrm->time.tm_hour = bcd2bin(alarmvals[1]);
+	alrm->time.tm_mday = bcd2bin(alarmvals[2]);
 
 	alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
 	alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
 
+	if ((unsigned int)alrm->time.tm_mday > 31 ||
+	    (unsigned int)alrm->time.tm_hour >= 24 ||
+	    (unsigned int)alrm->time.tm_min >= 60)
+		goto err_invalid;
+
 	return 0;
+
+err_invalid:
+	rv8803->alarm_invalid = true;
+	return -EINVAL;
 }
 
 static int rv8803_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
-- 
2.30.2

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Sascha Hauer]

Hi Alexandre,

Any input to this series?

Sascha

On Wed, Aug 17, 2022 at 10:53:28AM +0200, Sascha Hauer wrote:
> This series has the remainder of
> https://lore.kernel.org/all/20220426071056.1187235-1-s.hauer@pengutronix.de/
> which was partly applied.
> 
> Sascha
> 
> Ahmad Fatoum (1):
>   include/linux/bcd.h: provide bcd_is_valid() helper
> 
> Sascha Hauer (1):
>   rtc: rv8803: invalidate date/time if alarm time is invalid
> 
>  drivers/rtc/rtc-rv8803.c | 45 +++++++++++++++++++++++++++++++++-------
>  include/linux/bcd.h      |  4 ++++
>  2 files changed, 42 insertions(+), 7 deletions(-)
> 
> -- 
> 2.30.2
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Alexandre Belloni]

Hi,

On 21/09/2022 15:17:53+0200, Sascha Hauer wrote:
> Hi Alexandre,
> 
> Any input to this series?

I'm not convinced this is necessary. Having an invalid alarm doesn't
mean that the time is invalid and that check will only ever happen at
boot time whereas V2F is a reliable indication that the time is invalid.

Have you really had an RTC with an invalid time that is not caught by
rtc_valid_tm and with V2F not set?


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Sascha Hauer]

On Wed, Sep 21, 2022 at 04:22:09PM +0200, Alexandre Belloni wrote:
> Hi,
> 
> On 21/09/2022 15:17:53+0200, Sascha Hauer wrote:
> > Hi Alexandre,
> > 
> > Any input to this series?
> 
> I'm not convinced this is necessary. Having an invalid alarm doesn't
> mean that the time is invalid and that check will only ever happen at
> boot time whereas V2F is a reliable indication that the time is invalid.
> 
> Have you really had an RTC with an invalid time that is not caught by
> rtc_valid_tm and with V2F not set?

I don't know. I must talk to Ahmad in this regard, he'll be back next
week. It could be that we only created this patch to be sure the RTC
state is sane.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Ahmad Fatoum]

Hello Alexandre,
Hello Sascha,

On 21.09.22 15:35, Sascha Hauer wrote:
> On Wed, Sep 21, 2022 at 04:22:09PM +0200, Alexandre Belloni wrote:
>> Hi,
>>
>> On 21/09/2022 15:17:53+0200, Sascha Hauer wrote:
>>> Hi Alexandre,
>>>
>>> Any input to this series?
>>
>> I'm not convinced this is necessary. Having an invalid alarm doesn't
>> mean that the time is invalid and that check will only ever happen at
>> boot time whereas V2F is a reliable indication that the time is invalid.
>>
>> Have you really had an RTC with an invalid time that is not caught by
>> rtc_valid_tm and with V2F not set?
> 
> I don't know. I must talk to Ahmad in this regard, he'll be back next
> week. It could be that we only created this patch to be sure the RTC
> state is sane.

The kernel message

  rtc rtc0: invalid alarm value: 2020-3-27 7:82:0

listed in the commit message is something I actually ran into. There
was no v2f set then. The customer has also variously observed bit flips
independently of v2f: During EMC testing, electrostatic discharge at developer
desks and even in the field: Suspected causes were lightning strikes in the
vicinity and the switching of larger inductive loads.
They're very paranoid of logging invalid timestamps, so we'll keep the patch
anyhow at our side, but I think it is generally useful as well: If we can't
set an invalid alarm time by normal means, but read back an invalid time,
something may have corrupted other memory, so treating it as a v2f is sensible.

Thanks,
Ahmad

> 
> Sascha
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2035 bytes --]

Hi Alexandre,

On 26.09.2022 11:23:13, Ahmad Fatoum wrote:
> Hello Alexandre,
> Hello Sascha,
> 
> On 21.09.22 15:35, Sascha Hauer wrote:
> > On Wed, Sep 21, 2022 at 04:22:09PM +0200, Alexandre Belloni wrote:
> >> Hi,
> >>
> >> On 21/09/2022 15:17:53+0200, Sascha Hauer wrote:
> >>> Hi Alexandre,
> >>>
> >>> Any input to this series?
> >>
> >> I'm not convinced this is necessary. Having an invalid alarm doesn't
> >> mean that the time is invalid and that check will only ever happen at
> >> boot time whereas V2F is a reliable indication that the time is invalid.
> >>
> >> Have you really had an RTC with an invalid time that is not caught by
> >> rtc_valid_tm and with V2F not set?
> > 
> > I don't know. I must talk to Ahmad in this regard, he'll be back next
> > week. It could be that we only created this patch to be sure the RTC
> > state is sane.
> 
> The kernel message
> 
>   rtc rtc0: invalid alarm value: 2020-3-27 7:82:0
> 
> listed in the commit message is something I actually ran into. There
> was no v2f set then. The customer has also variously observed bit flips
> independently of v2f: During EMC testing, electrostatic discharge at developer
> desks and even in the field: Suspected causes were lightning strikes in the
> vicinity and the switching of larger inductive loads.
> They're very paranoid of logging invalid timestamps, so we'll keep the patch
> anyhow at our side, but I think it is generally useful as well: If we can't
> set an invalid alarm time by normal means, but read back an invalid time,
> something may have corrupted other memory, so treating it as a v2f is sensible.

Should we re-send the patch with an updated patch description, or do you
take it as is?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/2] rtc: rv8803 patches

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]

Hello Alexandre,

On 06.10.2022 13:53:46, Marc Kleine-Budde wrote:
> Hi Alexandre,
> 
> On 26.09.2022 11:23:13, Ahmad Fatoum wrote:
> > Hello Alexandre,
> > Hello Sascha,
> > 
> > On 21.09.22 15:35, Sascha Hauer wrote:
> > > On Wed, Sep 21, 2022 at 04:22:09PM +0200, Alexandre Belloni wrote:
> > >> Hi,
> > >>
> > >> On 21/09/2022 15:17:53+0200, Sascha Hauer wrote:
> > >>> Hi Alexandre,
> > >>>
> > >>> Any input to this series?
> > >>
> > >> I'm not convinced this is necessary. Having an invalid alarm doesn't
> > >> mean that the time is invalid and that check will only ever happen at
> > >> boot time whereas V2F is a reliable indication that the time is invalid.
> > >>
> > >> Have you really had an RTC with an invalid time that is not caught by
> > >> rtc_valid_tm and with V2F not set?
> > > 
> > > I don't know. I must talk to Ahmad in this regard, he'll be back next
> > > week. It could be that we only created this patch to be sure the RTC
> > > state is sane.
> > 
> > The kernel message
> > 
> >   rtc rtc0: invalid alarm value: 2020-3-27 7:82:0
> > 
> > listed in the commit message is something I actually ran into. There
> > was no v2f set then. The customer has also variously observed bit flips
> > independently of v2f: During EMC testing, electrostatic discharge at developer
> > desks and even in the field: Suspected causes were lightning strikes in the
> > vicinity and the switching of larger inductive loads.
> > They're very paranoid of logging invalid timestamps, so we'll keep the patch
> > anyhow at our side, but I think it is generally useful as well: If we can't
> > set an invalid alarm time by normal means, but read back an invalid time,
> > something may have corrupted other memory, so treating it as a v2f is sensible.
> 
> Should we re-send the patch with an updated patch description, or do you
> take it as is?

As v6.1-rc1 just came out, what about this patch?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]