2010-02-08 21:10:49

by Uwe Kleine-König

[permalink] [raw]
Subject: [PATCH] rtc/hctosys: only claim the RTC provided the system time if it did

Moreover change error handling in rtc_hctosys() to use goto and so
reduce the indention level.

Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/rtc/hctosys.c | 59 ++++++++++++++++++++++++++++--------------------
include/linux/rtc.h | 6 +++++
2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
index 33c0e98..bc90b09 100644
--- a/drivers/rtc/hctosys.c
+++ b/drivers/rtc/hctosys.c
@@ -22,48 +22,57 @@
* the best guess is to add 0.5s.
*/

+int rtc_hctosys_ret = -ENODEV;
+
static int __init rtc_hctosys(void)
{
- int err;
+ int err = -ENODEV;
struct rtc_time tm;
+ struct timespec tv = {
+ .tv_nsec = NSEC_PER_SEC >> 1,
+ };
struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);

if (rtc == NULL) {
- printk("%s: unable to open rtc device (%s)\n",
+ pr_err("%s: unable to open rtc device (%s)\n",
__FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
- return -ENODEV;
+ goto err_open;
}

err = rtc_read_time(rtc, &tm);
- if (err == 0) {
- err = rtc_valid_tm(&tm);
- if (err == 0) {
- struct timespec tv;
+ if (err) {
+ dev_err(rtc->dev.parent,
+ "hctosys: unable to read the hardware clock\n");
+ goto err_read;

- tv.tv_nsec = NSEC_PER_SEC >> 1;
+ }

- rtc_tm_to_time(&tm, &tv.tv_sec);
+ err = rtc_valid_tm(&tm);
+ if (err) {
+ dev_err(rtc->dev.parent,
+ "hctosys: invalid date/time\n");
+ goto err_invalid;
+ }

- do_settimeofday(&tv);
+ rtc_tm_to_time(&tm, &tv.tv_sec);

- dev_info(rtc->dev.parent,
- "setting system clock to "
- "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
- tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
- tm.tm_hour, tm.tm_min, tm.tm_sec,
- (unsigned int) tv.tv_sec);
- }
- else
- dev_err(rtc->dev.parent,
- "hctosys: invalid date/time\n");
- }
- else
- dev_err(rtc->dev.parent,
- "hctosys: unable to read the hardware clock\n");
+ do_settimeofday(&tv);

+ dev_info(rtc->dev.parent,
+ "setting system clock to "
+ "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+ tm.tm_hour, tm.tm_min, tm.tm_sec,
+ (unsigned int) tv.tv_sec);
+
+err_invalid:
+err_read:
rtc_class_close(rtc);

- return 0;
+err_open:
+ rtc_hctosys_ret = err;
+
+ return err;
}

late_initcall(rtc_hctosys);
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 60f88a7..14dbc83 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -238,6 +238,12 @@ static inline bool is_leap_year(unsigned int year)
return (!(year % 4) && (year % 100)) || !(year % 400);
}

+#ifdef CONFIG_RTC_HCTOSYS
+extern int rtc_hctosys_ret;
+#else
+#define rtc_hctosys_ret -ENODEV
+#endif
+
#endif /* __KERNEL__ */

#endif /* _LINUX_RTC_H_ */
--
1.6.6


2010-02-08 21:21:15

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH] rtc/hctosys: only claim the RTC provided the system time if it did

Hello,

On Mon, Feb 08, 2010 at 10:10:18PM +0100, Uwe Kleine-K?nig wrote:
> Moreover change error handling in rtc_hctosys() to use goto and so
> reduce the indention level.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> drivers/rtc/hctosys.c | 59 ++++++++++++++++++++++++++++--------------------
> include/linux/rtc.h | 6 +++++
> 2 files changed, 40 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
> index 33c0e98..bc90b09 100644
> --- a/drivers/rtc/hctosys.c
> +++ b/drivers/rtc/hctosys.c
> @@ -22,48 +22,57 @@
> * the best guess is to add 0.5s.
> */
>
> +int rtc_hctosys_ret = -ENODEV;
> +
> static int __init rtc_hctosys(void)
> {
> - int err;
> + int err = -ENODEV;
> struct rtc_time tm;
> + struct timespec tv = {
> + .tv_nsec = NSEC_PER_SEC >> 1,
> + };
> struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
>
> if (rtc == NULL) {
> - printk("%s: unable to open rtc device (%s)\n",
> + pr_err("%s: unable to open rtc device (%s)\n",
> __FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
> - return -ENODEV;
> + goto err_open;
> }
>
> err = rtc_read_time(rtc, &tm);
> - if (err == 0) {
> - err = rtc_valid_tm(&tm);
> - if (err == 0) {
> - struct timespec tv;
> + if (err) {
> + dev_err(rtc->dev.parent,
> + "hctosys: unable to read the hardware clock\n");
> + goto err_read;
>
> - tv.tv_nsec = NSEC_PER_SEC >> 1;
> + }
>
> - rtc_tm_to_time(&tm, &tv.tv_sec);
> + err = rtc_valid_tm(&tm);
> + if (err) {
> + dev_err(rtc->dev.parent,
> + "hctosys: invalid date/time\n");
> + goto err_invalid;
> + }
>
> - do_settimeofday(&tv);
> + rtc_tm_to_time(&tm, &tv.tv_sec);
>
> - dev_info(rtc->dev.parent,
> - "setting system clock to "
> - "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
> - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> - tm.tm_hour, tm.tm_min, tm.tm_sec,
> - (unsigned int) tv.tv_sec);
> - }
> - else
> - dev_err(rtc->dev.parent,
> - "hctosys: invalid date/time\n");
> - }
> - else
> - dev_err(rtc->dev.parent,
> - "hctosys: unable to read the hardware clock\n");
> + do_settimeofday(&tv);
>
> + dev_info(rtc->dev.parent,
> + "setting system clock to "
> + "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> + tm.tm_hour, tm.tm_min, tm.tm_sec,
> + (unsigned int) tv.tv_sec);
> +
> +err_invalid:
> +err_read:
> rtc_class_close(rtc);
>
> - return 0;
> +err_open:
> + rtc_hctosys_ret = err;
> +
> + return err;
> }
>
> late_initcall(rtc_hctosys);
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 60f88a7..14dbc83 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -238,6 +238,12 @@ static inline bool is_leap_year(unsigned int year)
> return (!(year % 4) && (year % 100)) || !(year % 400);
> }
>
> +#ifdef CONFIG_RTC_HCTOSYS
> +extern int rtc_hctosys_ret;
> +#else
> +#define rtc_hctosys_ret -ENODEV
> +#endif
> +
> #endif /* __KERNEL__ */
>
> #endif /* _LINUX_RTC_H_ */
oops, I missed a hunk here, namely:

diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index 7dd23a6..380083c 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -107,8 +107,9 @@ rtc_sysfs_show_hctosys(struct device *dev, struct device_attribute *attr,
char *buf)
{
#ifdef CONFIG_RTC_HCTOSYS_DEVICE
- if (strcmp(dev_name(&to_rtc_device(dev)->dev),
- CONFIG_RTC_HCTOSYS_DEVICE) == 0)
+ if (rtc_hctosys_ret == 0 &&
+ strcmp(dev_name(&to_rtc_device(dev)->dev),
+ CONFIG_RTC_HCTOSYS_DEVICE) == 0)
return sprintf(buf, "1\n");
else
#endif

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

2010-02-11 10:52:53

by Uwe Kleine-König

[permalink] [raw]
Subject: [PATCH RESEND] rtc/hctosys: only claim the RTC provided the system time if it did

Moreover change error handling in rtc_hctosys() to use goto and so
reduce the indention level.

Signed-off-by: Uwe Kleine-König <[email protected]>
---
Hello,

this time the patch is complete.

What do you think about it?

Best regards
Uwe

drivers/rtc/hctosys.c | 59 +++++++++++++++++++++++++++--------------------
drivers/rtc/rtc-sysfs.c | 5 ++-
include/linux/rtc.h | 6 ++++
3 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
index 33c0e98..bc90b09 100644
--- a/drivers/rtc/hctosys.c
+++ b/drivers/rtc/hctosys.c
@@ -22,48 +22,57 @@
* the best guess is to add 0.5s.
*/

+int rtc_hctosys_ret = -ENODEV;
+
static int __init rtc_hctosys(void)
{
- int err;
+ int err = -ENODEV;
struct rtc_time tm;
+ struct timespec tv = {
+ .tv_nsec = NSEC_PER_SEC >> 1,
+ };
struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);

if (rtc == NULL) {
- printk("%s: unable to open rtc device (%s)\n",
+ pr_err("%s: unable to open rtc device (%s)\n",
__FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
- return -ENODEV;
+ goto err_open;
}

err = rtc_read_time(rtc, &tm);
- if (err == 0) {
- err = rtc_valid_tm(&tm);
- if (err == 0) {
- struct timespec tv;
+ if (err) {
+ dev_err(rtc->dev.parent,
+ "hctosys: unable to read the hardware clock\n");
+ goto err_read;

- tv.tv_nsec = NSEC_PER_SEC >> 1;
+ }

- rtc_tm_to_time(&tm, &tv.tv_sec);
+ err = rtc_valid_tm(&tm);
+ if (err) {
+ dev_err(rtc->dev.parent,
+ "hctosys: invalid date/time\n");
+ goto err_invalid;
+ }

- do_settimeofday(&tv);
+ rtc_tm_to_time(&tm, &tv.tv_sec);

- dev_info(rtc->dev.parent,
- "setting system clock to "
- "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
- tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
- tm.tm_hour, tm.tm_min, tm.tm_sec,
- (unsigned int) tv.tv_sec);
- }
- else
- dev_err(rtc->dev.parent,
- "hctosys: invalid date/time\n");
- }
- else
- dev_err(rtc->dev.parent,
- "hctosys: unable to read the hardware clock\n");
+ do_settimeofday(&tv);

+ dev_info(rtc->dev.parent,
+ "setting system clock to "
+ "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
+ tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+ tm.tm_hour, tm.tm_min, tm.tm_sec,
+ (unsigned int) tv.tv_sec);
+
+err_invalid:
+err_read:
rtc_class_close(rtc);

- return 0;
+err_open:
+ rtc_hctosys_ret = err;
+
+ return err;
}

late_initcall(rtc_hctosys);
diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
index 7dd23a6..380083c 100644
--- a/drivers/rtc/rtc-sysfs.c
+++ b/drivers/rtc/rtc-sysfs.c
@@ -107,8 +107,9 @@ rtc_sysfs_show_hctosys(struct device *dev, struct device_attribute *attr,
char *buf)
{
#ifdef CONFIG_RTC_HCTOSYS_DEVICE
- if (strcmp(dev_name(&to_rtc_device(dev)->dev),
- CONFIG_RTC_HCTOSYS_DEVICE) == 0)
+ if (rtc_hctosys_ret == 0 &&
+ strcmp(dev_name(&to_rtc_device(dev)->dev),
+ CONFIG_RTC_HCTOSYS_DEVICE) == 0)
return sprintf(buf, "1\n");
else
#endif
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 60f88a7..14dbc83 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -238,6 +238,12 @@ static inline bool is_leap_year(unsigned int year)
return (!(year % 4) && (year % 100)) || !(year % 400);
}

+#ifdef CONFIG_RTC_HCTOSYS
+extern int rtc_hctosys_ret;
+#else
+#define rtc_hctosys_ret -ENODEV
+#endif
+
#endif /* __KERNEL__ */

#endif /* _LINUX_RTC_H_ */
--
1.6.6

2010-02-11 10:57:13

by Alessandro Zummo

[permalink] [raw]
Subject: Re: [rtc-linux] [PATCH RESEND] rtc/hctosys: only claim the RTC provided the system time if it did

On Thu, 11 Feb 2010 11:52:29 +0100
Uwe Kleine-K?nig <[email protected]> wrote:


> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> Hello,
>
> this time the patch is complete.
>
> What do you think about it?

looks nice but I'll have to give a deeper look.
will probably queue for the next merge window


>
> Best regards
> Uwe
>
> drivers/rtc/hctosys.c | 59 +++++++++++++++++++++++++++--------------------
> drivers/rtc/rtc-sysfs.c | 5 ++-
> include/linux/rtc.h | 6 ++++
> 3 files changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
> index 33c0e98..bc90b09 100644
> --- a/drivers/rtc/hctosys.c
> +++ b/drivers/rtc/hctosys.c
> @@ -22,48 +22,57 @@
> * the best guess is to add 0.5s.
> */
>
> +int rtc_hctosys_ret = -ENODEV;
> +
> static int __init rtc_hctosys(void)
> {
> - int err;
> + int err = -ENODEV;
> struct rtc_time tm;
> + struct timespec tv = {
> + .tv_nsec = NSEC_PER_SEC >> 1,
> + };
> struct rtc_device *rtc = rtc_class_open(CONFIG_RTC_HCTOSYS_DEVICE);
>
> if (rtc == NULL) {
> - printk("%s: unable to open rtc device (%s)\n",
> + pr_err("%s: unable to open rtc device (%s)\n",
> __FILE__, CONFIG_RTC_HCTOSYS_DEVICE);
> - return -ENODEV;
> + goto err_open;
> }
>
> err = rtc_read_time(rtc, &tm);
> - if (err == 0) {
> - err = rtc_valid_tm(&tm);
> - if (err == 0) {
> - struct timespec tv;
> + if (err) {
> + dev_err(rtc->dev.parent,
> + "hctosys: unable to read the hardware clock\n");
> + goto err_read;
>
> - tv.tv_nsec = NSEC_PER_SEC >> 1;
> + }
>
> - rtc_tm_to_time(&tm, &tv.tv_sec);
> + err = rtc_valid_tm(&tm);
> + if (err) {
> + dev_err(rtc->dev.parent,
> + "hctosys: invalid date/time\n");
> + goto err_invalid;
> + }
>
> - do_settimeofday(&tv);
> + rtc_tm_to_time(&tm, &tv.tv_sec);
>
> - dev_info(rtc->dev.parent,
> - "setting system clock to "
> - "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
> - tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> - tm.tm_hour, tm.tm_min, tm.tm_sec,
> - (unsigned int) tv.tv_sec);
> - }
> - else
> - dev_err(rtc->dev.parent,
> - "hctosys: invalid date/time\n");
> - }
> - else
> - dev_err(rtc->dev.parent,
> - "hctosys: unable to read the hardware clock\n");
> + do_settimeofday(&tv);
>
> + dev_info(rtc->dev.parent,
> + "setting system clock to "
> + "%d-%02d-%02d %02d:%02d:%02d UTC (%u)\n",
> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> + tm.tm_hour, tm.tm_min, tm.tm_sec,
> + (unsigned int) tv.tv_sec);
> +
> +err_invalid:
> +err_read:
> rtc_class_close(rtc);
>
> - return 0;
> +err_open:
> + rtc_hctosys_ret = err;
> +
> + return err;
> }
>
> late_initcall(rtc_hctosys);
> diff --git a/drivers/rtc/rtc-sysfs.c b/drivers/rtc/rtc-sysfs.c
> index 7dd23a6..380083c 100644
> --- a/drivers/rtc/rtc-sysfs.c
> +++ b/drivers/rtc/rtc-sysfs.c
> @@ -107,8 +107,9 @@ rtc_sysfs_show_hctosys(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> #ifdef CONFIG_RTC_HCTOSYS_DEVICE
> - if (strcmp(dev_name(&to_rtc_device(dev)->dev),
> - CONFIG_RTC_HCTOSYS_DEVICE) == 0)
> + if (rtc_hctosys_ret == 0 &&
> + strcmp(dev_name(&to_rtc_device(dev)->dev),
> + CONFIG_RTC_HCTOSYS_DEVICE) == 0)
> return sprintf(buf, "1\n");
> else
> #endif
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 60f88a7..14dbc83 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -238,6 +238,12 @@ static inline bool is_leap_year(unsigned int year)
> return (!(year % 4) && (year % 100)) || !(year % 400);
> }
>
> +#ifdef CONFIG_RTC_HCTOSYS
> +extern int rtc_hctosys_ret;
> +#else
> +#define rtc_hctosys_ret -ENODEV
> +#endif
> +
> #endif /* __KERNEL__ */
>
> #endif /* _LINUX_RTC_H_ */
> --
> 1.6.6
>
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.


--

Best regards,

Alessandro Zummo,
Tower Technologies - Torino, Italy

http://www.towertech.it

2010-02-11 22:42:59

by Andrew Morton

[permalink] [raw]
Subject: Re: [rtc-linux] [PATCH RESEND] rtc/hctosys: only claim the RTC provided the system time if it did

On Thu, 11 Feb 2010 11:57:00 +0100
Alessandro Zummo <[email protected]> wrote:

> On Thu, 11 Feb 2010 11:52:29 +0100
> Uwe Kleine-K__nig <[email protected]> wrote:
>
>
> > Signed-off-by: Uwe Kleine-K__nig <[email protected]>
> > ---
> > Hello,
> >
> > this time the patch is complete.
> >
> > What do you think about it?

I do dislike having to trawl through the patch trying to work out what
the changelog forgot to tell me.

What problem is being fixed here?

What is the user-observable manifestation of that problem?

How does the patch fix it?

Stuff like that!

> looks nice but I'll have to give a deeper look.
> will probably queue for the next merge window

I merged it with a "needs Alessandro ack" note-to-self.

2010-02-12 08:52:45

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [rtc-linux] [PATCH RESEND] rtc/hctosys: only claim the RTC provided the system time if it did

Hello Andrew,

On Thu, Feb 11, 2010 at 02:42:40PM -0800, Andrew Morton wrote:
> On Thu, 11 Feb 2010 11:57:00 +0100
> Alessandro Zummo <[email protected]> wrote:
>
> > On Thu, 11 Feb 2010 11:52:29 +0100
> > Uwe Kleine-K__nig <[email protected]> wrote:
> >
> >
> > > Signed-off-by: Uwe Kleine-K__nig <[email protected]>
> > > ---
> > > Hello,
> > >
> > > this time the patch is complete.
> > >
> > > What do you think about it?
>
> I do dislike having to trawl through the patch trying to work out what
> the changelog forgot to tell me.
>
> What problem is being fixed here?
>
> What is the user-observable manifestation of that problem?
>
> How does the patch fix it?
>
> Stuff like that!
What do you think about:

---->8----
rtc/hctosys: only claim the RTC provided the system time if it did

Without this patch /sys/class/rtc/$CONFIG_RTC_HCTOSYS_DEVICE/hctosys
contains a 1 (meaning "This rtc was used to initialize the system
clock") even if reading the time at bootup failed.

Moreover change error handling in rtc_hctosys() to use goto and so
reduce the indention level.
---->8----

> > looks nice but I'll have to give a deeper look.
> > will probably queue for the next merge window
>
> I merged it with a "needs Alessandro ack" note-to-self.
fine, thanks
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |