Re: [PATCH 5/5] rtc: rv8803: invalidate date/time if alarm time is invalid

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hello Sascha,

Sorry for the very late review. I'm mostly fine with the whole series.

On 26/04/2022 09:10:56+0200, Sascha Hauer wrote:
> From: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> 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 | 46 +++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rv8803.c b/drivers/rtc/rtc-rv8803.c
> index fe1247e771b98..036c449cf1c20 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,10 +319,16 @@ 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)
>  			return ret;
> +		rv8803->alarm_invalid = false;
>  	}
>  
>  	ret = rv8803_write_reg(rv8803->client, RV8803_FLAG,
> @@ -342,13 +354,27 @@ static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	if (flags < 0)
>  		return flags;
>  
> -	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);
> +	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])) {
> +		alrm->time.tm_sec  = 0;
> +		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;
> +		alrm->enabled = !!(rv8803->ctrl & RV8803_CTRL_AIE);
> +		alrm->pending = (flags & RV8803_FLAG_AF) && alrm->enabled;
> +	}
> +
> +	if ((unsigned)alrm->time.tm_mday > 31 ||

You'd need to use (unsigned int) here, else I'll get follow up patches
doing the conversion. My only issue is actually this check as it would
be better to use rtc_valid_tm that checks tm_month but obviously, your
rtc_tm is missing information at this time and this would require to
replicate what is done in __rtc_read_alarm. I don't have a great
solution to that though

> +	    (unsigned)alrm->time.tm_hour >= 24 ||
> +	    (unsigned)alrm->time.tm_min >= 60) {
> +		rv8803->alarm_invalid = true;
> +		return -EINVAL;
> +	}


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