[PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Nick Crews]

The tm_yday and tm_wday fields are not used by userspace,
so since they aren't needed within the driver, don't
bother calculating them. This is especially needed since
the rtc_year_days() call was crashing if the HW returned
an invalid time.

Signed-off-by: Nick Crews <ncrews@chromium.org>
---
 drivers/rtc/rtc-wilco-ec.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
index 8ad4c4e6d557..e84faa268caf 100644
--- a/drivers/rtc/rtc-wilco-ec.c
+++ b/drivers/rtc/rtc-wilco-ec.c
@@ -110,10 +110,6 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
 	tm->tm_mday	= rtc.day;
 	tm->tm_mon	= rtc.month - 1;
 	tm->tm_year	= rtc.year + (rtc.century * 100) - 1900;
-	tm->tm_yday	= rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
-
-	/* Don't compute day of week, we don't need it. */
-	tm->tm_wday = -1;
 
 	return 0;
 }
-- 
2.21.0

[PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Nick Crews]

Signed-off-by: Nick Crews <ncrews@chromium.org>
---
 drivers/rtc/rtc-wilco-ec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
index e84faa268caf..951268f5e690 100644
--- a/drivers/rtc/rtc-wilco-ec.c
+++ b/drivers/rtc/rtc-wilco-ec.c
@@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
 
 MODULE_ALIAS("platform:rtc-wilco-ec");
 MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
-MODULE_LICENSE("GPL v2");
+MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Wilco EC RTC driver");
-- 
2.21.0

Re: [PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Pavel Machek]

On Mon 2019-09-16 12:12:15, Nick Crews wrote:
> The tm_yday and tm_wday fields are not used by userspace,
> so since they aren't needed within the driver, don't
> bother calculating them. This is especially needed since
> the rtc_year_days() call was crashing if the HW returned
> an invalid time.
> 
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  drivers/rtc/rtc-wilco-ec.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> index 8ad4c4e6d557..e84faa268caf 100644
> --- a/drivers/rtc/rtc-wilco-ec.c
> +++ b/drivers/rtc/rtc-wilco-ec.c
> @@ -110,10 +110,6 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
>  	tm->tm_mday	= rtc.day;
>  	tm->tm_mon	= rtc.month - 1;
>  	tm->tm_year	= rtc.year + (rtc.century * 100) - 1900;
> -	tm->tm_yday	= rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> -
> -	/* Don't compute day of week, we don't need it. */
> -	tm->tm_wday = -1;
>  
>  	return 0;

Are you sure? It would be bad to pass unititialized memory to userspace...

If userspace does not need those fields, why are they there?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Alexandre Belloni]

On 22/09/2019 18:13:06+0200, Pavel Machek wrote:
> On Mon 2019-09-16 12:12:15, Nick Crews wrote:
> > The tm_yday and tm_wday fields are not used by userspace,
> > so since they aren't needed within the driver, don't
> > bother calculating them. This is especially needed since
> > the rtc_year_days() call was crashing if the HW returned
> > an invalid time.
> > 
> > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > ---
> >  drivers/rtc/rtc-wilco-ec.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > index 8ad4c4e6d557..e84faa268caf 100644
> > --- a/drivers/rtc/rtc-wilco-ec.c
> > +++ b/drivers/rtc/rtc-wilco-ec.c
> > @@ -110,10 +110,6 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
> >  	tm->tm_mday	= rtc.day;
> >  	tm->tm_mon	= rtc.month - 1;
> >  	tm->tm_year	= rtc.year + (rtc.century * 100) - 1900;
> > -	tm->tm_yday	= rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> > -
> > -	/* Don't compute day of week, we don't need it. */
> > -	tm->tm_wday = -1;
> >  
> >  	return 0;
> 
> Are you sure? It would be bad to pass unititialized memory to userspace...
> 

This problem doesn't exist because userspace is passing the memory, not
the other way around.

> If userspace does not need those fields, why are they there?
> 

This is coming from struct tm, it is part of C89 but I think I was not
born when this decision was made. man rtc properly reports that those
fields are unused and no userspace tools are actually making use of
them. Nobody cares about the broken down representation of the time.
What is done is use the ioctl then mktime to have a UNIX timestamp.

"The mktime function ignores the specified contents of the tm_wday,
tm_yday, tm_gmtoff, and tm_zone members of the broken-down time
structure. It uses the values of the other components to determine the
calendar time; it’s permissible for these components to have
unnormalized values outside their normal ranges. The last thing that
mktime does is adjust the components of the brokentime structure,
including the members that were initially ignored."


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Pavel Machek]

On Mon 2019-09-16 12:12:17, Nick Crews wrote:
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  drivers/rtc/rtc-wilco-ec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> index e84faa268caf..951268f5e690 100644
> --- a/drivers/rtc/rtc-wilco-ec.c
> +++ b/drivers/rtc/rtc-wilco-ec.c
> @@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
>  
>  MODULE_ALIAS("platform:rtc-wilco-ec");
>  MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> -MODULE_LICENSE("GPL v2");
> +MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Wilco EC RTC driver");

File spdx header says GPL-2.0, this change would make it inconsistent with that...

Best regards,
											Pavel

Re: [PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Alexandre Belloni]

On 22/09/2019 22:29:48+0200, Pavel Machek wrote:
> On Mon 2019-09-16 12:12:17, Nick Crews wrote:
> > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > ---
> >  drivers/rtc/rtc-wilco-ec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > index e84faa268caf..951268f5e690 100644
> > --- a/drivers/rtc/rtc-wilco-ec.c
> > +++ b/drivers/rtc/rtc-wilco-ec.c
> > @@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
> >  
> >  MODULE_ALIAS("platform:rtc-wilco-ec");
> >  MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> > -MODULE_LICENSE("GPL v2");
> > +MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("Wilco EC RTC driver");
> 
> File spdx header says GPL-2.0, this change would make it inconsistent with that...
> 

Commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2"
bogosity") doesn't agree with you (but I was surprised too).


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Nick Crews]

Would you like me to change this patch at all? Perhaps reference
bf7fbeeae6db in the commit description? I probably should have
done that since the beginning, I just couldn't find bf7fbeeae6db at first.

Thanks,
Nick

On Sun, Sep 22, 2019 at 2:43 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 22/09/2019 22:29:48+0200, Pavel Machek wrote:
> > On Mon 2019-09-16 12:12:17, Nick Crews wrote:
> > > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > > ---
> > >  drivers/rtc/rtc-wilco-ec.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > > index e84faa268caf..951268f5e690 100644
> > > --- a/drivers/rtc/rtc-wilco-ec.c
> > > +++ b/drivers/rtc/rtc-wilco-ec.c
> > > @@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
> > >
> > >  MODULE_ALIAS("platform:rtc-wilco-ec");
> > >  MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> > > -MODULE_LICENSE("GPL v2");
> > > +MODULE_LICENSE("GPL");
> > >  MODULE_DESCRIPTION("Wilco EC RTC driver");
> >
> > File spdx header says GPL-2.0, this change would make it inconsistent with that...
> >
>
> Commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2"
> bogosity") doesn't agree with you (but I was surprised too).
>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Nick Crews]

On Sun, Sep 22, 2019 at 1:05 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 22/09/2019 18:13:06+0200, Pavel Machek wrote:
> > On Mon 2019-09-16 12:12:15, Nick Crews wrote:
> > > The tm_yday and tm_wday fields are not used by userspace,
> > > so since they aren't needed within the driver, don't
> > > bother calculating them. This is especially needed since
> > > the rtc_year_days() call was crashing if the HW returned
> > > an invalid time.
> > >
> > > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > > ---
> > >  drivers/rtc/rtc-wilco-ec.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > >
> > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > > index 8ad4c4e6d557..e84faa268caf 100644
> > > --- a/drivers/rtc/rtc-wilco-ec.c
> > > +++ b/drivers/rtc/rtc-wilco-ec.c
> > > @@ -110,10 +110,6 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
> > >     tm->tm_mday     = rtc.day;
> > >     tm->tm_mon      = rtc.month - 1;
> > >     tm->tm_year     = rtc.year + (rtc.century * 100) - 1900;
> > > -   tm->tm_yday     = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> > > -
> > > -   /* Don't compute day of week, we don't need it. */
> > > -   tm->tm_wday = -1;
> > >
> > >     return 0;
> >
> > Are you sure? It would be bad to pass unititialized memory to userspace...
> >
>
> This problem doesn't exist because userspace is passing the memory, not
> the other way around.
>
> > If userspace does not need those fields, why are they there?
> >
>
> This is coming from struct tm, it is part of C89 but I think I was not
> born when this decision was made. man rtc properly reports that those
> fields are unused and no userspace tools are actually making use of
> them. Nobody cares about the broken down representation of the time.
> What is done is use the ioctl then mktime to have a UNIX timestamp.
>
> "The mktime function ignores the specified contents of the tm_wday,
> tm_yday, tm_gmtoff, and tm_zone members of the broken-down time
> structure. It uses the values of the other components to determine the
> calendar time; it’s permissible for these components to have
> unnormalized values outside their normal ranges. The last thing that
> mktime does is adjust the components of the brokentime structure,
> including the members that were initially ignored."

This is very non-obvious and I only knew this from talking to you,
Alexandre. Perhaps we should add this note to the RTC core,
such as in the description for rtc_class_ops?

For this patch, do you want me to make any further changes?

Thanks,
Nick

>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Alexandre Belloni]

On 23/09/2019 11:20:42-0600, Nick Crews wrote:
> > This is coming from struct tm, it is part of C89 but I think I was not
> > born when this decision was made. man rtc properly reports that those
> > fields are unused and no userspace tools are actually making use of
> > them. Nobody cares about the broken down representation of the time.
> > What is done is use the ioctl then mktime to have a UNIX timestamp.
> >
> > "The mktime function ignores the specified contents of the tm_wday,
> > tm_yday, tm_gmtoff, and tm_zone members of the broken-down time
> > structure. It uses the values of the other components to determine the
> > calendar time; it’s permissible for these components to have
> > unnormalized values outside their normal ranges. The last thing that
> > mktime does is adjust the components of the brokentime structure,
> > including the members that were initially ignored."
> 
> This is very non-obvious and I only knew this from talking to you,
> Alexandre. Perhaps we should add this note to the RTC core,
> such as in the description for rtc_class_ops?
> 

I'm planning to add documentation on what should be done in an RTC
driver, I'll ensure to add something on this topic.

> For this patch, do you want me to make any further changes?
> 

No need for any changes, however, I can't apply it right now because we
are in the middle of the merge window.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Sun 2019-09-22 22:43:53, Alexandre Belloni wrote:
> On 22/09/2019 22:29:48+0200, Pavel Machek wrote:
> > On Mon 2019-09-16 12:12:17, Nick Crews wrote:
> > > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > > ---
> > >  drivers/rtc/rtc-wilco-ec.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > > index e84faa268caf..951268f5e690 100644
> > > --- a/drivers/rtc/rtc-wilco-ec.c
> > > +++ b/drivers/rtc/rtc-wilco-ec.c
> > > @@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
> > >  
> > >  MODULE_ALIAS("platform:rtc-wilco-ec");
> > >  MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> > > -MODULE_LICENSE("GPL v2");
> > > +MODULE_LICENSE("GPL");
> > >  MODULE_DESCRIPTION("Wilco EC RTC driver");
> > 
> > File spdx header says GPL-2.0, this change would make it inconsistent with that...
> 
> Commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2"
> bogosity") doesn't agree with you (but I was surprised too).

Still don't get it. bf7fbeeae6db makes MODULE_LICENSE less useful, and
declares "GPL" == "GPL v2" in MODULE_LICENSE. So.. this change is no
longer wrong, it is just unneccessary...? Why do it? It is not a fix
as a subject line says...

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v2 2/2] rtc: wilco-ec: Fix license to GPL from GPLv2

[Nick Crews]

On Tue, Sep 24, 2019 at 1:55 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Sun 2019-09-22 22:43:53, Alexandre Belloni wrote:
> > On 22/09/2019 22:29:48+0200, Pavel Machek wrote:
> > > On Mon 2019-09-16 12:12:17, Nick Crews wrote:
> > > > Signed-off-by: Nick Crews <ncrews@chromium.org>
> > > > ---
> > > >  drivers/rtc/rtc-wilco-ec.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> > > > index e84faa268caf..951268f5e690 100644
> > > > --- a/drivers/rtc/rtc-wilco-ec.c
> > > > +++ b/drivers/rtc/rtc-wilco-ec.c
> > > > @@ -184,5 +184,5 @@ module_platform_driver(wilco_ec_rtc_driver);
> > > >
> > > >  MODULE_ALIAS("platform:rtc-wilco-ec");
> > > >  MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> > > > -MODULE_LICENSE("GPL v2");
> > > > +MODULE_LICENSE("GPL");
> > > >  MODULE_DESCRIPTION("Wilco EC RTC driver");
> > >
> > > File spdx header says GPL-2.0, this change would make it inconsistent with that...
> >
> > Commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2"
> > bogosity") doesn't agree with you (but I was surprised too).
>
> Still don't get it. bf7fbeeae6db makes MODULE_LICENSE less useful, and
> declares "GPL" == "GPL v2" in MODULE_LICENSE. So.. this change is no
> longer wrong, it is just unneccessary...? Why do it? It is not a fix
> as a subject line says...

All new modules should have the plain "GPL", or at least that's what I
was told when I submitted a patch adding a "GPL v2" license. Therefore
I assumed that if the distinction was worthwhile there, I should try to make
existing code consistent too. Sounds fine to me to drop this though,
unless anyone else has strong opinions,

>
>                                                                 Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v2 1/2] rtc: wilco-ec: Remove yday and wday calculations

[Nick Crews]

Hi Alexandre,

Sorry to be a pain, but I passed this by some other Chrome OS
kernel engineers, and when the HW gives a bogus time we
want logging at a more severe level than the dev_dbg() call
in the core, so I'm going to send another revision. It's going to
require duplicated calls to rtc_valid_tm(), but we feel that it is
required.

Thanks,
Nick

On Mon, Sep 23, 2019 at 2:19 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 23/09/2019 11:20:42-0600, Nick Crews wrote:
> > > This is coming from struct tm, it is part of C89 but I think I was not
> > > born when this decision was made. man rtc properly reports that those
> > > fields are unused and no userspace tools are actually making use of
> > > them. Nobody cares about the broken down representation of the time.
> > > What is done is use the ioctl then mktime to have a UNIX timestamp.
> > >
> > > "The mktime function ignores the specified contents of the tm_wday,
> > > tm_yday, tm_gmtoff, and tm_zone members of the broken-down time
> > > structure. It uses the values of the other components to determine the
> > > calendar time; it’s permissible for these components to have
> > > unnormalized values outside their normal ranges. The last thing that
> > > mktime does is adjust the components of the brokentime structure,
> > > including the members that were initially ignored."
> >
> > This is very non-obvious and I only knew this from talking to you,
> > Alexandre. Perhaps we should add this note to the RTC core,
> > such as in the description for rtc_class_ops?
> >
>
> I'm planning to add documentation on what should be done in an RTC
> driver, I'll ensure to add something on this topic.
>
> > For this patch, do you want me to make any further changes?
> >
>
> No need for any changes, however, I can't apply it right now because we
> are in the middle of the merge window.
>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com