Re: [PATCH] drivers/rtc/interface.c: check the error after __rtc_read_time()
On Wed, Dec 17, 2014 at 01:51:50PM -0800, Andrew Morton wrote: > On Wed, 17 Dec 2014 12:15:24 +0900 Hyogi Gim wrote: > > > Add the verification code for returned __rtc_read_time() error in > > rtc_update_irq_enable() and rtc_timer_do_work(). > > > > ... > L > > --- a/drivers/rtc/interface.c > > +++ b/drivers/rtc/interface.c > > @@ -489,7 +489,10 @@ int rtc_update_irq_enable(struct rtc_device *rtc, > > unsigned int enabled) > > struct rtc_time tm; > > ktime_t now, onesec; > > > > - __rtc_read_time(rtc, &tm); > > + err = __rtc_read_time(rtc, &tm); > > + if (err < 0) > > + goto out; > > + > > onesec = ktime_set(1, 0); > > now = rtc_tm_to_ktime(tm); > > rtc->uie_rtctimer.node.expires = ktime_add(now, onesec); > > I'm not sure about this part. If __rtc_read_time() returns -EINVAL > (due to !rtc->ops->read_time) then rtc_update_irq_enable() will go and > call rtc_dev_update_irq_enable_emul(), inappropriately. > > On the other hand, if __rtc_read_time() returns -EINVAL because that's > what rtc->ops->read_time() returned then perhaps > rtc_update_irq_enable() *should* call rtc_dev_update_irq_enable_emul(). > > Messy. > As you said, if rtc driver has no read_time callback, rtc_read_time() is failed in rtc_dev_update_irq_enable_emul() anyway. rtc_dev_update_irq_enable_emul() |_ set_uie() |_ rtc_read_time() What I worried about the error from rtc->ops->read_time(). If rtc->ops->read_time() returns another type of error except -EINVAL, then rtc interface can't run rtc_dev_update_irq_enable_emul(). I think it needs to be changed that check to ensure error for rtc_dev_update_irq_enable_emul(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/rtc/interface.c: check the error after __rtc_read_time()
Add the verification code for returned __rtc_read_time() error in
rtc_update_irq_enable() and rtc_timer_do_work().
Signed-off-by: Hyogi Gim
---
drivers/rtc/interface.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 45bfc28ee..fa1f119 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -489,7 +489,10 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned
int enabled)
struct rtc_time tm;
ktime_t now, onesec;
- __rtc_read_time(rtc, &tm);
+ err = __rtc_read_time(rtc, &tm);
+ if (err < 0)
+ goto out;
+
onesec = ktime_set(1, 0);
now = rtc_tm_to_ktime(tm);
rtc->uie_rtctimer.node.expires = ktime_add(now, onesec);
@@ -867,13 +870,17 @@ void rtc_timer_do_work(struct work_struct *work)
struct timerqueue_node *next;
ktime_t now;
struct rtc_time tm;
+ int err = 0;
struct rtc_device *rtc =
container_of(work, struct rtc_device, irqwork);
mutex_lock(&rtc->ops_lock);
again:
- __rtc_read_time(rtc, &tm);
+ err = __rtc_read_time(rtc, &tm);
+ if (err < 0)
+ goto out;
+
now = rtc_tm_to_ktime(tm);
while ((next = timerqueue_getnext(&rtc->timerqueue))) {
if (next->expires.tv64 > now.tv64)
@@ -898,7 +905,6 @@ again:
/* Set next alarm */
if (next) {
struct rtc_wkalrm alarm;
- int err;
int retry = 3;
alarm.time = rtc_ktime_to_tm(next->expires);
@@ -920,6 +926,7 @@ reprogram:
} else
rtc_alarm_disable(rtc);
+out:
pm_relax(rtc->dev.parent);
mutex_unlock(&rtc->ops_lock);
}
--
1.8.3.2
--
Hyogi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers/rtc/interface.c: check the validation of rtc_time in __rtc_read_time
On Tue, Oct 14, 2014 at 03:21:31PM -0700, Andrew Morton wrote: > On Wed, 8 Oct 2014 11:05:29 +0900 Hyogi Gim wrote: > > > Some of rtc devices always return '0' when rtc_class_ops.read_time is > > called. > > So if rtc_time isn't verified in callback, rtc interface cannot know whether > > rtc_time is valid. > > We should fix the buggy .read_time() implementations... > Thanks for comments. I'm studying rtc framework and trying to find a problem. This is a minor bug. But, as you know, it can be a big problem for newbies like me. Even though it was late, I send you reply because a question. If this patch is merged, rtc_valid_tm() in rtc_class_ops.read_time callback is not necessary anymore. So, I think each rtc device driver should change the code. I can change the code. But, test is a little different. In this case, how can I progress this modification? Can I just send the notification for rtc_valid_tm() to the driver manufacturer? or, request test for the code that I've modified? -- Hyogi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/rtc/interface.c: check the validation of rtc_time in __rtc_read_time
Some of rtc devices always return '0' when rtc_class_ops.read_time is called.
So if rtc_time isn't verified in callback, rtc interface cannot know whether
rtc_time is valid.
Check rtc_time by using 'rtc_valid_tm' in '__rtc_read_time'. And add the
message for debugging.
Signed-off-by: Hyogi Gim
---
drivers/rtc/interface.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index 5b2717f..818ea97 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -30,6 +30,14 @@ static int __rtc_read_time(struct rtc_device *rtc, struct
rtc_time *tm)
else {
memset(tm, 0, sizeof(struct rtc_time));
err = rtc->ops->read_time(rtc->dev.parent, tm);
+ if (err < 0) {
+ dev_err(&rtc->dev, "read_time: fail to read\n");
+ return err;
+ }
+
+ err = rtc_valid_tm(tm);
+ if (err < 0)
+ dev_err(&rtc->dev, "read_time: rtc_time isn't valid\n");
}
return err;
}
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] alarmtimer: Add the verification code for rtc device error.
On 07/31/2014 02:58 AM, John Stultz wrote: > > Hey! Sorry for the late response here. > > So this seems reasonable as always failing suspend is problematic, but > I worry that for the case where we do have a failure to read or set > the RTC, we'd suspend and not wake up as specified, which is an issue > as well. Absorbing the error silently in these cases would make it > difficult to debug. Should we at least print some output out to help > folks hunt down this sort of issue? > I agree. Most RTC device drivers don't print out the error of read_time. So, I think the higher level like /drivers/rtc/interface.c should check the error and give the information of the RTC device status. I'll try to find a proper point. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] driver/rtc/class.c: check the error after rtc_read_time()
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] driver/rtc/class.c: check the error after rtc_read_time()
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] alarmtimer: Add the verification code for rtc device error.
In alarmtimer_suspend(), the error after rtc_read_time() is not checked. If rtc device fail to read rtc time, we cannot ensure the following process. Furthermore, the return value of rtc_timer_start() needs to distinguish -ETIME and other rtc device error. If the error is relevant to rtc device, suspend is failed unintentionally. In this case, it just returns zero for a stable suspend. Otherwise, in the worst case, suspend will fail continually. So, this patch verifies the rtc device error in alarmtimer_suspend(). it includes "rtc_err goto" statement instead of a direct "return 0" to clarify the rtc device error. Signed-off-by: Hyogi Gim --- kernel/time/alarmtimer.c | 12 ++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 88c9c65..8cc43b8 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -261,15 +261,23 @@ static int alarmtimer_suspend(struct device *dev) /* Setup an rtc timer to fire that far in the future */ rtc_timer_cancel(rtc, &rtctimer); - rtc_read_time(rtc, &tm); + ret = rtc_read_time(rtc, &tm); + if (ret < 0) + goto rtc_err; now = rtc_tm_to_ktime(tm); now = ktime_add(now, min); /* Set alarm, if in the past reject suspend briefly to handle */ ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0)); - if (ret < 0) + if (ret == -ETIME) __pm_wakeup_event(ws, MSEC_PER_SEC); + else if (ret < 0) + goto rtc_err; + return ret; + +rtc_err: + return 0; } #else static int alarmtimer_suspend(struct device *dev) -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] alarmtimers: verify the alarmtimer_type value from clock2alarm()
clock2alarm() can return a minus value. so, we cannot use this
returned value for a index of an array. but, some functions use
this value directly as a index of an array:
- alarm_clock_getres()
- alarm_clock_get()
- alarm_timer_create()
- alarm_timer_nsleep()
add the verification code for the returned alarmtimer_type from
clock2alarm().
Signed-off-by: Hyogi Gim
Cc: Thomas Gleixner
---
kernel/time/alarmtimer.c | 27 ---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index 88c9c65..0b117c6 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -487,7 +487,14 @@ static enum alarmtimer_restart alarm_handle_timer(struct
alarm *alarm,
*/
static int alarm_clock_getres(const clockid_t which_clock, struct timespec *tp)
{
- clockid_t baseid = alarm_bases[clock2alarm(which_clock)].base_clockid;
+ enum alarmtimer_type type;
+ clockid_t baseid;
+
+ type = clock2alarm(which_clock);
+ if (type < 0)
+ return -EINVAL;
+
+ baseid = alarm_bases[type].base_clockid;
if (!alarmtimer_get_rtcdev())
return -EINVAL;
@@ -504,7 +511,14 @@ static int alarm_clock_getres(const clockid_t which_clock,
struct timespec *tp)
*/
static int alarm_clock_get(clockid_t which_clock, struct timespec *tp)
{
- struct alarm_base *base = &alarm_bases[clock2alarm(which_clock)];
+ enum alarmtimer_type type;
+ struct alarm_base *base;
+
+ type = clock2alarm(which_clock);
+ if (type < 0)
+ return -EINVAL;
+
+ base = &alarm_bases[type];
if (!alarmtimer_get_rtcdev())
return -EINVAL;
@@ -531,6 +545,9 @@ static int alarm_timer_create(struct k_itimer *new_timer)
return -EPERM;
type = clock2alarm(new_timer->it_clock);
+ if (type < 0)
+ return -EINVAL;
+
base = &alarm_bases[type];
alarm_init(&new_timer->it.alarm.alarmtimer, type, alarm_handle_timer);
return 0;
@@ -721,7 +738,7 @@ out:
static int alarm_timer_nsleep(const clockid_t which_clock, int flags,
struct timespec *tsreq, struct timespec __user *rmtp)
{
- enum alarmtimer_type type = clock2alarm(which_clock);
+ enum alarmtimer_type type;
struct alarm alarm;
ktime_t exp;
int ret = 0;
@@ -733,6 +750,10 @@ static int alarm_timer_nsleep(const clockid_t which_clock,
int flags,
if (!capable(CAP_WAKE_ALARM))
return -EPERM;
+ type = clock2alarm(which_clock);
+ if (type < 0)
+ return -EINVAL;
+
alarm_init(&alarm, type, alarmtimer_nsleep_wakeup);
exp = timespec_to_ktime(*tsreq);
--
1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] drivers/rtc/interface.c: check the error after __rtc_read_time()
In __rtc_set_alarm(), 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 --- drivers/rtc/interface.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index 5813fa5..5b2717f 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -348,6 +348,8 @@ static int __rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm) /* Make sure we're not setting alarms in the past */ err = __rtc_read_time(rtc, &tm); + if (err) + return err; rtc_tm_to_time(&tm, &now); if (scheduled <= now) return -ETIME; -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
