| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAG3LavJku7+0pj0pLEktenLBGA-pXhRHOs_fE4W_Z2EzjdF0+Q@mail.gmail.com>
Date: Wed, 2 Apr 2025 01:10:11 +0530
From: Niyas Mydeen <nmydeen@...sta.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org,
cminyard@...sta.com
Subject: Re: [PATCH] rtc-m41t62: kickstart ocillator upon failure
Hi Alexandre,
Thanks for your comments, I have incorporated all your suggestions and
attached here the updated version.
Hope it meets your expectations.
On Tue, Apr 1, 2025 at 2:34 PM Alexandre Belloni <
alexandre.belloni@...tlin.com> wrote:
> Hello,
>
> On 16/01/2025 11:56:41+0530, nmydeen@...sta.com wrote:
> > From: "A. Niyas Ahamed Mydeen" <nmydeen@...sta.com>
> >
> > The ocillator on the m41t62 (and other chips of this type) needs
> > a kickstart upon a failure; the RTC read routine will notice the
> > oscillator failure and fail reads. This is added in the RTC write
> > routine; this allows the system to know that the time in the RTC
> > is accurate. This is following the procedure described in section
> > 3.11 of "https://www.st.com/resource/en/datasheet/m41t62.pdf"
> >
> > Signed-off-by: A. Niyas Ahamed Mydeen <nmydeen@...sta.com>
> > Reviewed-by: Corey Minyard <cminyard@...sta.com>
> > ---
> > drivers/rtc/rtc-m41t80.c | 70 ++++++++++++++++++++++++++++------------
> > 1 file changed, 49 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> > index 1f58ae8b151e..77c21c91bae3 100644
> > --- a/drivers/rtc/rtc-m41t80.c
> > +++ b/drivers/rtc/rtc-m41t80.c
> > @@ -22,6 +22,7 @@
> > #include <linux/slab.h>
> > #include <linux/mutex.h>
> > #include <linux/string.h>
> > +#include <linux/delay.h>
> > #ifdef CONFIG_RTC_DRV_M41T80_WDT
> > #include <linux/fs.h>
> > #include <linux/ioctl.h>
> > @@ -204,7 +205,7 @@ static int m41t80_rtc_read_time(struct device *dev,
> struct rtc_time *tm)
> > return flags;
> >
> > if (flags & M41T80_FLAGS_OF) {
> > - dev_err(&client->dev, "Oscillator failure, data is
> invalid.\n");
> > + dev_err(&client->dev, "Oscillator failure, time may not be
> accurate, write time to RTC to fix it.\n");
> > return -EINVAL;
> > }
> >
> > @@ -227,21 +228,60 @@ static int m41t80_rtc_read_time(struct device
> *dev, struct rtc_time *tm)
> > return 0;
> > }
> >
> > -static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +static int m41t80_rtc_set_time(struct device *dev, struct rtc_time
> *in_tm)
> > {
> > struct i2c_client *client = to_i2c_client(dev);
> > struct m41t80_data *clientdata = i2c_get_clientdata(client);
> > + struct rtc_time tm = *in_tm;
> > unsigned char buf[8];
> > int err, flags;
> > + time64_t time = 0;
> >
> > + flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> > + if (flags < 0)
> > + return flags;
> > + if (flags & M41T80_FLAGS_OF) {
> > + /* OF cannot be immediately reset: oscillator has to be
> restarted. */
> > + dev_warn(&client->dev, "OF bit is still set, kickstarting
> clock.\n");
> > + err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> M41T80_SEC_ST);
> > + if (err < 0) {
> > + dev_err(&client->dev, "Can't set ST bit\n");
>
> This is super verbose, please use dev_dbg or drop the dev_errs. The only
> user action after a failure would be to restart the operation anyway.
>
> > + return err;
> > + }
> > + err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> > + flags &
> ~M41T80_SEC_ST);
> > + if (err < 0) {
> > + dev_err(&client->dev, "Can't clear ST bit\n");
> > + return err;
> > + }
> > + /* oscillator must run for 4sec before we attempt to reset
> OF bit */
> > + msleep(4000);
> > + /* Clear the OF bit of Flags Register */
> > + err = i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
> > + flags & ~M41T80_FLAGS_OF);
>
> checkpatch --strict complains about some style issues, please fix those.
>
> > + if (err < 0) {
> > + dev_err(&client->dev, "Unable to write flags
> register\n");
> > + return err;
> > + }
> > + flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> > + if (flags < 0)
> > + return flags;
> > + else if (flags & M41T80_FLAGS_OF) {
> > + dev_err(&client->dev, "Can't clear the OF bit
> check battery\n");
> > + return err;
> > + }
> > + /* add 4sec of oscillator stablize time otherwise we are
> behind 4sec */
> > + time = rtc_tm_to_time64(&tm);
> > + rtc_time64_to_tm(time+4, &tm);
> > + }
>
> The main issue is that now, you have cleared OF so if any read/write to
> the RTC fails, you would return from the function without having set the
> time. So when OF is set, you should first add the 4s, then set the time,
> then kickstart the RTC.
>
> > buf[M41T80_REG_SSEC] = 0;
> > - buf[M41T80_REG_SEC] = bin2bcd(tm->tm_sec);
> > - buf[M41T80_REG_MIN] = bin2bcd(tm->tm_min);
> > - buf[M41T80_REG_HOUR] = bin2bcd(tm->tm_hour);
> > - buf[M41T80_REG_DAY] = bin2bcd(tm->tm_mday);
> > - buf[M41T80_REG_MON] = bin2bcd(tm->tm_mon + 1);
> > - buf[M41T80_REG_YEAR] = bin2bcd(tm->tm_year - 100);
> > - buf[M41T80_REG_WDAY] = tm->tm_wday;
> > + buf[M41T80_REG_SEC] = bin2bcd(tm.tm_sec);
> > + buf[M41T80_REG_MIN] = bin2bcd(tm.tm_min);
> > + buf[M41T80_REG_HOUR] = bin2bcd(tm.tm_hour);
> > + buf[M41T80_REG_DAY] = bin2bcd(tm.tm_mday);
> > + buf[M41T80_REG_MON] = bin2bcd(tm.tm_mon + 1);
> > + buf[M41T80_REG_YEAR] = bin2bcd(tm.tm_year - 100);
> > + buf[M41T80_REG_WDAY] = tm.tm_wday;
> >
> > /* If the square wave output is controlled in the weekday register
> */
> > if (clientdata->features & M41T80_FEATURE_SQ_ALT) {
> > @@ -261,18 +301,6 @@ static int m41t80_rtc_set_time(struct device *dev,
> struct rtc_time *tm)
> > return err;
> > }
> >
> > - /* Clear the OF bit of Flags Register */
> > - flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> > - if (flags < 0)
> > - return flags;
> > -
> > - err = i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
> > - flags & ~M41T80_FLAGS_OF);
> > - if (err < 0) {
> > - dev_err(&client->dev, "Unable to write flags register\n");
> > - return err;
> > - }
> > -
> > return err;
> > }
> >
> > --
> > 2.34.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
--
Regards,
A. Mydeen.
Content of type "text/html" skipped
View attachment "0001-rtc-m41t62-kickstart-ocillator-upon-failure.patch" of type "text/x-patch" (4787 bytes)
Powered by blists - more mailing lists