[PATCH] rtc: core: let rtc_set_time properly populate element wday

[PATCH] rtc: core: let rtc_set_time properly populate element wday

[Heiner Kallweit]

Some drivers use element wday of the struct rtc_time which is passed to
callback set_time. However element wday may incorrectly or not be
populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
rtc_valid_tm() doesn't check element wday.

Therefore convert struct rtc_time to time64_t and then use
rtc_time64_to_tm to generate a struct rtc_time with all elements properly
populated.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/rtc/interface.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 8cec9a02..5a53c590 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
 
 int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
 {
+	time64_t secs64;
 	int err;
 
 	err = rtc_valid_tm(tm);
 	if (err != 0)
 		return err;
 
+	secs64 = rtc_tm_to_time64(tm);
+
 	err = mutex_lock_interruptible(&rtc->ops_lock);
 	if (err)
 		return err;
 
 	if (!rtc->ops)
 		err = -ENODEV;
-	else if (rtc->ops->set_time)
-		err = rtc->ops->set_time(rtc->dev.parent, tm);
-	else if (rtc->ops->set_mmss64) {
-		time64_t secs64 = rtc_tm_to_time64(tm);
-
+	else if (rtc->ops->set_time) {
+		struct rtc_time tmp;
+
+		/*
+		 * element wday of tm may not be set, therefore use
+		 * rtc_time64_to_tm to generate a struct rtc_time
+		 * with all elements being properly populated
+		 */
+		rtc_time64_to_tm(secs64, &tmp);
+		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
+	} else if (rtc->ops->set_mmss64)
 		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
-	} else if (rtc->ops->set_mmss) {
-		time64_t secs64 = rtc_tm_to_time64(tm);
+	else if (rtc->ops->set_mmss)
 		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
-	} else
+	else
 		err = -EINVAL;
 
 	pm_stay_awake(rtc->dev.parent);
-- 
2.14.1

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Alexandre Belloni]

There is no point in fixing that. Nobody uses wday.

On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
> Some drivers use element wday of the struct rtc_time which is passed to
> callback set_time. However element wday may incorrectly or not be
> populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
> rtc_valid_tm() doesn't check element wday.
> 
> Therefore convert struct rtc_time to time64_t and then use
> rtc_time64_to_tm to generate a struct rtc_time with all elements properly
> populated.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/rtc/interface.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index 8cec9a02..5a53c590 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
>  
>  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>  {
> +	time64_t secs64;
>  	int err;
>  
>  	err = rtc_valid_tm(tm);
>  	if (err != 0)
>  		return err;
>  
> +	secs64 = rtc_tm_to_time64(tm);
> +
>  	err = mutex_lock_interruptible(&rtc->ops_lock);
>  	if (err)
>  		return err;
>  
>  	if (!rtc->ops)
>  		err = -ENODEV;
> -	else if (rtc->ops->set_time)
> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
> -	else if (rtc->ops->set_mmss64) {
> -		time64_t secs64 = rtc_tm_to_time64(tm);
> -
> +	else if (rtc->ops->set_time) {
> +		struct rtc_time tmp;
> +
> +		/*
> +		 * element wday of tm may not be set, therefore use
> +		 * rtc_time64_to_tm to generate a struct rtc_time
> +		 * with all elements being properly populated
> +		 */
> +		rtc_time64_to_tm(secs64, &tmp);
> +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
> +	} else if (rtc->ops->set_mmss64)
>  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
> -	} else if (rtc->ops->set_mmss) {
> -		time64_t secs64 = rtc_tm_to_time64(tm);
> +	else if (rtc->ops->set_mmss)
>  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
> -	} else
> +	else
>  		err = -EINVAL;
>  
>  	pm_stay_awake(rtc->dev.parent);
> -- 
> 2.14.1
> 

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

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Alexandre Belloni]

On 26/08/2017 at 21:56:07 +0200, Alexandre Belloni wrote:
> There is no point in fixing that. Nobody uses wday.
> 

And that was going to be my comment on your other patch. You may as well
just remove the whole wday correction from ds1307.

> On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
> > Some drivers use element wday of the struct rtc_time which is passed to
> > callback set_time. However element wday may incorrectly or not be
> > populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
> > rtc_valid_tm() doesn't check element wday.
> > 
> > Therefore convert struct rtc_time to time64_t and then use
> > rtc_time64_to_tm to generate a struct rtc_time with all elements properly
> > populated.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/rtc/interface.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> > index 8cec9a02..5a53c590 100644
> > --- a/drivers/rtc/interface.c
> > +++ b/drivers/rtc/interface.c
> > @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
> >  
> >  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
> >  {
> > +	time64_t secs64;
> >  	int err;
> >  
> >  	err = rtc_valid_tm(tm);
> >  	if (err != 0)
> >  		return err;
> >  
> > +	secs64 = rtc_tm_to_time64(tm);
> > +
> >  	err = mutex_lock_interruptible(&rtc->ops_lock);
> >  	if (err)
> >  		return err;
> >  
> >  	if (!rtc->ops)
> >  		err = -ENODEV;
> > -	else if (rtc->ops->set_time)
> > -		err = rtc->ops->set_time(rtc->dev.parent, tm);
> > -	else if (rtc->ops->set_mmss64) {
> > -		time64_t secs64 = rtc_tm_to_time64(tm);
> > -
> > +	else if (rtc->ops->set_time) {
> > +		struct rtc_time tmp;
> > +
> > +		/*
> > +		 * element wday of tm may not be set, therefore use
> > +		 * rtc_time64_to_tm to generate a struct rtc_time
> > +		 * with all elements being properly populated
> > +		 */
> > +		rtc_time64_to_tm(secs64, &tmp);
> > +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
> > +	} else if (rtc->ops->set_mmss64)
> >  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
> > -	} else if (rtc->ops->set_mmss) {
> > -		time64_t secs64 = rtc_tm_to_time64(tm);
> > +	else if (rtc->ops->set_mmss)
> >  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
> > -	} else
> > +	else
> >  		err = -EINVAL;
> >  
> >  	pm_stay_awake(rtc->dev.parent);
> > -- 
> > 2.14.1
> > 
> 
> -- 
> Alexandre Belloni, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

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

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Heiner Kallweit]

Am 26.08.2017 um 21:59 schrieb Alexandre Belloni:
> On 26/08/2017 at 21:56:07 +0200, Alexandre Belloni wrote:
>> There is no point in fixing that. Nobody uses wday.
>>
> 
> And that was going to be my comment on your other patch. You may as well
> just remove the whole wday correction from ds1307.
> 
Actually I was wondering too when wday should ever be used, except
theoretically for an alarm, however commit e29385fab0bf
"rtc: ds1307: Fix relying on reset value for weekday" was added just
a year ago.

I think then we can remove the usage of wday in general, including
writing / reading wday in ds1307_get/set_time.

>> On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
>>> Some drivers use element wday of the struct rtc_time which is passed to
>>> callback set_time. However element wday may incorrectly or not be
>>> populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
>>> rtc_valid_tm() doesn't check element wday.
>>>
>>> Therefore convert struct rtc_time to time64_t and then use
>>> rtc_time64_to_tm to generate a struct rtc_time with all elements properly
>>> populated.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/rtc/interface.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
>>> index 8cec9a02..5a53c590 100644
>>> --- a/drivers/rtc/interface.c
>>> +++ b/drivers/rtc/interface.c
>>> @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
>>>  
>>>  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>>>  {
>>> +	time64_t secs64;
>>>  	int err;
>>>  
>>>  	err = rtc_valid_tm(tm);
>>>  	if (err != 0)
>>>  		return err;
>>>  
>>> +	secs64 = rtc_tm_to_time64(tm);
>>> +
>>>  	err = mutex_lock_interruptible(&rtc->ops_lock);
>>>  	if (err)
>>>  		return err;
>>>  
>>>  	if (!rtc->ops)
>>>  		err = -ENODEV;
>>> -	else if (rtc->ops->set_time)
>>> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
>>> -	else if (rtc->ops->set_mmss64) {
>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>> -
>>> +	else if (rtc->ops->set_time) {
>>> +		struct rtc_time tmp;
>>> +
>>> +		/*
>>> +		 * element wday of tm may not be set, therefore use
>>> +		 * rtc_time64_to_tm to generate a struct rtc_time
>>> +		 * with all elements being properly populated
>>> +		 */
>>> +		rtc_time64_to_tm(secs64, &tmp);
>>> +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
>>> +	} else if (rtc->ops->set_mmss64)
>>>  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
>>> -	} else if (rtc->ops->set_mmss) {
>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>> +	else if (rtc->ops->set_mmss)
>>>  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
>>> -	} else
>>> +	else
>>>  		err = -EINVAL;
>>>  
>>>  	pm_stay_awake(rtc->dev.parent);
>>> -- 
>>> 2.14.1
>>>
>>
>> -- 
>> Alexandre Belloni, Free Electrons
>> Embedded Linux and Kernel engineering
>> http://free-electrons.com
> 

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Heiner Kallweit]

Am 27.08.2017 um 14:39 schrieb Heiner Kallweit:
> Am 26.08.2017 um 21:59 schrieb Alexandre Belloni:
>> On 26/08/2017 at 21:56:07 +0200, Alexandre Belloni wrote:
>>> There is no point in fixing that. Nobody uses wday.
>>>
>>
>> And that was going to be my comment on your other patch. You may as well
>> just remove the whole wday correction from ds1307.
>>
> Actually I was wondering too when wday should ever be used, except
> theoretically for an alarm, however commit e29385fab0bf
> "rtc: ds1307: Fix relying on reset value for weekday" was added just
> a year ago.
> 
When having a look at the MCP794XX datasheet it became clear why the
mentioned weekday-related patch was submitted. This chip is quite
strange regarding the possible alarm match conditions:

ALMxMSK<2:0>: Alarm Mask bits
000 = Seconds match
001 = Minutes match
010 = Hours match (logic takes into account 12-/24-hour operation)
011 = Day of week match
100 = Date match
101 = Reserved; do not use
110 = Reserved; do not use
111 = Seconds, Minutes, Hour, Day of Week, Date and Month

When not having a proper weekday you get only either seconds or
minutes or hours or date match.

So it's best to ensure that the weekday is properly populated.
However we can do this in the driver and don't have to touch the core.

> I think then we can remove the usage of wday in general, including
> writing / reading wday in ds1307_get/set_time.
> 
>>> On 26/08/2017 at 21:16:34 +0200, Heiner Kallweit wrote:
>>>> Some drivers use element wday of the struct rtc_time which is passed to
>>>> callback set_time. However element wday may incorrectly or not be
>>>> populated if struct rtc_time is coming from userspace via rtc_dev_ioctl.
>>>> rtc_valid_tm() doesn't check element wday.
>>>>
>>>> Therefore convert struct rtc_time to time64_t and then use
>>>> rtc_time64_to_tm to generate a struct rtc_time with all elements properly
>>>> populated.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/rtc/interface.c | 24 ++++++++++++++++--------
>>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
>>>> index 8cec9a02..5a53c590 100644
>>>> --- a/drivers/rtc/interface.c
>>>> +++ b/drivers/rtc/interface.c
>>>> @@ -59,28 +59,36 @@ EXPORT_SYMBOL_GPL(rtc_read_time);
>>>>  
>>>>  int rtc_set_time(struct rtc_device *rtc, struct rtc_time *tm)
>>>>  {
>>>> +	time64_t secs64;
>>>>  	int err;
>>>>  
>>>>  	err = rtc_valid_tm(tm);
>>>>  	if (err != 0)
>>>>  		return err;
>>>>  
>>>> +	secs64 = rtc_tm_to_time64(tm);
>>>> +
>>>>  	err = mutex_lock_interruptible(&rtc->ops_lock);
>>>>  	if (err)
>>>>  		return err;
>>>>  
>>>>  	if (!rtc->ops)
>>>>  		err = -ENODEV;
>>>> -	else if (rtc->ops->set_time)
>>>> -		err = rtc->ops->set_time(rtc->dev.parent, tm);
>>>> -	else if (rtc->ops->set_mmss64) {
>>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>>> -
>>>> +	else if (rtc->ops->set_time) {
>>>> +		struct rtc_time tmp;
>>>> +
>>>> +		/*
>>>> +		 * element wday of tm may not be set, therefore use
>>>> +		 * rtc_time64_to_tm to generate a struct rtc_time
>>>> +		 * with all elements being properly populated
>>>> +		 */
>>>> +		rtc_time64_to_tm(secs64, &tmp);
>>>> +		err = rtc->ops->set_time(rtc->dev.parent, &tmp);
>>>> +	} else if (rtc->ops->set_mmss64)
>>>>  		err = rtc->ops->set_mmss64(rtc->dev.parent, secs64);
>>>> -	} else if (rtc->ops->set_mmss) {
>>>> -		time64_t secs64 = rtc_tm_to_time64(tm);
>>>> +	else if (rtc->ops->set_mmss)
>>>>  		err = rtc->ops->set_mmss(rtc->dev.parent, secs64);
>>>> -	} else
>>>> +	else
>>>>  		err = -EINVAL;
>>>>  
>>>>  	pm_stay_awake(rtc->dev.parent);
>>>> -- 
>>>> 2.14.1
>>>>
>>>
>>> -- 
>>> Alexandre Belloni, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>>
> 

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Alexandre Belloni]

On 27/08/2017 at 21:35:34 +0200, Heiner Kallweit wrote:
> When having a look at the MCP794XX datasheet it became clear why the
> mentioned weekday-related patch was submitted. This chip is quite
> strange regarding the possible alarm match conditions:
> 
> ALMxMSK<2:0>: Alarm Mask bits
> 000 = Seconds match
> 001 = Minutes match
> 010 = Hours match (logic takes into account 12-/24-hour operation)
> 011 = Day of week match
> 100 = Date match
> 101 = Reserved; do not use
> 110 = Reserved; do not use
> 111 = Seconds, Minutes, Hour, Day of Week, Date and Month
> 
> When not having a proper weekday you get only either seconds or
> minutes or hours or date match.
> 
> So it's best to ensure that the weekday is properly populated.
> However we can do this in the driver and don't have to touch the core.
> 

Can you do that only for mcp794xx? I received one earlier this month and
I'll separate the driver from rtc-ds1307.c at some point.

BTW, I've rebased and pushed some cleanup for this driver in:
https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/log/?h=rtc-ds1307

Can you base yourself on that?

Thanks,

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

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Heiner Kallweit]

Am 28.08.2017 um 09:54 schrieb Alexandre Belloni:
> On 27/08/2017 at 21:35:34 +0200, Heiner Kallweit wrote:
>> When having a look at the MCP794XX datasheet it became clear why the
>> mentioned weekday-related patch was submitted. This chip is quite
>> strange regarding the possible alarm match conditions:
>>
>> ALMxMSK<2:0>: Alarm Mask bits
>> 000 = Seconds match
>> 001 = Minutes match
>> 010 = Hours match (logic takes into account 12-/24-hour operation)
>> 011 = Day of week match
>> 100 = Date match
>> 101 = Reserved; do not use
>> 110 = Reserved; do not use
>> 111 = Seconds, Minutes, Hour, Day of Week, Date and Month
>>
>> When not having a proper weekday you get only either seconds or
>> minutes or hours or date match.
>>
>> So it's best to ensure that the weekday is properly populated.
>> However we can do this in the driver and don't have to touch the core.
>>
> 
> Can you do that only for mcp794xx? I received one earlier this month and
> I'll separate the driver from rtc-ds1307.c at some point.
> 
Sure, I'll submit a patch for this. Separating the driver for mcp794xx may
not be needed as it is not that different from the other chips supported
by this driver. Once I have my patch series proposal towards a ds1307_lib
in a little more clean state this should become clearer.

Regards, Heiner

> BTW, I've rebased and pushed some cleanup for this driver in:
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git/log/?h=rtc-ds1307
> 
> Can you base yourself on that?
> 
OK

> Thanks,
> 

Re: [PATCH] rtc: core: let rtc_set_time properly populate element wday

[Alexandre Belloni]

On 28/08/2017 at 20:13:25 +0200, Heiner Kallweit wrote:
> Am 28.08.2017 um 09:54 schrieb Alexandre Belloni:
> > On 27/08/2017 at 21:35:34 +0200, Heiner Kallweit wrote:
> >> When having a look at the MCP794XX datasheet it became clear why the
> >> mentioned weekday-related patch was submitted. This chip is quite
> >> strange regarding the possible alarm match conditions:
> >>
> >> ALMxMSK<2:0>: Alarm Mask bits
> >> 000 = Seconds match
> >> 001 = Minutes match
> >> 010 = Hours match (logic takes into account 12-/24-hour operation)
> >> 011 = Day of week match
> >> 100 = Date match
> >> 101 = Reserved; do not use
> >> 110 = Reserved; do not use
> >> 111 = Seconds, Minutes, Hour, Day of Week, Date and Month
> >>
> >> When not having a proper weekday you get only either seconds or
> >> minutes or hours or date match.
> >>
> >> So it's best to ensure that the weekday is properly populated.
> >> However we can do this in the driver and don't have to touch the core.
> >>
> > 
> > Can you do that only for mcp794xx? I received one earlier this month and
> > I'll separate the driver from rtc-ds1307.c at some point.
> > 
> Sure, I'll submit a patch for this. Separating the driver for mcp794xx may
> not be needed as it is not that different from the other chips supported
> by this driver. Once I have my patch series proposal towards a ds1307_lib
> in a little more clean state this should become clearer.
> 

I would like to find a better name than that but I didn't come up with
anything yet... feel free to get creative ;)


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