[PATCH RESEND 0/2] rtc: rv8803 patches

[PATCH RESEND 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.

Alexandre,

Last time this series was send you asked if this series fixes a problem
we've really seen to which Ahmad answered:

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

There was no answer to this. I would be glad if you could take this
series. I would understand though if you say that this problem is too
esoteric to fix it upstream, we would keep the patches locally then.
Please just say so, it would help me to get the problem from my desk ;)

Thanks,
  Sascha



Ahman 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 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>
Signed-off-by: Sascha Hauer <s.hauer@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 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>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 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 RESEND 0/2] rtc: rv8803 patches

[Marc Kleine-Budde]

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

Hey Alexandre,

On 23.11.2022 10:55:25, 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.
> 
> Alexandre,
> 
> Last time this series was send you asked if this series fixes a problem
> we've really seen to which Ahmad answered:
> 
> > 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.
> 
> There was no answer to this. I would be glad if you could take this
> series. I would understand though if you say that this problem is too
> esoteric to fix it upstream, we would keep the patches locally then.
> Please just say so, it would help me to get the problem from my desk ;)

Any news on this, what's the status?

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 RESEND 0/2] rtc: rv8803 patches

[Marc Kleine-Budde]

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

On 23.11.2022 10:55:25, 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.
> 
> Alexandre,
> 
> Last time this series was send you asked if this series fixes a problem
> we've really seen to which Ahmad answered:

Happy new Year Alexandre!

What can we do to bring this patch froward?

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 RESEND 0/2] rtc: rv8803 patches

[Marc Kleine-Budde]

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

Hello Alexandre,

On 23.11.2022 10:55:25, 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.
> 
> Alexandre,
> 
> Last time this series was send you asked if this series fixes a problem
> we've really seen to which Ahmad answered:
> 
> > 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.
> 
> There was no answer to this. I would be glad if you could take this
> series. I would understand though if you say that this problem is too
> esoteric to fix it upstream, we would keep the patches locally then.
> Please just say so, it would help me to get the problem from my desk
> ;)

Can someone take this patch series? If not, what can we do to get these
changes upstream?

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 RESEND 0/2] rtc: rv8803 patches

[Alexandre Belloni]

On 31/01/2023 09:19:55+0100, Marc Kleine-Budde wrote:
> Hello Alexandre,
> 
> On 23.11.2022 10:55:25, 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.
> > 
> > Alexandre,
> > 
> > Last time this series was send you asked if this series fixes a problem
> > we've really seen to which Ahmad answered:
> > 
> > > 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.
> > 
> > There was no answer to this. I would be glad if you could take this
> > series. I would understand though if you say that this problem is too
> > esoteric to fix it upstream, we would keep the patches locally then.
> > Please just say so, it would help me to get the problem from my desk
> > ;)
> 
> Can someone take this patch series? If not, what can we do to get these
> changes upstream?

I'm going to take it but this may silently break existing users with a
niche use case.
Also, this check will only happen at boot time so I'm not sure there is
a huge benefit, unless your customer reboots the platform often.


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

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

[Marc Kleine-Budde]

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

On 31.01.2023 13:00:12, Alexandre Belloni wrote:
> On 31/01/2023 09:19:55+0100, Marc Kleine-Budde wrote:
> > Hello Alexandre,
> > 
> > On 23.11.2022 10:55:25, 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.
> > > 
> > > Alexandre,
> > > 
> > > Last time this series was send you asked if this series fixes a problem
> > > we've really seen to which Ahmad answered:
> > > 
> > > > 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.
> > > 
> > > There was no answer to this. I would be glad if you could take this
> > > series. I would understand though if you say that this problem is too
> > > esoteric to fix it upstream, we would keep the patches locally then.
> > > Please just say so, it would help me to get the problem from my desk
> > > ;)
> > 
> > Can someone take this patch series? If not, what can we do to get these
> > changes upstream?
> 
> I'm going to take it but this may silently break existing users with a
> niche use case.
> Also, this check will only happen at boot time so I'm not sure there is
> a huge benefit, unless your customer reboots the platform often.

Thanks Alexandre, is this patch already on an immutable branch?

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 RESEND 0/2] rtc: rv8803 patches

[Alexandre Belloni]

On Wed, 23 Nov 2022 10:55:25 +0100, 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.
> 
> Alexandre,
> 
> Last time this series was send you asked if this series fixes a problem
> we've really seen to which Ahmad answered:
> 
> [...]

Applied, thanks!

[1/2] include/linux/bcd.h: provide bcd_is_valid() helper
      commit: 19409796578c879a41e88ddbdbce50c19457658d
[2/2] rtc: rv8803: invalidate date/time if alarm time is invalid
      commit: e5c594233fcf1a55a439dec103aa815cdbf392a7

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

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

[Alexandre Belloni]

On 06/02/2023 16:18:24+0100, Marc Kleine-Budde wrote:
> On 31.01.2023 13:00:12, Alexandre Belloni wrote:
> > On 31/01/2023 09:19:55+0100, Marc Kleine-Budde wrote:
> > > Hello Alexandre,
> > > 
> > > On 23.11.2022 10:55:25, 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.
> > > > 
> > > > Alexandre,
> > > > 
> > > > Last time this series was send you asked if this series fixes a problem
> > > > we've really seen to which Ahmad answered:
> > > > 
> > > > > 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.
> > > > 
> > > > There was no answer to this. I would be glad if you could take this
> > > > series. I would understand though if you say that this problem is too
> > > > esoteric to fix it upstream, we would keep the patches locally then.
> > > > Please just say so, it would help me to get the problem from my desk
> > > > ;)
> > > 
> > > Can someone take this patch series? If not, what can we do to get these
> > > changes upstream?
> > 
> > I'm going to take it but this may silently break existing users with a
> > niche use case.
> > Also, this check will only happen at boot time so I'm not sure there is
> > a huge benefit, unless your customer reboots the platform often.
> 
> Thanks Alexandre, is this patch already on an immutable branch?

It is not, I just pushed it to rtc-next now.

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