[rtc-linux] [PATCH] rtc-da9063: avoid writing undefined data to rtc

[rtc-linux] [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Enrico Scholz]

driver did

| static void da9063_tm_to_data(struct rtc_time *tm, u8 *data,
| {
|  	const struct da9063_compatible_rtc_regmap *config = rtc->config;
|
| 	data[RTC_SEC] &= ~config->rtc_count_sec_mask;
| 	data[RTC_SEC] |= tm->tm_sec & config->rtc_count_sec_mask;
| ...
| }
| ...
| static int da9063_rtc_set_time(struct device *dev, struct rtc_time *tm)
| {
|       ...
|	u8 data[RTC_DATA_LEN];
|	int ret;
|
|	da9063_tm_to_data(tm, data, rtc);

which means that some bits of stack content (in 'data[]') was masked out
and written to the RTC.

Because da9063_tm_to_data() is used only by da9063_rtc_set_time() and
da9063_rtc_set_alarm(), we can write fields directly.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 drivers/rtc/rtc-da9063.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index 00a8f7f..8d5b9be 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c
@@ -191,24 +191,13 @@ static void da9063_tm_to_data(struct rtc_time *tm, u8 *data,
 {
 	const struct da9063_compatible_rtc_regmap *config = rtc->config;
 
-	data[RTC_SEC] &= ~config->rtc_count_sec_mask;
-	data[RTC_SEC] |= tm->tm_sec & config->rtc_count_sec_mask;
-
-	data[RTC_MIN] &= ~config->rtc_count_min_mask;
-	data[RTC_MIN] |= tm->tm_min & config->rtc_count_min_mask;
-
-	data[RTC_HOUR] &= ~config->rtc_count_hour_mask;
-	data[RTC_HOUR] |= tm->tm_hour & config->rtc_count_hour_mask;
-
-	data[RTC_DAY] &= ~config->rtc_count_day_mask;
-	data[RTC_DAY] |= tm->tm_mday & config->rtc_count_day_mask;
-
-	data[RTC_MONTH] &= ~config->rtc_count_month_mask;
-	data[RTC_MONTH] |= MONTHS_TO_DA9063(tm->tm_mon) &
+	data[RTC_SEC]   = tm->tm_sec & config->rtc_count_sec_mask;
+	data[RTC_MIN]   = tm->tm_min & config->rtc_count_min_mask;
+	data[RTC_HOUR]  = tm->tm_hour & config->rtc_count_hour_mask;
+	data[RTC_DAY]   = tm->tm_mday & config->rtc_count_day_mask;
+	data[RTC_MONTH] = MONTHS_TO_DA9063(tm->tm_mon) &
 				config->rtc_count_month_mask;
-
-	data[RTC_YEAR] &= ~config->rtc_count_year_mask;
-	data[RTC_YEAR] |= YEARS_TO_DA9063(tm->tm_year) &
+	data[RTC_YEAR]  = YEARS_TO_DA9063(tm->tm_year) &
 				config->rtc_count_year_mask;
 }
 
-- 
2.4.3

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Alexandre Belloni]

Hi,

On 27/11/2015 at 13:02:55 +0100, Enrico Scholz wrote :
> driver did
> 
> | static void da9063_tm_to_data(struct rtc_time *tm, u8 *data,
> | {
> |  	const struct da9063_compatible_rtc_regmap *config = rtc->config;
> |
> | 	data[RTC_SEC] &= ~config->rtc_count_sec_mask;
> | 	data[RTC_SEC] |= tm->tm_sec & config->rtc_count_sec_mask;
> | ...
> | }
> | ...
> | static int da9063_rtc_set_time(struct device *dev, struct rtc_time *tm)
> | {
> |       ...
> |	u8 data[RTC_DATA_LEN];
> |	int ret;
> |
> |	da9063_tm_to_data(tm, data, rtc);
> 
> which means that some bits of stack content (in 'data[]') was masked out
> and written to the RTC.
> 
> Because da9063_tm_to_data() is used only by da9063_rtc_set_time() and
> da9063_rtc_set_alarm(), we can write fields directly.
> 

Unfortunately, the datasheets are behind a register wall and I can't
check. Is there any meaningful bits mixed in the date/time registers?

I think this was intended to keep those bits to their previous value but
this is not yet implemented.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Enrico Scholz]

Alexandre Belloni <alexandre.belloni@free-electrons.com> writes:

>> | static void da9063_tm_to_data(struct rtc_time *tm, u8 *data,
>> | {
>> |  	const struct da9063_compatible_rtc_regmap *config = rtc->config;
>> |
>> | 	data[RTC_SEC] &= ~config->rtc_count_sec_mask;
>> | 	data[RTC_SEC] |= tm->tm_sec & config->rtc_count_sec_mask;
>> | ...
>> | }
>> ...
>> which means that some bits of stack content (in 'data[]') was masked out
>> and written to the RTC.
>> 
>> Because da9063_tm_to_data() is used only by da9063_rtc_set_time() and
>> da9063_rtc_set_alarm(), we can write fields directly.
>
> Unfortunately, the datasheets are behind a register wall and I can't
> check. Is there any meaningful bits mixed in the date/time registers?

We can set "RTC_EN and CRYSTAL writing disabled" (COUNT_Y[6]) randomly;
this bit has a different meaning for read ("RTC clock okay") and write
("disable writing"; not clearable).

ALARM_MO[4] "Period of the Tick (0: every second, 1: every minute)" or
ALARM_Y[7+6] (enable tick/alarm) should not be set randomly either.


I think, it is always a bad idea to write random data from stack to a
device.


> I think this was intended to keep those bits to their previous value but
> this is not yet implemented.

Datasheet does not say how to deal with "reserved" bits; but it would be
imo overkill to apply a regmap_update_bits() like access to the RTC
registers.


Enrico

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Alexandre Belloni]

On 27/11/2015 at 16:27:25 +0100, Enrico Scholz wrote :
> > Unfortunately, the datasheets are behind a register wall and I can't
> > check. Is there any meaningful bits mixed in the date/time registers?
> 
> We can set "RTC_EN and CRYSTAL writing disabled" (COUNT_Y[6]) randomly;
> this bit has a different meaning for read ("RTC clock okay") and write
> ("disable writing"; not clearable).
> 
> ALARM_MO[4] "Period of the Tick (0: every second, 1: every minute)" or
> ALARM_Y[7+6] (enable tick/alarm) should not be set randomly either.
> 
> 
> I think, it is always a bad idea to write random data from stack to a
> device.
> 

I agree.

> 
> > I think this was intended to keep those bits to their previous value but
> > this is not yet implemented.
> 
> Datasheet does not say how to deal with "reserved" bits; but it would be
> imo overkill to apply a regmap_update_bits() like access to the RTC
> registers.
> 

It depends on how much you care about those bits across reboots.

I'll take your patch anyway, unless Steve has another opinion on that.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

RE: [rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Opensource [Steve Twiss]]

On 27 November 2015 15:57 Alexandre Belloni wrote:

> To: Enrico Scholz; Opensource [Steve Twiss]
> Cc: rtc-linux@googlegroups.com; Alessandro Zummo; Support Opensource
> Subject: Re: [rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data
> to rtc
> 
> On 27/11/2015 at 16:27:25 +0100, Enrico Scholz wrote :
> > > Unfortunately, the datasheets are behind a register wall and I can't
> > > check. Is there any meaningful bits mixed in the date/time registers?
> >
> > We can set "RTC_EN and CRYSTAL writing disabled" (COUNT_Y[6]) randomly;
> > this bit has a different meaning for read ("RTC clock okay") and write
> > ("disable writing"; not clearable).
> >
> > ALARM_MO[4] "Period of the Tick (0: every second, 1: every minute)" or
> > ALARM_Y[7+6] (enable tick/alarm) should not be set randomly either.
> >
> > I think, it is always a bad idea to write random data from stack to a
> > device.
> 
> I agree.
> 
> > > I think this was intended to keep those bits to their previous value but
> > > this is not yet implemented.
> >
> > Datasheet does not say how to deal with "reserved" bits; but it would be
> > imo overkill to apply a regmap_update_bits() like access to the RTC
> > registers.
> >
> 
> It depends on how much you care about those bits across reboots.
> 
> I'll take your patch anyway, unless Steve has another opinion on that.
> 

 I'll take a look at this later today.

Regards,
Steve

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

RE: [rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Opensource [Steve Twiss]]

On Sent: 27 November 2015 15:57 Alexandre Belloni wrote:

> Subject: Re: [rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc
> On 27/11/2015 at 16:27:25 +0100, Enrico Scholz wrote :
> >
> > I think, it is always a bad idea to write random data from stack to a
> > device.
> 
> I agree.
> 

Hi Alexandre and Enrico,

I have double checked this morning and in these cases it is not important whether it is a 1 or a 0
written to these specific reserved bits.

But, I did intended to write a zero to the reserved bits.
Enrico's patch will make the recommendation clear.

> 
> I'll take your patch anyway, unless Steve has another opinion on that.
> 

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>
Tested-by: Steve Twiss <stwiss.opensource@diasemi.com>

Regards,
Steve

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[rtc-linux] Re: [PATCH] rtc-da9063: avoid writing undefined data to rtc

[Alexandre Belloni]

On 27/11/2015 at 13:02:55 +0100, Enrico Scholz wrote :
> driver did
> 
> | static void da9063_tm_to_data(struct rtc_time *tm, u8 *data,
> | {
> |  	const struct da9063_compatible_rtc_regmap *config = rtc->config;
> |
> | 	data[RTC_SEC] &= ~config->rtc_count_sec_mask;
> | 	data[RTC_SEC] |= tm->tm_sec & config->rtc_count_sec_mask;
> | ...
> | }
> | ...
> | static int da9063_rtc_set_time(struct device *dev, struct rtc_time *tm)
> | {
> |       ...
> |	u8 data[RTC_DATA_LEN];
> |	int ret;
> |
> |	da9063_tm_to_data(tm, data, rtc);
> 
> which means that some bits of stack content (in 'data[]') was masked out
> and written to the RTC.
> 
> Because da9063_tm_to_data() is used only by da9063_rtc_set_time() and
> da9063_rtc_set_alarm(), we can write fields directly.
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  drivers/rtc/rtc-da9063.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.