Skip to contentSkip to conversation

rtc: rs5c372: Fix read the time from RTC is illegal When reading time from an uninitialized RTC chip, The value may be illegal

PATCH In review [email protected] 3 participants 4 messages May 20, 2021 – May 26, 2021 2 messages not in the archive Real time clock (RTC)

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 rtc: rs5c372: Fix read the time from RTC is illegal When reading time from an uninitialized RTC chip, The value may be illegal

From: qiuxiaojin <[email protected]>

Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/rtc/rtc-rs5c372.c +16 -0
index 3bd6eaa0dcf6..ce61e15d5f3a 100644
@@ -212,6 +212,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	struct rs5c372	*rs5c = i2c_get_clientdata(client);
 	int		status = rs5c_get_regs(rs5c);
 	unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+	int flags_utime = 0;
 
 	if (status < 0)
 		return status;

@@ -239,12 +240,27 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
 	tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+	if (tm->tm_mday < 1) {
+		// The value read from the register may be zero, which is an illegal value
+		flags_utime = flags_utime + 1;
+		tm->tm_mday = 1;
+	}
+
 	/* tm->tm_mon is zero-based */
 	tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;
 
+	if (tm->tm_mon < 0) {
+		flags_utime = flags_utime + 1;
+		tm->tm_mday = 0;
+	}
+
 	/* year is 1900 + tm->tm_year */
 	tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+	if (flags_utime > 0) {
+		rs5c372_rtc_set_time(dev, tm);
+	}
+
 	dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
 		"mday=%d, mon=%d, year=%d, wday=%d\n",
 		__func__,
Signature · 1 line
2.29.0
#1 lore raw

Hi,

2021年5月20日(木) 12:32 <[email protected]>:

Quoted 2 lines

From: qiuxiaojin <[email protected]>

Please briefly describe the patch contained in the email to the subject. And, please write a description of the patch in the text

Quoted 19 lines
Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/rtc/rtc-rs5c372.c +1 -0
index 3bd6eaa0dcf6..ce61e15d5f3a 100644
@@ -212,6 +212,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        struct rs5c372  *rs5c = i2c_get_clientdata(client);
        int             status = rs5c_get_regs(rs5c);
        unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+       int flags_utime = 0;
 
        if (status < 0)
                return status;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 244

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +2 -0
@@ -239,12 +240,27 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
        tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+       if (tm->tm_mday < 1) {
+               // The value read from the register may be zero, which is an illegal value

Please use C89 style commet (/* */).

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 245

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               flags_utime = flags_utime + 1;

I like using ++ (flags_utime++).

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +3 -0
+               tm->tm_mday = 1;
+       }
+
        /* tm->tm_mon is zero-based */
        tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 260

Quoted 10 lines
drivers/rtc/rtc-rs5c372.c +6 -0
 
+       if (tm->tm_mon < 0) {
+               flags_utime = flags_utime + 1;
+               tm->tm_mday = 0;
+       }
+
        /* year is 1900 + tm->tm_year */
        tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+       if (flags_utime > 0) {

{} is unnecessary.

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 261

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               rs5c372_rtc_set_time(dev, tm);

A build error will occur because there is no declaration of rs5c372_rtc_set_time.
Please make sure this can be compiled.

Quoted 8 lines
drivers/rtc/rtc-rs5c372.c +2 -1
+       }
+
        dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
                "mday=%d, mon=%d, year=%d, wday=%d\n",
                __func__,
--

2.29.0

Signature · 3 lines
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#3 lore raw

Hi,

Quoted 6 lines · On 25/05/2021 19:24:14+0800, [email protected] wrote:

Please briefly describe the patch contained in the email to the subject. And, please write a description of the patch in the text
----> The legal days are 1 to 31 and the legal month is less than or equal to 12.
But for an uninitialized RTC chip, the time is random. Days and month may be zero, leading to RTC_valid_TM failed

If the time on the RTC is invalid, then rtc_valid_tm has to fail, there is nothing to fix here.

Quoted 148 lines

add description && fix build error, so I modefied my code :

Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/rtc/rtc-rs5c372.c +20 -0
index 3bd6eaa0dcf6..cb3f90983778 100644
@@ -128,6 +128,9 @@ struct rs5c372 {
  char *regs;
 };
 
+
+static int rs5c372_rtc_set_time(struct device *dev, struct rtc_time *tm);
+
 static int rs5c_get_regs(struct rs5c372 *rs5c)
 {
  struct i2c_client *client = rs5c->client;

@@ -212,6 +215,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
  struct rs5c372 *rs5c = i2c_get_clientdata(client);
  int status = rs5c_get_regs(rs5c);
  unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+ int flags_utime = 0;
 
  if (status < 0)
  return status;

@@ -239,12 +243,28 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
  tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
  tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+ /* The value read from the register may be zero, which is an illegal value */
+ if (tm->tm_mday < 1) {
+ flags_utime++;
+ tm->tm_mday = 1;
+ }
+
  /* tm->tm_mon is zero-based */
  tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;
 
+ if (tm->tm_mon < 0) {
+ /* avoid illegal month */
+ flags_utime++;
+ tm->tm_mon = 0;
+ }
+
  /* year is 1900 + tm->tm_year */
  tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+ /* update legal time */
+ if (flags_utime > 0)
+ rs5c372_rtc_set_time(dev, tm);
+
  dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
  "mday=%d, mon=%d, year=%d, wday=%d\n",
  __func__,
Signature · 14 lines
2.29.0



[email protected]
 
From: Nobuhiro Iwamatsu
Date: 2021-05-24 12:40
To: qxj511mail
CC: Alessandro Zummo; Alexandre Belloni; linux-rtc; Linux Kernel Mailing List; qiuxiaojin
Subject: Re: [PATCH] rtc: rs5c372: Fix read the time from RTC is illegal When reading time from an uninitialized RTC chip, The value may be illegal
Hi,
 
2021年5月20日(木) 12:32 <[email protected]>:
Quoted 2 lines

From: qiuxiaojin <[email protected]>

Please briefly describe the patch contained in the email to the subject. And, please write a description of the patch in the text

Quoted 19 lines
Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/rtc/rtc-rs5c372.c +1 -0
index 3bd6eaa0dcf6..ce61e15d5f3a 100644
@@ -212,6 +212,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        struct rs5c372  *rs5c = i2c_get_clientdata(client);
        int             status = rs5c_get_regs(rs5c);
        unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+       int flags_utime = 0;
 
        if (status < 0)
                return status;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 244

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +2 -0
@@ -239,12 +240,27 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
        tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+       if (tm->tm_mday < 1) {
+               // The value read from the register may be zero, which is an illegal value

Please use C89 style commet (/* */).

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 245

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               flags_utime = flags_utime + 1;

I like using ++ (flags_utime++).

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +3 -0
+               tm->tm_mday = 1;
+       }
+
        /* tm->tm_mon is zero-based */
        tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 260

Quoted 10 lines
drivers/rtc/rtc-rs5c372.c +6 -0
 
+       if (tm->tm_mon < 0) {
+               flags_utime = flags_utime + 1;
+               tm->tm_mday = 0;
+       }
+
        /* year is 1900 + tm->tm_year */
        tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+       if (flags_utime > 0) {

{} is unnecessary.

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 261

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               rs5c372_rtc_set_time(dev, tm);

A build error will occur because there is no declaration of rs5c372_rtc_set_time.
Please make sure this can be compiled.

Quoted 8 lines
drivers/rtc/rtc-rs5c372.c +2 -1
+       }
+
        dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
                "mday=%d, mon=%d, year=%d, wday=%d\n",
                __func__,
--

2.29.0

Signature · 3 lines
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

maintainer
#5 lore raw
Quoted 6 lines · On 26/05/2021 19:35:31+0800, [email protected] wrote:
Quoted 2 lines

If the time on the RTC is invalid, then rtc_valid_tm has to fail, there is nothing to fix here.

I mean, if you try to get the time or execute hwclock command, Invalid arguments are returned when the RTC chip is not initialized therefore，we need to get a valid time

Well, the time is invalid so hwclock needs to know it is invalid. Returning a bogus time and date doesn't make it right. hwclock needs to fail.

Quoted 177 lines

[email protected]

From: Alexandre Belloni
Date: 2021-05-26 05:18
To: [email protected]
CC: Nobuhiro Iwamatsu; a.zummo; linux-rtc; linux-kernel; 邱晓金
Subject: Re: Re: [PATCH] rtc: rs5c372: Fix read the time from RTC is illegal When reading time from an uninitialized RTC chip, The value may be illegal Hi,

Quoted 6 lines · On 25/05/2021 19:24:14+0800, [email protected] wrote:

Please briefly describe the patch contained in the email to the subject. And, please write a description of the patch in the text
----> The legal days are 1 to 31 and the legal month is less than or equal to 12.
But for an uninitialized RTC chip, the time is random. Days and month may be zero, leading to RTC_valid_TM failed

If the time on the RTC is invalid, then rtc_valid_tm has to fail, there is nothing to fix here.

Quoted 148 lines

add description && fix build error, so I modefied my code :

Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
drivers/rtc/rtc-rs5c372.c +20 -0
index 3bd6eaa0dcf6..cb3f90983778 100644
@@ -128,6 +128,9 @@ struct rs5c372 {
  char *regs;
 };
 
+
+static int rs5c372_rtc_set_time(struct device *dev, struct rtc_time *tm);
+
 static int rs5c_get_regs(struct rs5c372 *rs5c)
 {
  struct i2c_client *client = rs5c->client;

@@ -212,6 +215,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
  struct rs5c372 *rs5c = i2c_get_clientdata(client);
  int status = rs5c_get_regs(rs5c);
  unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+ int flags_utime = 0;
 
  if (status < 0)
  return status;

@@ -239,12 +243,28 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
  tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
  tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+ /* The value read from the register may be zero, which is an illegal value */
+ if (tm->tm_mday < 1) {
+ flags_utime++;
+ tm->tm_mday = 1;
+ }
+
  /* tm->tm_mon is zero-based */
  tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;
 
+ if (tm->tm_mon < 0) {
+ /* avoid illegal month */
+ flags_utime++;
+ tm->tm_mon = 0;
+ }
+
  /* year is 1900 + tm->tm_year */
  tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+ /* update legal time */
+ if (flags_utime > 0)
+ rs5c372_rtc_set_time(dev, tm);
+
  dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
  "mday=%d, mon=%d, year=%d, wday=%d\n",
  __func__,
Signature · 14 lines
2.29.0



[email protected]
 
From: Nobuhiro Iwamatsu
Date: 2021-05-24 12:40
To: qxj511mail
CC: Alessandro Zummo; Alexandre Belloni; linux-rtc; Linux Kernel Mailing List; qiuxiaojin
Subject: Re: [PATCH] rtc: rs5c372: Fix read the time from RTC is illegal When reading time from an uninitialized RTC chip, The value may be illegal
Hi,
 
2021年5月20日(木) 12:32 <[email protected]>:
Quoted 2 lines

From: qiuxiaojin <[email protected]>

Please briefly describe the patch contained in the email to the subject. And, please write a description of the patch in the text

Quoted 19 lines
Signed-off-by qiuxiaojin <[email protected]>
---
 drivers/rtc/rtc-rs5c372.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/rtc/rtc-rs5c372.c +1 -0
index 3bd6eaa0dcf6..ce61e15d5f3a 100644
@@ -212,6 +212,7 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        struct rs5c372  *rs5c = i2c_get_clientdata(client);
        int             status = rs5c_get_regs(rs5c);
        unsigned char ctrl2 = rs5c->regs[RS5C_REG_CTRL2];
+       int flags_utime = 0;
 
        if (status < 0)
                return status;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 244

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +2 -0
@@ -239,12 +240,27 @@ static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm)
        tm->tm_wday = bcd2bin(rs5c->regs[RS5C372_REG_WDAY] & 0x07);
        tm->tm_mday = bcd2bin(rs5c->regs[RS5C372_REG_DAY] & 0x3f);
 
+       if (tm->tm_mday < 1) {
+               // The value read from the register may be zero, which is an illegal value

Please use C89 style commet (/* */).

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 245

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               flags_utime = flags_utime + 1;

I like using ++ (flags_utime++).

Quoted 5 lines
drivers/rtc/rtc-rs5c372.c +3 -0
+               tm->tm_mday = 1;
+       }
+
        /* tm->tm_mon is zero-based */
        tm->tm_mon = bcd2bin(rs5c->regs[RS5C372_REG_MONTH] & 0x1f) - 1;

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 260

Quoted 10 lines
drivers/rtc/rtc-rs5c372.c +6 -0
 
+       if (tm->tm_mon < 0) {
+               flags_utime = flags_utime + 1;
+               tm->tm_mday = 0;
+       }
+
        /* year is 1900 + tm->tm_year */
        tm->tm_year = bcd2bin(rs5c->regs[RS5C372_REG_YEAR]) + 100;
 
+       if (flags_utime > 0) {

{} is unnecessary.

drivers/rtc/rtc-rs5c372.c static int rs5c372_rtc_read_time(struct device *dev, struct rtc_time *tm) · line 261

Quoted 1 line
drivers/rtc/rtc-rs5c372.c +1 -0
+               rs5c372_rtc_set_time(dev, tm);

A build error will occur because there is no declaration of rs5c372_rtc_set_time.
Please make sure this can be compiled.

Quoted 8 lines
drivers/rtc/rtc-rs5c372.c +2 -1
+       }
+
        dev_dbg(&client->dev, "%s: tm is secs=%d, mins=%d, hours=%d, "
                "mday=%d, mon=%d, year=%d, wday=%d\n",
                __func__,
--

2.29.0

Signature · 3 lines
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Signature · 3 lines
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com