Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Thomas Gleixner]

On Tue, 21 Mar 2017, Arnd Bergmann wrote:

> On Mon, Mar 20, 2017 at 9:40 PM, Thomas Gleixner  wrote:
> > On Mon, 20 Mar 2017, Deepa Dinamani wrote:
> >> >> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec 
> >> >> *tp)
> >> >> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 
> >> >> *tp)
> >> >
> >> > That's a pretty pointless exercise. getres() returns the resolution of 
> >> > the
> >> > clock which obviously can never be affected by Y2038.
> >>
> >> True, tv_sec does not need to be more than 32 bits here.
> >> We plan to limit the use of struct timespec to existing user interfaces 
> >> only.
> >
> > This is an existing user space interface and there is no need to change it
> > at all.
> 
> I think we should change it in the kernel, otherwise every libc implementation
> has to include a copy of this, to convert between the user space 16-byte
> timespec and the 8-byte kernel timespec. If we do it in the kernel, we only
> need one copy and the interface is consistent between 32-bit and 64-bit
> user space.

Fair enough. Please add a comment which explains why this uses a timespec64
as it is not obvious - as demonstrated :)

Thanks,

tglx
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 9:40 PM, Thomas Gleixner  wrote:
> On Mon, 20 Mar 2017, Deepa Dinamani wrote:
>> >> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
>> >> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 
>> >> *tp)
>> >
>> > That's a pretty pointless exercise. getres() returns the resolution of the
>> > clock which obviously can never be affected by Y2038.
>>
>> True, tv_sec does not need to be more than 32 bits here.
>> We plan to limit the use of struct timespec to existing user interfaces only.
>
> This is an existing user space interface and there is no need to change it
> at all.

I think we should change it in the kernel, otherwise every libc implementation
has to include a copy of this, to convert between the user space 16-byte
timespec and the 8-byte kernel timespec. If we do it in the kernel, we only
need one copy and the interface is consistent between 32-bit and 64-bit
user space.

The other point that Deepa made is important for verification purposes:
If we can eliminate timespec/time_t/timeval from all in-kernel code, we have
a much better chance at showing that we have no y2038 problems.
I did a private patch series to try this out a while ago, and managed to
actually get the kernel to build fine with no 32-bit time_t code left in
it (lots of minor drivers being marked in Kconfig as depending on old time_t)

   Arnd
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Tomoyoshi ASANO]

On Sun, 19 Mar 2017 11:51:34 +0100 (CET)
Thomas Gleixner  wrote:

> On Sat, 18 Mar 2017, Deepa Dinamani wrote:
> 
> >  struct timespec is not y2038 safe.
> >  Replace the posix_clock ops interfaces to use
> >  struct timespec64.
> >  The patch also changes struct itimerspec interfaces to
> >  struct itimerspec64 as itimerspec internally uses timespec
> >  and itimerspec64 uses timespec64.
> >  PTP clocks is the only module that sets up these interfaces.
> >  All individual drivers rely on PTP class driver for exposure
> >  to userspace. Hence, the change also deals with fixing up these
> >  PTP interfaces.
> >  The patch also changes dynamic posix clock implementation to
> >  reflect the changes in the functional clock interface.
> 
> Please do not explain WHAT the patch is doing. We can see that from the
> diff itself. What's important is the WHY. A good changelog is structured in
> paragraphs, which explain the context, the problem and the solution. Please
> read Documentation/process/submitting-patches.rst. Let me give you an
> example.
> 
>   struct timespec is not Y2038 safe on 32 bit machines and needs to be
>   replaced with struct timespec64.
> 
>   The posix clock functions use struct timespec directly and through struct
>   itimerspec.
> 
>   Change all function prototypes to use timespec64 and itimerspec64 and fix
>   up all implementations.
> 
> That gives all the information a reviewer or someone who is looking at the
> commit later needs: context, problem scope and solution.
> 
> Hmm?
> 
> >  /* posix clock implementation */
> >  
> > -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
> > +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)
> 
> That's a pretty pointless exercise. getres() returns the resolution of the
> clock which obviously can never be affected by Y2038. 
> 
> >  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
> >  {
> > struct posix_clock_desc cd;
> > +   struct timespec64 ts64 = timespec_to_timespec64(*ts);
> > int err;
> 
> Please order the variables as a reverse fir tree sorted by length.
> 
>   struct timespec64 ts64 = timespec_to_timespec64(*ts);
>   struct posix_clock_desc cd;
>   int err;
> 
> That's way simpler to parse than the above random length odering.
> 
> > @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int 
> > flags,
> >  {
> > clockid_t id = kit->it_clock;
> > struct posix_clock_desc cd;
> > +   struct itimerspec64 old64;
> > +   struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);
> 
> See above.
> 
> Thanks,
> 
>   tglx
> ___
> Y2038 mailing list
> Y2038@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/y2038

--
 　┏┓
 ┏┫  リネオは お客様の これりな を後押し   ┣┓
 ┃┗┳━━┳┛┃
 ┗━┛ http://www.lineo.co.jp ┗━┛


___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Deepa Dinamani]

> Please do not explain WHAT the patch is doing. We can see that from the
> diff itself. What's important is the WHY. A good changelog is structured in
> paragraphs, which explain the context, the problem and the solution. Please
> read Documentation/process/submitting-patches.rst. Let me give you an
> example.
>
>   struct timespec is not Y2038 safe on 32 bit machines and needs to be
>   replaced with struct timespec64.
>
>   The posix clock functions use struct timespec directly and through struct
>   itimerspec.
>
>   Change all function prototypes to use timespec64 and itimerspec64 and fix
>   up all implementations.
>
> That gives all the information a reviewer or someone who is looking at the
> commit later needs: context, problem scope and solution.
>
> Hmm?

Thanks for the guidance. Will fix the changelog along these lines.

>>  /* posix clock implementation */
>>
>> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
>> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)
>
> That's a pretty pointless exercise. getres() returns the resolution of the
> clock which obviously can never be affected by Y2038.

True, tv_sec does not need to be more than 32 bits here.
We plan to limit the use of struct timespec to existing user interfaces only.
This is the reason for the change.

>>  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
>>  {
>>   struct posix_clock_desc cd;
>> + struct timespec64 ts64 = timespec_to_timespec64(*ts);
>>   int err;
>
> Please order the variables as a reverse fir tree sorted by length.

Will take care of these orderings.

> struct timespec64 ts64 = timespec_to_timespec64(*ts);
> struct posix_clock_desc cd;
> int err;
>
> That's way simpler to parse than the above random length odering.
>
>> @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int 
>> flags,
>>  {
>>   clockid_t id = kit->it_clock;
>>   struct posix_clock_desc cd;
>> + struct itimerspec64 old64;
>> + struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);

Thanks,
Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Deepa Dinamani]

> When changing the PTP code, please put the PTP maintainer onto CC.

Will do. Thanks for pointing out the omission.

-Deepa
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Richard Cochran]

On Sat, Mar 18, 2017 at 10:17:41PM -0700, Deepa Dinamani wrote:
>  struct timespec is not y2038 safe.
>  Replace the posix_clock ops interfaces to use
>  struct timespec64.
>  The patch also changes struct itimerspec interfaces to
>  struct itimerspec64 as itimerspec internally uses timespec
>  and itimerspec64 uses timespec64.
>  PTP clocks is the only module that sets up these interfaces.
>  All individual drivers rely on PTP class driver for exposure
>  to userspace. Hence, the change also deals with fixing up these
>  PTP interfaces.
>  The patch also changes dynamic posix clock implementation to
>  reflect the changes in the functional clock interface.
> 
> Signed-off-by: Deepa Dinamani 
> ---
>  drivers/ptp/ptp_clock.c | 18 +++---

When changing the PTP code, please put the PTP maintainer onto CC.

Thanks,
Richard
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038

Re: [Y2038] [RESEND PATCH 2/7] time: Change posix clocks ops interfaces to use timespec64

[Thomas Gleixner]

On Sat, 18 Mar 2017, Deepa Dinamani wrote:

>  struct timespec is not y2038 safe.
>  Replace the posix_clock ops interfaces to use
>  struct timespec64.
>  The patch also changes struct itimerspec interfaces to
>  struct itimerspec64 as itimerspec internally uses timespec
>  and itimerspec64 uses timespec64.
>  PTP clocks is the only module that sets up these interfaces.
>  All individual drivers rely on PTP class driver for exposure
>  to userspace. Hence, the change also deals with fixing up these
>  PTP interfaces.
>  The patch also changes dynamic posix clock implementation to
>  reflect the changes in the functional clock interface.

Please do not explain WHAT the patch is doing. We can see that from the
diff itself. What's important is the WHY. A good changelog is structured in
paragraphs, which explain the context, the problem and the solution. Please
read Documentation/process/submitting-patches.rst. Let me give you an
example.

  struct timespec is not Y2038 safe on 32 bit machines and needs to be
  replaced with struct timespec64.

  The posix clock functions use struct timespec directly and through struct
  itimerspec.

  Change all function prototypes to use timespec64 and itimerspec64 and fix
  up all implementations.

That gives all the information a reviewer or someone who is looking at the
commit later needs: context, problem scope and solution.

Hmm?

>  /* posix clock implementation */
>  
> -static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp)
> +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp)

That's a pretty pointless exercise. getres() returns the resolution of the
clock which obviously can never be affected by Y2038. 

>  static int pc_clock_settime(clockid_t id, const struct timespec *ts)
>  {
>   struct posix_clock_desc cd;
> + struct timespec64 ts64 = timespec_to_timespec64(*ts);
>   int err;

Please order the variables as a reverse fir tree sorted by length.

struct timespec64 ts64 = timespec_to_timespec64(*ts);
struct posix_clock_desc cd;
int err;

That's way simpler to parse than the above random length odering.

> @@ -418,14 +427,19 @@ static int pc_timer_settime(struct k_itimer *kit, int 
> flags,
>  {
>   clockid_t id = kit->it_clock;
>   struct posix_clock_desc cd;
> + struct itimerspec64 old64;
> + struct itimerspec64 ts64 = itimerspec_to_itimerspec64(ts);

See above.

Thanks,

tglx
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038