| Message ID | 3b43da73-ff4e-4519-d1c7-266b17cc5d05@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
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 >
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
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 >
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 >> >
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,
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, >
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 ;)
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);
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(-)