* [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
@ 2014-07-15 8:25 Hyogi Gim
2014-07-23 21:56 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Hyogi Gim @ 2014-07-15 8:25 UTC (permalink / raw)
To: Alessandro Zummo; +Cc: rtc-linux, linux-kernel, Hyogi Gim
In rtc_suspend() and rtc_resume(), the error after rtc_read_time() is not
checked. If rtc device fail to read time, we cannot guarantee the following
process.
Add the verification code for returned rtc_read_time() error.
Signed-off-by: Hyogi Gim <hyogi.gim@lge.com>
---
drivers/rtc/class.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 589351e..38e26be 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -53,6 +53,7 @@ static int rtc_suspend(struct device *dev)
struct rtc_device *rtc = to_rtc_device(dev);
struct rtc_time tm;
struct timespec delta, delta_delta;
+ int err;
if (has_persistent_clock())
return 0;
@@ -61,7 +62,12 @@ static int rtc_suspend(struct device *dev)
return 0;
/* snapshot the current RTC and system time at suspend*/
- rtc_read_time(rtc, &tm);
+ err = rtc_read_time(rtc, &tm);
+ if (err < 0) {
+ pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
+ return 0;
+ }
+
getnstimeofday(&old_system);
rtc_tm_to_time(&tm, &old_rtc.tv_sec);
@@ -94,6 +100,7 @@ static int rtc_resume(struct device *dev)
struct rtc_time tm;
struct timespec new_system, new_rtc;
struct timespec sleep_time;
+ int err;
if (has_persistent_clock())
return 0;
@@ -104,7 +111,12 @@ static int rtc_resume(struct device *dev)
/* snapshot the current rtc and system time at resume */
getnstimeofday(&new_system);
- rtc_read_time(rtc, &tm);
+ err = rtc_read_time(rtc, &tm);
+ if (err < 0) {
+ pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
+ return 0;
+ }
+
if (rtc_valid_tm(&tm) != 0) {
pr_debug("%s: bogus resume time\n", dev_name(&rtc->dev));
return 0;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
2014-07-15 8:25 [PATCH] driver/rtc/class.c: check the error after rtc_read_time() Hyogi Gim
@ 2014-07-23 21:56 ` Andrew Morton
2014-07-23 23:47 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2014-07-23 21:56 UTC (permalink / raw)
To: Hyogi Gim; +Cc: Alessandro Zummo, rtc-linux, linux-kernel, Rafael J. Wysocki
On Tue, 15 Jul 2014 17:25:23 +0900 Hyogi Gim <hyogi.gim@lge.com> wrote:
> In rtc_suspend() and rtc_resume(), the error after rtc_read_time() is not
> checked. If rtc device fail to read time, we cannot guarantee the following
> process.
>
> Add the verification code for returned rtc_read_time() error.
>
> ...
>
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -53,6 +53,7 @@ static int rtc_suspend(struct device *dev)
> struct rtc_device *rtc = to_rtc_device(dev);
> struct rtc_time tm;
> struct timespec delta, delta_delta;
> + int err;
>
> if (has_persistent_clock())
> return 0;
> @@ -61,7 +62,12 @@ static int rtc_suspend(struct device *dev)
> return 0;
>
> /* snapshot the current RTC and system time at suspend*/
> - rtc_read_time(rtc, &tm);
> + err = rtc_read_time(rtc, &tm);
> + if (err < 0) {
> + pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
> + return 0;
> + }
OK, it makes no sense to go ahead and set the system time from a
garbage rtc_time.
But I'm wondering if we should propagate the error back to the
rtc_suspend() caller. What does the PM core do if a particular
device's ->suspend or ->resume fails?
> getnstimeofday(&old_system);
> rtc_tm_to_time(&tm, &old_rtc.tv_sec);
>
> @@ -94,6 +100,7 @@ static int rtc_resume(struct device *dev)
> struct rtc_time tm;
> struct timespec new_system, new_rtc;
> struct timespec sleep_time;
> + int err;
>
> if (has_persistent_clock())
> return 0;
> @@ -104,7 +111,12 @@ static int rtc_resume(struct device *dev)
>
> /* snapshot the current rtc and system time at resume */
> getnstimeofday(&new_system);
> - rtc_read_time(rtc, &tm);
> + err = rtc_read_time(rtc, &tm);
> + if (err < 0) {
> + pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
> + return 0;
> + }
> +
> if (rtc_valid_tm(&tm) != 0) {
> pr_debug("%s: bogus resume time\n", dev_name(&rtc->dev));
> return 0;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
2014-07-23 21:56 ` Andrew Morton
@ 2014-07-23 23:47 ` Rafael J. Wysocki
2014-07-23 23:49 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-07-23 23:47 UTC (permalink / raw)
To: Andrew Morton
Cc: Hyogi Gim, Alessandro Zummo, rtc-linux, linux-kernel,
Rafael J. Wysocki
On Wednesday, July 23, 2014 02:56:34 PM Andrew Morton wrote:
> On Tue, 15 Jul 2014 17:25:23 +0900 Hyogi Gim <hyogi.gim@lge.com> wrote:
>
> > In rtc_suspend() and rtc_resume(), the error after rtc_read_time() is not
> > checked. If rtc device fail to read time, we cannot guarantee the following
> > process.
> >
> > Add the verification code for returned rtc_read_time() error.
> >
> > ...
> >
> > --- a/drivers/rtc/class.c
> > +++ b/drivers/rtc/class.c
> > @@ -53,6 +53,7 @@ static int rtc_suspend(struct device *dev)
> > struct rtc_device *rtc = to_rtc_device(dev);
> > struct rtc_time tm;
> > struct timespec delta, delta_delta;
> > + int err;
> >
> > if (has_persistent_clock())
> > return 0;
> > @@ -61,7 +62,12 @@ static int rtc_suspend(struct device *dev)
> > return 0;
> >
> > /* snapshot the current RTC and system time at suspend*/
> > - rtc_read_time(rtc, &tm);
> > + err = rtc_read_time(rtc, &tm);
> > + if (err < 0) {
> > + pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
> > + return 0;
> > + }
>
> OK, it makes no sense to go ahead and set the system time from a
> garbage rtc_time.
>
> But I'm wondering if we should propagate the error back to the
> rtc_suspend() caller. What does the PM core do if a particular
> device's ->suspend or ->resume fails?
It aborts the suspend.
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
2014-07-23 23:47 ` Rafael J. Wysocki
@ 2014-07-23 23:49 ` Rafael J. Wysocki
2014-07-24 0:19 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-07-23 23:49 UTC (permalink / raw)
To: Andrew Morton
Cc: Hyogi Gim, Alessandro Zummo, rtc-linux, linux-kernel,
Rafael J. Wysocki
On Thursday, July 24, 2014 01:47:57 AM Rafael J. Wysocki wrote:
> On Wednesday, July 23, 2014 02:56:34 PM Andrew Morton wrote:
> > On Tue, 15 Jul 2014 17:25:23 +0900 Hyogi Gim <hyogi.gim@lge.com> wrote:
> >
> > > In rtc_suspend() and rtc_resume(), the error after rtc_read_time() is not
> > > checked. If rtc device fail to read time, we cannot guarantee the following
> > > process.
> > >
> > > Add the verification code for returned rtc_read_time() error.
> > >
> > > ...
> > >
> > > --- a/drivers/rtc/class.c
> > > +++ b/drivers/rtc/class.c
> > > @@ -53,6 +53,7 @@ static int rtc_suspend(struct device *dev)
> > > struct rtc_device *rtc = to_rtc_device(dev);
> > > struct rtc_time tm;
> > > struct timespec delta, delta_delta;
> > > + int err;
> > >
> > > if (has_persistent_clock())
> > > return 0;
> > > @@ -61,7 +62,12 @@ static int rtc_suspend(struct device *dev)
> > > return 0;
> > >
> > > /* snapshot the current RTC and system time at suspend*/
> > > - rtc_read_time(rtc, &tm);
> > > + err = rtc_read_time(rtc, &tm);
> > > + if (err < 0) {
> > > + pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
> > > + return 0;
> > > + }
> >
> > OK, it makes no sense to go ahead and set the system time from a
> > garbage rtc_time.
> >
> > But I'm wondering if we should propagate the error back to the
> > rtc_suspend() caller. What does the PM core do if a particular
> > device's ->suspend or ->resume fails?
>
> It aborts the suspend.
I mean, if ->suspend fails, the suspend is aborted.
If ->resume fails, on the other hand, we cannot do much more than logging
an error message.
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
2014-07-23 23:49 ` Rafael J. Wysocki
@ 2014-07-24 0:19 ` Andrew Morton
2014-08-07 1:58 ` Hyogi Gim
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2014-07-24 0:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Hyogi Gim, Alessandro Zummo, rtc-linux, linux-kernel,
Rafael J. Wysocki
On Thu, 24 Jul 2014 01:49:44 +0200 "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
> On Thursday, July 24, 2014 01:47:57 AM Rafael J. Wysocki wrote:
> > On Wednesday, July 23, 2014 02:56:34 PM Andrew Morton wrote:
> > > On Tue, 15 Jul 2014 17:25:23 +0900 Hyogi Gim <hyogi.gim@lge.com> wrote:
> > >
> > > > In rtc_suspend() and rtc_resume(), the error after rtc_read_time() is not
> > > > checked. If rtc device fail to read time, we cannot guarantee the following
> > > > process.
> > > >
> > > > Add the verification code for returned rtc_read_time() error.
> > > >
> > > > ...
> > > >
> > > > --- a/drivers/rtc/class.c
> > > > +++ b/drivers/rtc/class.c
> > > > @@ -53,6 +53,7 @@ static int rtc_suspend(struct device *dev)
> > > > struct rtc_device *rtc = to_rtc_device(dev);
> > > > struct rtc_time tm;
> > > > struct timespec delta, delta_delta;
> > > > + int err;
> > > >
> > > > if (has_persistent_clock())
> > > > return 0;
> > > > @@ -61,7 +62,12 @@ static int rtc_suspend(struct device *dev)
> > > > return 0;
> > > >
> > > > /* snapshot the current RTC and system time at suspend*/
> > > > - rtc_read_time(rtc, &tm);
> > > > + err = rtc_read_time(rtc, &tm);
> > > > + if (err < 0) {
> > > > + pr_debug("%s: fail to read rtc time\n", dev_name(&rtc->dev));
> > > > + return 0;
> > > > + }
> > >
> > > OK, it makes no sense to go ahead and set the system time from a
> > > garbage rtc_time.
> > >
> > > But I'm wondering if we should propagate the error back to the
> > > rtc_suspend() caller. What does the PM core do if a particular
> > > device's ->suspend or ->resume fails?
> >
> > It aborts the suspend.
>
> I mean, if ->suspend fails, the suspend is aborted.
So what should rtc do in this case? At present it pretends the read
succeeded. Either way, this doesn't seem to be the place to be making
such policy decisions..
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
2014-07-24 0:19 ` Andrew Morton
@ 2014-08-07 1:58 ` Hyogi Gim
0 siblings, 0 replies; 6+ messages in thread
From: Hyogi Gim @ 2014-08-07 1:58 UTC (permalink / raw)
To: Andrew Morton, Rafael J. Wysocki
Cc: Alessandro Zummo, rtc-linux, linux-kernel, Rafael J. Wysocki
On 07/24/2014 09:19 AM, Andrew Morton wrote:
>
> So what should rtc do in this case? At present it pretends the read
> succeeded. Either way, this doesn't seem to be the place to be making
> such policy decisions..
>
>
>
I agree. But, in this case, RTC device driver can not do anything. And if
rtc_suspend() returns a minus value, then suspend will be aborted. So,
in the worst case, suspend will be failed continually. I think this is not
good.
Most RTC device drivers don't verify the read time value. Even some drivers
just return '0' value(omap, tegra, ...). So, I think the higher level
framework like /drivers/rtc/interface.c should check and handle the rtc
read time.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-07 1:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-15 8:25 [PATCH] driver/rtc/class.c: check the error after rtc_read_time() Hyogi Gim
2014-07-23 21:56 ` Andrew Morton
2014-07-23 23:47 ` Rafael J. Wysocki
2014-07-23 23:49 ` Rafael J. Wysocki
2014-07-24 0:19 ` Andrew Morton
2014-08-07 1:58 ` Hyogi Gim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox