| Message ID | 20190430093212.28425-2-alexandre.belloni@bootlin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/4] rtc: digicolor: fix possible race condition | expand |
Hi Alexandre, On Tue, Apr 30 2019, Alexandre Belloni wrote: > While the range of REFERENCE + TIME is actually 33 bits, the counter > itself (TIME) is a 32-bits seconds counter. > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > drivers/rtc/rtc-digicolor.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c > index 5bb14c56bc9a..e6e16aaac254 100644 > --- a/drivers/rtc/rtc-digicolor.c > +++ b/drivers/rtc/rtc-digicolor.c > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, rtc); > > rtc->rtc_dev->ops = &dc_rtc_ops; > + rtc->rtc_dev->range_max = U32_MAX; Where can I find documentation on the meaning and usage of the range_max value? I could not find anything in the kernel source. baruch > > return rtc_register_device(rtc->rtc_dev); > }
On 30/04/2019 14:36:24+0300, Baruch Siach wrote: > Hi Alexandre, > > On Tue, Apr 30 2019, Alexandre Belloni wrote: > > > While the range of REFERENCE + TIME is actually 33 bits, the counter > > itself (TIME) is a 32-bits seconds counter. > > > > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > > --- > > drivers/rtc/rtc-digicolor.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c > > index 5bb14c56bc9a..e6e16aaac254 100644 > > --- a/drivers/rtc/rtc-digicolor.c > > +++ b/drivers/rtc/rtc-digicolor.c > > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, rtc); > > > > rtc->rtc_dev->ops = &dc_rtc_ops; > > + rtc->rtc_dev->range_max = U32_MAX; > > Where can I find documentation on the meaning and usage of the range_max > value? I could not find anything in the kernel source. > It should be set to the maximum UNIX timestamp the RTC can be set to while keeping range_min to range_max contiguous. In the digicolor case, you could go up to 8589934590 (Wed Mar 16 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm not sure it can also update DC_RTC_TIME safely.
Hi Alexandre, On Tue, Apr 30 2019, Alexandre Belloni wrote: > On 30/04/2019 14:36:24+0300, Baruch Siach wrote: >> Hi Alexandre, >> >> On Tue, Apr 30 2019, Alexandre Belloni wrote: >> >> > While the range of REFERENCE + TIME is actually 33 bits, the counter >> > itself (TIME) is a 32-bits seconds counter. >> > >> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >> > --- >> > drivers/rtc/rtc-digicolor.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c >> > index 5bb14c56bc9a..e6e16aaac254 100644 >> > --- a/drivers/rtc/rtc-digicolor.c >> > +++ b/drivers/rtc/rtc-digicolor.c >> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) >> > platform_set_drvdata(pdev, rtc); >> > >> > rtc->rtc_dev->ops = &dc_rtc_ops; >> > + rtc->rtc_dev->range_max = U32_MAX; >> >> Where can I find documentation on the meaning and usage of the range_max >> value? I could not find anything in the kernel source. > > It should be set to the maximum UNIX timestamp the RTC can be set to > while keeping range_min to range_max contiguous. > > In the digicolor case, you could go up to 8589934590 (Wed Mar 16 > 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm > not sure it can also update DC_RTC_TIME safely. DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that dc_rtc_write stores there. So the driver will return values larger than U32_MAX if you happen to cross this point in time between dc_rtc_write and dc_rtc_read. But you can't store a value larger than U32_MAX in DC_RTC_REFERENCE. Will the core RTC code handle the U32_MAX cross correctly? baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On 30/04/2019 15:20:08+0300, Baruch Siach wrote: > Hi Alexandre, > > On Tue, Apr 30 2019, Alexandre Belloni wrote: > > On 30/04/2019 14:36:24+0300, Baruch Siach wrote: > >> Hi Alexandre, > >> > >> On Tue, Apr 30 2019, Alexandre Belloni wrote: > >> > >> > While the range of REFERENCE + TIME is actually 33 bits, the counter > >> > itself (TIME) is a 32-bits seconds counter. > >> > > >> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > >> > --- > >> > drivers/rtc/rtc-digicolor.c | 1 + > >> > 1 file changed, 1 insertion(+) > >> > > >> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c > >> > index 5bb14c56bc9a..e6e16aaac254 100644 > >> > --- a/drivers/rtc/rtc-digicolor.c > >> > +++ b/drivers/rtc/rtc-digicolor.c > >> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) > >> > platform_set_drvdata(pdev, rtc); > >> > > >> > rtc->rtc_dev->ops = &dc_rtc_ops; > >> > + rtc->rtc_dev->range_max = U32_MAX; > >> > >> Where can I find documentation on the meaning and usage of the range_max > >> value? I could not find anything in the kernel source. > > > > It should be set to the maximum UNIX timestamp the RTC can be set to > > while keeping range_min to range_max contiguous. > > > > In the digicolor case, you could go up to 8589934590 (Wed Mar 16 > > 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm > > not sure it can also update DC_RTC_TIME safely. > > DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the > DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that > dc_rtc_write stores there. So the driver will return values larger than > U32_MAX if you happen to cross this point in time between dc_rtc_write > and dc_rtc_read. But you can't store a value larger than U32_MAX in > DC_RTC_REFERENCE. > > Will the core RTC code handle the U32_MAX cross correctly? > Yes, this is ok to return a valid value that is higher than range_max. However, at that time, you will not be able to set any alarms anymore as the core doesn't allow to set alarms after range_max. I would think that this is fine because this will happen in 2106 and we have a way to offset the time (the whole goal of setting the range) using device tree.
Hi Alexandre, On Tue, Apr 30 2019, Alexandre Belloni wrote: > On 30/04/2019 15:20:08+0300, Baruch Siach wrote: >> On Tue, Apr 30 2019, Alexandre Belloni wrote: >> > On 30/04/2019 14:36:24+0300, Baruch Siach wrote: >> >> Hi Alexandre, >> >> >> >> On Tue, Apr 30 2019, Alexandre Belloni wrote: >> >> >> >> > While the range of REFERENCE + TIME is actually 33 bits, the counter >> >> > itself (TIME) is a 32-bits seconds counter. >> >> > >> >> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> >> >> > --- >> >> > drivers/rtc/rtc-digicolor.c | 1 + >> >> > 1 file changed, 1 insertion(+) >> >> > >> >> > diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c >> >> > index 5bb14c56bc9a..e6e16aaac254 100644 >> >> > --- a/drivers/rtc/rtc-digicolor.c >> >> > +++ b/drivers/rtc/rtc-digicolor.c >> >> > @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) >> >> > platform_set_drvdata(pdev, rtc); >> >> > >> >> > rtc->rtc_dev->ops = &dc_rtc_ops; >> >> > + rtc->rtc_dev->range_max = U32_MAX; >> >> >> >> Where can I find documentation on the meaning and usage of the range_max >> >> value? I could not find anything in the kernel source. >> > >> > It should be set to the maximum UNIX timestamp the RTC can be set to >> > while keeping range_min to range_max contiguous. >> > >> > In the digicolor case, you could go up to 8589934590 (Wed Mar 16 >> > 12:56:30 UTC 2242) but the driver only writes DC_RTC_REFERENCE and I'm >> > not sure it can also update DC_RTC_TIME safely. >> >> DC_RTC_TIME resets to zero whenever dc_rtc_write writes CMD_RESET to the >> DC_RTC_CONTROL register. DC_RTC_REFERENCE keeps the value that >> dc_rtc_write stores there. So the driver will return values larger than >> U32_MAX if you happen to cross this point in time between dc_rtc_write >> and dc_rtc_read. But you can't store a value larger than U32_MAX in >> DC_RTC_REFERENCE. >> >> Will the core RTC code handle the U32_MAX cross correctly? > > Yes, this is ok to return a valid value that is higher than range_max. > However, at that time, you will not be able to set any alarms anymore as > the core doesn't allow to set alarms after range_max. > > I would think that this is fine because this will happen in 2106 and we > have a way to offset the time (the whole goal of setting the range) > using device tree. That's the sort of documentation that I'm missing. The 'start-year' property is mentioned in the DT binding documentation. But I don't see where range_max is documented as a facility for the time offset feature. Anyway, Acked-by: Baruch Siach <baruch@tkos.co.il> Thanks, baruch
On 30/04/2019 18:25:52+0300, Baruch Siach wrote: > > Yes, this is ok to return a valid value that is higher than range_max. > > However, at that time, you will not be able to set any alarms anymore as > > the core doesn't allow to set alarms after range_max. > > > > I would think that this is fine because this will happen in 2106 and we > > have a way to offset the time (the whole goal of setting the range) > > using device tree. > > That's the sort of documentation that I'm missing. The 'start-year' > property is mentioned in the DT binding documentation. But I don't see > where range_max is documented as a facility for the time offset feature. > Sure, I'm planning to document better how a proper RTC driver should be written. I needed to cleanup the digicolor driver because I4m removing .set_mmss and .set_mmss64 this cycle.
diff --git a/drivers/rtc/rtc-digicolor.c b/drivers/rtc/rtc-digicolor.c index 5bb14c56bc9a..e6e16aaac254 100644 --- a/drivers/rtc/rtc-digicolor.c +++ b/drivers/rtc/rtc-digicolor.c @@ -206,6 +206,7 @@ static int __init dc_rtc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, rtc); rtc->rtc_dev->ops = &dc_rtc_ops; + rtc->rtc_dev->range_max = U32_MAX; return rtc_register_device(rtc->rtc_dev); }
While the range of REFERENCE + TIME is actually 33 bits, the counter itself (TIME) is a 32-bits seconds counter. Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com> --- drivers/rtc/rtc-digicolor.c | 1 + 1 file changed, 1 insertion(+)