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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250401090454fb0ccf16@mail.local>
Date: Tue, 1 Apr 2025 11:04:54 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: nmydeen@...sta.com
Cc: linux-rtc@...r.kernel.org, linux-kernel@...r.kernel.org,
	cminyard@...sta.com
Subject: Re: [PATCH] rtc-m41t62: kickstart ocillator upon failure

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ