| Message ID | 20211019131724.3109-5-semen.protsenko@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rtc: s3c: S3C driver improvements | expand |
On 19/10/2021 16:17:24+0300, Sam Protsenko wrote: > On first RTC boot it has the month register value set to 0. > Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads > to the next error message in kernel log: > > hctosys: unable to read the hardware clock > > That happens in s3c_rtc_probe() when trying to register the RTC, which > in turn tries to read and validate the time. Initialize RTC date/time > registers to valid values in probe function on the first boot to prevent > such errors. > No, never ever do that, the time is bogus and it has to stay this way, else userspace can't know whether the time on the RTC is the actual wall time or just some random value that you have set from the driver. > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > index 238928e29fbc..c7e763bcf61f 100644 > --- a/drivers/rtc/rtc-s3c.c > +++ b/drivers/rtc/rtc-s3c.c > @@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev) > return 0; > } > > +/* Set RTC with valid date/time values on first boot */ > +static int s3c_rtc_init_time(struct s3c_rtc *info) > +{ > + struct rtc_time tm; > + int ret; > + > + ret = s3c_rtc_read_time(info, &tm); > + if (ret) > + return ret; > + > + /* Only init RTC date/time on first boot */ > + if (tm.tm_mday > 0) > + return 0; > + > + /* Init date/time: 1 Jan 2000 00:00:00 */ > + memset(&tm, 0, sizeof(struct rtc_time)); > + tm.tm_mday = 1; /* tm_mday min valid value is 1 */ > + tm.tm_mon = 1; /* January in internal representation */ > + > + return s3c_rtc_write_time(info, &tm); > +} > + > static int s3c_rtc_probe(struct platform_device *pdev) > { > struct s3c_rtc *info = NULL; > @@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev) > > device_init_wakeup(&pdev->dev, 1); > > + ret = s3c_rtc_init_time(info); > + if (ret) > + goto err_nortc; > + > info->rtc = devm_rtc_allocate_device(&pdev->dev); > if (IS_ERR(info->rtc)) { > ret = PTR_ERR(info->rtc); > -- > 2.30.2 >
On Tue, 19 Oct 2021 at 18:48, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 19/10/2021 16:17:24+0300, Sam Protsenko wrote: > > On first RTC boot it has the month register value set to 0. > > Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads > > to the next error message in kernel log: > > > > hctosys: unable to read the hardware clock > > > > That happens in s3c_rtc_probe() when trying to register the RTC, which > > in turn tries to read and validate the time. Initialize RTC date/time > > registers to valid values in probe function on the first boot to prevent > > such errors. > > > > No, never ever do that, the time is bogus and it has to stay this way, > else userspace can't know whether the time on the RTC is the actual wall > time or just some random value that you have set from the driver. > Thought about that, but that error message looked distracting and not very helpful in understanding what's actually going on. Anyway, can you please drop this patch from series (and maybe [PATCH 3/4] too) and apply the rest? > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > > index 238928e29fbc..c7e763bcf61f 100644 > > --- a/drivers/rtc/rtc-s3c.c > > +++ b/drivers/rtc/rtc-s3c.c > > @@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev) > > return 0; > > } > > > > +/* Set RTC with valid date/time values on first boot */ > > +static int s3c_rtc_init_time(struct s3c_rtc *info) > > +{ > > + struct rtc_time tm; > > + int ret; > > + > > + ret = s3c_rtc_read_time(info, &tm); > > + if (ret) > > + return ret; > > + > > + /* Only init RTC date/time on first boot */ > > + if (tm.tm_mday > 0) > > + return 0; > > + > > + /* Init date/time: 1 Jan 2000 00:00:00 */ > > + memset(&tm, 0, sizeof(struct rtc_time)); > > + tm.tm_mday = 1; /* tm_mday min valid value is 1 */ > > + tm.tm_mon = 1; /* January in internal representation */ > > + > > + return s3c_rtc_write_time(info, &tm); > > +} > > + > > static int s3c_rtc_probe(struct platform_device *pdev) > > { > > struct s3c_rtc *info = NULL; > > @@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev) > > > > device_init_wakeup(&pdev->dev, 1); > > > > + ret = s3c_rtc_init_time(info); > > + if (ret) > > + goto err_nortc; > > + > > info->rtc = devm_rtc_allocate_device(&pdev->dev); > > if (IS_ERR(info->rtc)) { > > ret = PTR_ERR(info->rtc); > > -- > > 2.30.2 > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 19/10/2021 17:48, Alexandre Belloni wrote: > On 19/10/2021 16:17:24+0300, Sam Protsenko wrote: >> On first RTC boot it has the month register value set to 0. >> Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads >> to the next error message in kernel log: >> >> hctosys: unable to read the hardware clock >> >> That happens in s3c_rtc_probe() when trying to register the RTC, which >> in turn tries to read and validate the time. Initialize RTC date/time >> registers to valid values in probe function on the first boot to prevent >> such errors. >> > > No, never ever do that, the time is bogus and it has to stay this way, > else userspace can't know whether the time on the RTC is the actual wall > time or just some random value that you have set from the driver. > Indeed. This looks basically like a revert of your commit 5c78cceeb2d8 ("rtc: s3c: stop setting bogus time"). For the Samsung PMIC RTC, we dropped time initialization in commit fe787a5b2297 ("rtc: s5m: remove undocumented time init on first boot"). Best regards, Krzysztof
On 19/10/2021 18:04, Sam Protsenko wrote: > On Tue, 19 Oct 2021 at 18:48, Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: >> >> On 19/10/2021 16:17:24+0300, Sam Protsenko wrote: >>> On first RTC boot it has the month register value set to 0. >>> Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads >>> to the next error message in kernel log: >>> >>> hctosys: unable to read the hardware clock >>> >>> That happens in s3c_rtc_probe() when trying to register the RTC, which >>> in turn tries to read and validate the time. Initialize RTC date/time >>> registers to valid values in probe function on the first boot to prevent >>> such errors. >>> >> >> No, never ever do that, the time is bogus and it has to stay this way, >> else userspace can't know whether the time on the RTC is the actual wall >> time or just some random value that you have set from the driver. >> > > Thought about that, but that error message looked distracting and not > very helpful in understanding what's actually going on. Anyway, can > you please drop this patch from series (and maybe [PATCH 3/4] too) and > apply the rest? > Please give it some time for review. Pinging after few hours is too fast. Best regards, Krzysztof
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c index 238928e29fbc..c7e763bcf61f 100644 --- a/drivers/rtc/rtc-s3c.c +++ b/drivers/rtc/rtc-s3c.c @@ -403,6 +403,28 @@ static int s3c_rtc_remove(struct platform_device *pdev) return 0; } +/* Set RTC with valid date/time values on first boot */ +static int s3c_rtc_init_time(struct s3c_rtc *info) +{ + struct rtc_time tm; + int ret; + + ret = s3c_rtc_read_time(info, &tm); + if (ret) + return ret; + + /* Only init RTC date/time on first boot */ + if (tm.tm_mday > 0) + return 0; + + /* Init date/time: 1 Jan 2000 00:00:00 */ + memset(&tm, 0, sizeof(struct rtc_time)); + tm.tm_mday = 1; /* tm_mday min valid value is 1 */ + tm.tm_mon = 1; /* January in internal representation */ + + return s3c_rtc_write_time(info, &tm); +} + static int s3c_rtc_probe(struct platform_device *pdev) { struct s3c_rtc *info = NULL; @@ -471,6 +493,10 @@ static int s3c_rtc_probe(struct platform_device *pdev) device_init_wakeup(&pdev->dev, 1); + ret = s3c_rtc_init_time(info); + if (ret) + goto err_nortc; + info->rtc = devm_rtc_allocate_device(&pdev->dev); if (IS_ERR(info->rtc)) { ret = PTR_ERR(info->rtc);
On first RTC boot it has the month register value set to 0. Unconditional subtracting of 1 subsequently in s3c_rtc_gettime() leads to the next error message in kernel log: hctosys: unable to read the hardware clock That happens in s3c_rtc_probe() when trying to register the RTC, which in turn tries to read and validate the time. Initialize RTC date/time registers to valid values in probe function on the first boot to prevent such errors. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- drivers/rtc/rtc-s3c.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)