| Message ID | 1427576976-22353-2-git-send-email-aaro.koskinen@iki.fi |
|---|---|
| State | Accepted |
| Headers | show |
On 28/03/2015 at 23:09:35 +0200, Aaro Koskinen wrote : > __rtc_read_time logs should be debug logs instead of error logs. > > For example, when the RTC clock is not set, it's not really useful > to print a kernel error log every time someone tries to read the clock: > > ~ # hwclock -r > [ 604.508263] rtc rtc0: read_time: fail to read > hwclock: RTC_RD_TIME: Invalid argument > > If there's a real error, it's likely that lower level or higher level > code will tell it anyway. Make these logs debug logs, and also print > the error code for the read failure. > That actually may be the only error message printed for some failures. Some RTCs don't print anything in case of error in their .read_time() and there are in-kernel users of rtc_read_time that simply bail out without printing anything or have a trace that is already at the debug level. I would agree that this would need a better harmonization and I guess we can do that for now. I'll try to fix the in-kernel cases. > Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi> Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > drivers/rtc/interface.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c > index 37215cf..c786818 100644 > --- a/drivers/rtc/interface.c > +++ b/drivers/rtc/interface.c > @@ -31,13 +31,14 @@ static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) > 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"); > + dev_dbg(&rtc->dev, "read_time: fail to read: %d\n", > + err); > return err; > } > > err = rtc_valid_tm(tm); > if (err < 0) > - dev_err(&rtc->dev, "read_time: rtc_time isn't valid\n"); > + dev_dbg(&rtc->dev, "read_time: rtc_time isn't valid\n"); > } > return err; > } > -- > 2.2.0 >
On Wed, 2015-04-01 at 05:21 +0200, Alexandre Belloni wrote: > On 28/03/2015 at 23:09:35 +0200, Aaro Koskinen wrote : > > __rtc_read_time logs should be debug logs instead of error logs. > > > > For example, when the RTC clock is not set, it's not really useful > > to print a kernel error log every time someone tries to read the clock: > > > > ~ # hwclock -r > > [ 604.508263] rtc rtc0: read_time: fail to read > > hwclock: RTC_RD_TIME: Invalid argument > > > > If there's a real error, it's likely that lower level or higher level > > code will tell it anyway. Make these logs debug logs, and also print > > the error code for the read failure. > > > > That actually may be the only error message printed for some failures. > Some RTCs don't print anything in case of error in their .read_time() > and there are in-kernel users of rtc_read_time that simply bail out > without printing anything or have a trace that is already at the debug > level. > > I would agree that this would need a better harmonization and I guess we > can do that for now. I'll try to fix the in-kernel cases. Maybe these should use dev_err_once(). > > diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c [] > > @@ -31,13 +31,14 @@ static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) > > 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"); > > + dev_dbg(&rtc->dev, "read_time: fail to read: %d\n", > > + err); > > return err; > > } > > > > err = rtc_valid_tm(tm); > > if (err < 0) > > - dev_err(&rtc->dev, "read_time: rtc_time isn't valid\n"); > > + dev_dbg(&rtc->dev, "read_time: rtc_time isn't valid\n"); > > } > > return err; > > }
On 31/03/2015 at 20:25:19 -0700, Joe Perches wrote : > On Wed, 2015-04-01 at 05:21 +0200, Alexandre Belloni wrote: > > On 28/03/2015 at 23:09:35 +0200, Aaro Koskinen wrote : > > > __rtc_read_time logs should be debug logs instead of error logs. > > > > > > For example, when the RTC clock is not set, it's not really useful > > > to print a kernel error log every time someone tries to read the clock: > > > > > > ~ # hwclock -r > > > [ 604.508263] rtc rtc0: read_time: fail to read > > > hwclock: RTC_RD_TIME: Invalid argument > > > > > > If there's a real error, it's likely that lower level or higher level > > > code will tell it anyway. Make these logs debug logs, and also print > > > the error code for the read failure. > > > > > > > That actually may be the only error message printed for some failures. > > Some RTCs don't print anything in case of error in their .read_time() > > and there are in-kernel users of rtc_read_time that simply bail out > > without printing anything or have a trace that is already at the debug > > level. > > > > I would agree that this would need a better harmonization and I guess we > > can do that for now. I'll try to fix the in-kernel cases. > > Maybe these should use dev_err_once(). > As the issue may be sporadic, I would say that the reasoning behind the patch is correct. Userspace is already aware that something went wrong when reading and those debug messages are not adding any information.
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c index 37215cf..c786818 100644 --- a/drivers/rtc/interface.c +++ b/drivers/rtc/interface.c @@ -31,13 +31,14 @@ static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm) 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"); + dev_dbg(&rtc->dev, "read_time: fail to read: %d\n", + err); return err; } err = rtc_valid_tm(tm); if (err < 0) - dev_err(&rtc->dev, "read_time: rtc_time isn't valid\n"); + dev_dbg(&rtc->dev, "read_time: rtc_time isn't valid\n"); } return err; }
__rtc_read_time logs should be debug logs instead of error logs. For example, when the RTC clock is not set, it's not really useful to print a kernel error log every time someone tries to read the clock: ~ # hwclock -r [ 604.508263] rtc rtc0: read_time: fail to read hwclock: RTC_RD_TIME: Invalid argument If there's a real error, it's likely that lower level or higher level code will tell it anyway. Make these logs debug logs, and also print the error code for the read failure. Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi> --- drivers/rtc/interface.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)