diff mbox

[3/6] y2038: Introduce struct __timeval64 - new internal glibc type

Message ID 20200118072047.23071-4-lukma@denx.de
State Superseded
Headers

Commit Message

Lukasz Majewski Jan. 18, 2020, 7:20 a.m. UTC 
  This type is a glibc's "internal" type similar to struct timeval but
whose tv_sec field is a __time64_t rather than a time_t, which makes it
Y2038-proof. This struct is NOT supposed to be passed to the kernel -
instead it shall be converted to struct __timespec64 and clock_[sg]ettime
syscalls shall be used (which are now Y2038 safe).

Build tests:
./src/scripts/build-many-glibcs.py glibcs
---
 include/time.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Alistair Francis Jan. 18, 2020, 10:47 p.m. UTC | #1 
On Sat, Jan 18, 2020 at 5:21 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> This type is a glibc's "internal" type similar to struct timeval but
> whose tv_sec field is a __time64_t rather than a time_t, which makes it
> Y2038-proof. This struct is NOT supposed to be passed to the kernel -
> instead it shall be converted to struct __timespec64 and clock_[sg]ettime
> syscalls shall be used (which are now Y2038 safe).
>
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs

Looks good, I have the same thing in my tree :)

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/time.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/time.h b/include/time.h
> index 047f431a1a..22ecacd0d8 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -93,6 +93,20 @@ struct __itimerspec64
>  };
>  #endif
>
> +#if __TIMESIZE == 64
> +# define __timeval64 timeval
> +#else
> +/* The glibc Y2038-proof struct __timeval64 structure for a time value.
> +   This structure is NOT supposed to be passed to the Linux kernel.
> +   Instead, it shall be converted to struct __timespec64 and time shall
> +   be [sg]et via clock_[sg]ettime (which are now Y2038 safe).  */
> +struct __timeval64
> +{
> +  __time64_t tv_sec;         /* Seconds */
> +  __suseconds_t tv_usec;       /* Microseconds */
> +};
> +#endif
> +
>  #if __TIMESIZE == 64
>  # define __ctime64 ctime
>  #else
> --
> 2.20.1
>
Arnd Bergmann Jan. 19, 2020, 11:36 a.m. UTC | #2 
On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Jan 18, 2020 at 5:21 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > This type is a glibc's "internal" type similar to struct timeval but
> > whose tv_sec field is a __time64_t rather than a time_t, which makes it
> > Y2038-proof. This struct is NOT supposed to be passed to the kernel -
> > instead it shall be converted to struct __timespec64 and clock_[sg]ettime
> > syscalls shall be used (which are now Y2038 safe).
> >
> > Build tests:
> > ./src/scripts/build-many-glibcs.py glibcs
>
> Looks good, I have the same thing in my tree :)

I think it's slightly different: __suseconds_t on rv32 is 64 bit wide, while
on all existing 32-bit architectures as well as sparc64 it is 32-bit wide.

While almost all kernel interfaces based on timeval have been replaced
by those based on timespec, there are a small number that need this
to be compatible with the kernel's layout, or to have an explicit conversion:

- Socket timestamps with SO_TIMESTAMP
- Socket timeouts with SO_RCVTIMEO/SO_SNDTIMEO
- pc-style parallel ports with the PPGETTIME/PPSETTIME ioctls
- video4linux with the VIDIOC_QUERYBUF/VIDIOC_QBUF/VIDIOC_DQBUF
  VIDIOC_PREPARE_BUF and VIDIOC_OMAP3ISP_STAT_REQ ioctls

       Arnd
Lukasz Majewski Jan. 19, 2020, 3:22 p.m. UTC | #3 
Hi Arnd,

> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> <alistair23@gmail.com> wrote:
> >
> > On Sat, Jan 18, 2020 at 5:21 PM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > >
> > > This type is a glibc's "internal" type similar to struct timeval
> > > but whose tv_sec field is a __time64_t rather than a time_t,
> > > which makes it Y2038-proof. This struct is NOT supposed to be
> > > passed to the kernel - instead it shall be converted to struct
> > > __timespec64 and clock_[sg]ettime syscalls shall be used (which
> > > are now Y2038 safe).
> > >
> > > Build tests:
> > > ./src/scripts/build-many-glibcs.py glibcs  
> >
> > Looks good, I have the same thing in my tree :)  
> 
> I think it's slightly different: __suseconds_t on rv32 is 64 bit
> wide, while on all existing 32-bit architectures as well as sparc64
> it is 32-bit wide.
> 
> While almost all kernel interfaces based on timeval have been replaced
> by those based on timespec, there are a small number that need this
> to be compatible with the kernel's layout, or to have an explicit
> conversion:
> 
> - Socket timestamps with SO_TIMESTAMP
> - Socket timeouts with SO_RCVTIMEO/SO_SNDTIMEO
> - pc-style parallel ports with the PPGETTIME/PPSETTIME ioctls
> - video4linux with the VIDIOC_QUERYBUF/VIDIOC_QBUF/VIDIOC_DQBUF
>   VIDIOC_PREPARE_BUF and VIDIOC_OMAP3ISP_STAT_REQ ioctls
> 
>        Arnd

I've kept the __suseconds_t type from the original definition of struct
timeval.

Moreover, as I've noted in the commit message - the struct __timeval64
is NOT supposed to be passed directly to Linux kernel and shall be
explicitly converted if needed (thanks Arnd for pointing out exact
situations where conversion will be needed).

Or maybe somebody has better idea? Comments are more than welcome.

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Arnd Bergmann Jan. 19, 2020, 3:39 p.m. UTC | #4 
On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> > <alistair23@gmail.com> wrote:
> >
> > - Socket timestamps with SO_TIMESTAMP
> > - Socket timeouts with SO_RCVTIMEO/SO_SNDTIMEO
> > - pc-style parallel ports with the PPGETTIME/PPSETTIME ioctls
> > - video4linux with the VIDIOC_QUERYBUF/VIDIOC_QBUF/VIDIOC_DQBUF
> >   VIDIOC_PREPARE_BUF and VIDIOC_OMAP3ISP_STAT_REQ ioctls
> >
> >        Arnd
>
> I've kept the __suseconds_t type from the original definition of struct
> timeval.
>
> Moreover, as I've noted in the commit message - the struct __timeval64
> is NOT supposed to be passed directly to Linux kernel and shall be
> explicitly converted if needed (thanks Arnd for pointing out exact
> situations where conversion will be needed).
>
> Or maybe somebody has better idea? Comments are more than welcome.

musl went to a 64-bit suseconds_t, same as the riscv32 port on glibc.

It would certainly help if this could be consistent across architectures
and libraries.

      Arnd
Lukasz Majewski Jan. 19, 2020, 9:30 p.m. UTC | #5 
Hi Arnd,

> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> > > <alistair23@gmail.com> wrote:
> > >
> > > - Socket timestamps with SO_TIMESTAMP
> > > - Socket timeouts with SO_RCVTIMEO/SO_SNDTIMEO
> > > - pc-style parallel ports with the PPGETTIME/PPSETTIME ioctls
> > > - video4linux with the VIDIOC_QUERYBUF/VIDIOC_QBUF/VIDIOC_DQBUF
> > >   VIDIOC_PREPARE_BUF and VIDIOC_OMAP3ISP_STAT_REQ ioctls
> > >
> > >        Arnd  
> >
> > I've kept the __suseconds_t type from the original definition of
> > struct timeval.
> >
> > Moreover, as I've noted in the commit message - the struct
> > __timeval64 is NOT supposed to be passed directly to Linux kernel
> > and shall be explicitly converted if needed (thanks Arnd for
> > pointing out exact situations where conversion will be needed).
> >
> > Or maybe somebody has better idea? Comments are more than welcome.  
> 
> musl went to a 64-bit suseconds_t, same as the riscv32 port on glibc.
> 
> It would certainly help if this could be consistent across
> architectures and libraries.

Ok. Thanks for the input.

> 
>       Arnd




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Lukasz Majewski Jan. 20, 2020, 12:34 p.m. UTC | #6 
Hi Arnd,

> Hi Arnd,
> 
> > On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > > > On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> > > > <alistair23@gmail.com> wrote:
> > > >
> > > > - Socket timestamps with SO_TIMESTAMP
> > > > - Socket timeouts with SO_RCVTIMEO/SO_SNDTIMEO
> > > > - pc-style parallel ports with the PPGETTIME/PPSETTIME ioctls
> > > > - video4linux with the VIDIOC_QUERYBUF/VIDIOC_QBUF/VIDIOC_DQBUF
> > > >   VIDIOC_PREPARE_BUF and VIDIOC_OMAP3ISP_STAT_REQ ioctls
> > > >
> > > >        Arnd    
> > >
> > > I've kept the __suseconds_t type from the original definition of
> > > struct timeval.
> > >
> > > Moreover, as I've noted in the commit message - the struct
> > > __timeval64 is NOT supposed to be passed directly to Linux kernel
> > > and shall be explicitly converted if needed (thanks Arnd for
> > > pointing out exact situations where conversion will be needed).
> > >
> > > Or maybe somebody has better idea? Comments are more than
> > > welcome.    
> > 
> > musl went to a 64-bit suseconds_t, same as the riscv32 port on
> > glibc.
> > 
> > It would certainly help if this could be consistent across
> > architectures and libraries.  
> 
> Ok. Thanks for the input.

Would it be OK to have:

struct __timeval64
{
  __time64_t tv_sec;         /* Seconds */
  __int64_t tv_usec;       /* Microseconds */
};

I would prefer to avoid changing typedef of suseconds_t as it may
affect struct timeval related operations.

Using explicitly __int64_t for tv_usec seems better option, isn't it?

> 
> > 
> >       Arnd  
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma@denx.de




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Arnd Bergmann Jan. 20, 2020, 1 p.m. UTC | #7 
On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:
> > > > > On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis <alistair23@gmail.com> wrote:
> > >
> > > It would certainly help if this could be consistent across
> > > architectures and libraries.
> >
> > Ok. Thanks for the input.
>
> Would it be OK to have:
>
> struct __timeval64
> {
>   __time64_t tv_sec;         /* Seconds */
>   __int64_t tv_usec;       /* Microseconds */
> };

This would not work if you pass it into a kernel interface that expects
a 32-bit suseconds_t on sparc64, but it should work as an internal
type on all 32-bit architectures.

> I would prefer to avoid changing typedef of suseconds_t as it may
> affect struct timeval related operations.
>
> Using explicitly __int64_t for tv_usec seems better option, isn't it?

Maybe a __suseconds64_t that happens to be 32-bit wide on sparc64?

     Arnd
Adhemerval Zanella Netto Jan. 20, 2020, 4:46 p.m. UTC | #8 
On 20/01/2020 10:00, Arnd Bergmann wrote:
> On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de> wrote:
>>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
>>>> wrote:
>>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis <alistair23@gmail.com> wrote:
>>>>
>>>> It would certainly help if this could be consistent across
>>>> architectures and libraries.
>>>
>>> Ok. Thanks for the input.
>>
>> Would it be OK to have:
>>
>> struct __timeval64
>> {
>>   __time64_t tv_sec;         /* Seconds */
>>   __int64_t tv_usec;       /* Microseconds */
>> };
> 
> This would not work if you pass it into a kernel interface that expects
> a 32-bit suseconds_t on sparc64, but it should work as an internal
> type on all 32-bit architectures.
> 
>> I would prefer to avoid changing typedef of suseconds_t as it may
>> affect struct timeval related operations.
>>
>> Using explicitly __int64_t for tv_usec seems better option, isn't it?
> 
> Maybe a __suseconds64_t that happens to be 32-bit wide on sparc64?

If timeval64 kernel ABI expects tv_usec being a different type depending
of the architecture, it is an indication we should parametrize with a 
__suseconds64_t.

As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
related to some alignment constraint?
Arnd Bergmann Jan. 20, 2020, 7:09 p.m. UTC | #9 
On Mon, Jan 20, 2020 at 5:46 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 20/01/2020 10:00, Arnd Bergmann wrote:
> > On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de> wrote:
> >>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
> >>>> wrote:
> >>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis <alistair23@gmail.com> wrote:
> >>>>
> >>>> It would certainly help if this could be consistent across
> >>>> architectures and libraries.
> >>>
> >>> Ok. Thanks for the input.
> >>
> >> Would it be OK to have:
> >>
> >> struct __timeval64
> >> {
> >>   __time64_t tv_sec;         /* Seconds */
> >>   __int64_t tv_usec;       /* Microseconds */
> >> };
> >
> > This would not work if you pass it into a kernel interface that expects
> > a 32-bit suseconds_t on sparc64, but it should work as an internal
> > type on all 32-bit architectures.
> >
> >> I would prefer to avoid changing typedef of suseconds_t as it may
> >> affect struct timeval related operations.
> >>
> >> Using explicitly __int64_t for tv_usec seems better option, isn't it?
> >
> > Maybe a __suseconds64_t that happens to be 32-bit wide on sparc64?
>
> If timeval64 kernel ABI expects tv_usec being a different type depending
> of the architecture, it is an indication we should parametrize with a
> __suseconds64_t.
>
> As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
> related to some alignment constraint?

I think it's mostly for simplicity: we do need the microseconds to be
in the low bytes for compatibility with the 64-bit syscalls in the
kernel, so it's either a 64-bit microseconds value or the same
conditional padding that exists in timespec, with an extra special
case for sparc64.

The 64-bit case is slightly simpler for the few kernel interfaces that
take a timeval as input and then can avoid conditionally zeroing out
the upper 32 bits before checking the lower 32 bits again 1000000.
I think that is only needed for clock_adjtimex, VIDIOC_QBUF and
PPSETTIME though, everything else only passes a timeval
to user space.

       Arnd
Lukasz Majewski Jan. 21, 2020, 4:45 p.m. UTC | #10 
Hi Arnd, Adhemerval,

> On Mon, Jan 20, 2020 at 5:46 PM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
> > On 20/01/2020 10:00, Arnd Bergmann wrote:  
> > > On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > >>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
> > >>>> wrote:  
> > >>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> > >>>>>> <alistair23@gmail.com> wrote:  
> > >>>>
> > >>>> It would certainly help if this could be consistent across
> > >>>> architectures and libraries.  
> > >>>
> > >>> Ok. Thanks for the input.  
> > >>
> > >> Would it be OK to have:
> > >>
> > >> struct __timeval64
> > >> {
> > >>   __time64_t tv_sec;         /* Seconds */
> > >>   __int64_t tv_usec;       /* Microseconds */
> > >> };  
> > >
> > > This would not work if you pass it into a kernel interface that
> > > expects a 32-bit suseconds_t on sparc64, but it should work as an
> > > internal type on all 32-bit architectures.
> > >  
> > >> I would prefer to avoid changing typedef of suseconds_t as it may
> > >> affect struct timeval related operations.
> > >>
> > >> Using explicitly __int64_t for tv_usec seems better option,
> > >> isn't it?  
> > >
> > > Maybe a __suseconds64_t that happens to be 32-bit wide on
> > > sparc64?  
> >
> > If timeval64 kernel ABI expects tv_usec being a different type
> > depending of the architecture, it is an indication we should
> > parametrize with a __suseconds64_t.
> >
> > As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
> > related to some alignment constraint?  
> 
> I think it's mostly for simplicity: we do need the microseconds to be
> in the low bytes for compatibility with the 64-bit syscalls in the
> kernel, so it's either a 64-bit microseconds value or the same
> conditional padding that exists in timespec, with an extra special
> case for sparc64.
> 
> The 64-bit case is slightly simpler for the few kernel interfaces that
> take a timeval as input and then can avoid conditionally zeroing out
> the upper 32 bits before checking the lower 32 bits again 1000000.
> I think that is only needed for clock_adjtimex, VIDIOC_QBUF and
> PPSETTIME though, everything else only passes a timeval
> to user space.

I would opt for simpler approach :-)

Is the below code correct?
./include/time.h

struct __timeval64
{
   __time64_t tv_sec;         /* Seconds */
   __suseconds64_t tv_usec;       /* Microseconds */
};

However, I'm a bit confused on the correct place to define
__suseconds64_t.

Shall it be added to posix/bits/types.h?
__STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of
microseconds.  */

If yes, then wouldn't we break the compatibility by adding new type to
this file?

> 
>        Arnd




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Adhemerval Zanella Netto Jan. 21, 2020, 5:24 p.m. UTC | #11 
On 21/01/2020 13:45, Lukasz Majewski wrote:
> Hi Arnd, Adhemerval,
> 
>> On Mon, Jan 20, 2020 at 5:46 PM Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>> On 20/01/2020 10:00, Arnd Bergmann wrote:  
>>>> On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de>
>>>> wrote:  
>>>>>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski <lukma@denx.de>
>>>>>>> wrote:  
>>>>>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
>>>>>>>>> <alistair23@gmail.com> wrote:  
>>>>>>>
>>>>>>> It would certainly help if this could be consistent across
>>>>>>> architectures and libraries.  
>>>>>>
>>>>>> Ok. Thanks for the input.  
>>>>>
>>>>> Would it be OK to have:
>>>>>
>>>>> struct __timeval64
>>>>> {
>>>>>   __time64_t tv_sec;         /* Seconds */
>>>>>   __int64_t tv_usec;       /* Microseconds */
>>>>> };  
>>>>
>>>> This would not work if you pass it into a kernel interface that
>>>> expects a 32-bit suseconds_t on sparc64, but it should work as an
>>>> internal type on all 32-bit architectures.
>>>>  
>>>>> I would prefer to avoid changing typedef of suseconds_t as it may
>>>>> affect struct timeval related operations.
>>>>>
>>>>> Using explicitly __int64_t for tv_usec seems better option,
>>>>> isn't it?  
>>>>
>>>> Maybe a __suseconds64_t that happens to be 32-bit wide on
>>>> sparc64?  
>>>
>>> If timeval64 kernel ABI expects tv_usec being a different type
>>> depending of the architecture, it is an indication we should
>>> parametrize with a __suseconds64_t.
>>>
>>> As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
>>> related to some alignment constraint?  
>>
>> I think it's mostly for simplicity: we do need the microseconds to be
>> in the low bytes for compatibility with the 64-bit syscalls in the
>> kernel, so it's either a 64-bit microseconds value or the same
>> conditional padding that exists in timespec, with an extra special
>> case for sparc64.
>>
>> The 64-bit case is slightly simpler for the few kernel interfaces that
>> take a timeval as input and then can avoid conditionally zeroing out
>> the upper 32 bits before checking the lower 32 bits again 1000000.
>> I think that is only needed for clock_adjtimex, VIDIOC_QBUF and
>> PPSETTIME though, everything else only passes a timeval
>> to user space.
> 
> I would opt for simpler approach :-)
> 
> Is the below code correct?
> ./include/time.h
> 
> struct __timeval64
> {
>    __time64_t tv_sec;         /* Seconds */
>    __suseconds64_t tv_usec;       /* Microseconds */
> };
> 
> However, I'm a bit confused on the correct place to define
> __suseconds64_t.
> 
> Shall it be added to posix/bits/types.h?
> __STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of
> microseconds.  */


Yes, the idea is to add on posix/bits/types.h:

__STD_TYPE __SUSECONDS64_T_TYPE __suseconds64_t;

Then on generic bits bits/typesizes.h:

#define __SUSECONDS_T_TYPE      __SQUAD_TYPE;

And then also define it on each Linux bits/typesizes.h.

> 
> If yes, then wouldn't we break the compatibility by adding new type to
> this file?

What do you mean by break compatibility? It is a new type on an
installed header used by new types.
Lukasz Majewski Jan. 21, 2020, 9:40 p.m. UTC | #12 
Hi Adhemerval,

> On 21/01/2020 13:45, Lukasz Majewski wrote:
> > Hi Arnd, Adhemerval,
> >   
> >> On Mon, Jan 20, 2020 at 5:46 PM Adhemerval Zanella
> >> <adhemerval.zanella@linaro.org> wrote:  
> >>> On 20/01/2020 10:00, Arnd Bergmann wrote:    
> >>>> On Mon, Jan 20, 2020 at 1:34 PM Lukasz Majewski <lukma@denx.de>
> >>>> wrote:    
> >>>>>>> On Sun, Jan 19, 2020 at 4:22 PM Lukasz Majewski
> >>>>>>> <lukma@denx.de> wrote:    
> >>>>>>>>> On Sat, Jan 18, 2020 at 11:48 PM Alistair Francis
> >>>>>>>>> <alistair23@gmail.com> wrote:    
> >>>>>>>
> >>>>>>> It would certainly help if this could be consistent across
> >>>>>>> architectures and libraries.    
> >>>>>>
> >>>>>> Ok. Thanks for the input.    
> >>>>>
> >>>>> Would it be OK to have:
> >>>>>
> >>>>> struct __timeval64
> >>>>> {
> >>>>>   __time64_t tv_sec;         /* Seconds */
> >>>>>   __int64_t tv_usec;       /* Microseconds */
> >>>>> };    
> >>>>
> >>>> This would not work if you pass it into a kernel interface that
> >>>> expects a 32-bit suseconds_t on sparc64, but it should work as an
> >>>> internal type on all 32-bit architectures.
> >>>>    
> >>>>> I would prefer to avoid changing typedef of suseconds_t as it
> >>>>> may affect struct timeval related operations.
> >>>>>
> >>>>> Using explicitly __int64_t for tv_usec seems better option,
> >>>>> isn't it?    
> >>>>
> >>>> Maybe a __suseconds64_t that happens to be 32-bit wide on
> >>>> sparc64?    
> >>>
> >>> If timeval64 kernel ABI expects tv_usec being a different type
> >>> depending of the architecture, it is an indication we should
> >>> parametrize with a __suseconds64_t.
> >>>
> >>> As a side note, why tv_usec for timeval64 is a 64-bit value? Is it
> >>> related to some alignment constraint?    
> >>
> >> I think it's mostly for simplicity: we do need the microseconds to
> >> be in the low bytes for compatibility with the 64-bit syscalls in
> >> the kernel, so it's either a 64-bit microseconds value or the same
> >> conditional padding that exists in timespec, with an extra special
> >> case for sparc64.
> >>
> >> The 64-bit case is slightly simpler for the few kernel interfaces
> >> that take a timeval as input and then can avoid conditionally
> >> zeroing out the upper 32 bits before checking the lower 32 bits
> >> again 1000000. I think that is only needed for clock_adjtimex,
> >> VIDIOC_QBUF and PPSETTIME though, everything else only passes a
> >> timeval to user space.  
> > 
> > I would opt for simpler approach :-)
> > 
> > Is the below code correct?
> > ./include/time.h
> > 
> > struct __timeval64
> > {
> >    __time64_t tv_sec;         /* Seconds */
> >    __suseconds64_t tv_usec;       /* Microseconds */
> > };
> > 
> > However, I'm a bit confused on the correct place to define
> > __suseconds64_t.
> > 
> > Shall it be added to posix/bits/types.h?
> > __STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of
> > microseconds.  */  
> 
> 
> Yes, the idea is to add on posix/bits/types.h:
> 
> __STD_TYPE __SUSECONDS64_T_TYPE __suseconds64_t;
> 
> Then on generic bits bits/typesizes.h:
> 
> #define __SUSECONDS_T_TYPE      __SQUAD_TYPE;
> 
> And then also define it on each Linux bits/typesizes.h.

Ok.

> 
> > 
> > If yes, then wouldn't we break the compatibility by adding new type
> > to this file?  
> 
> What do you mean by break compatibility? It is a new type on an
> installed header used by new types.

Thanks for the explanation - now it is clear :-)


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff mbox

Patch

diff --git a/include/time.h b/include/time.h
index 047f431a1a..22ecacd0d8 100644
--- a/include/time.h
+++ b/include/time.h
@@ -93,6 +93,20 @@  struct __itimerspec64
 };
 #endif
 
+#if __TIMESIZE == 64
+# define __timeval64 timeval
+#else
+/* The glibc Y2038-proof struct __timeval64 structure for a time value.
+   This structure is NOT supposed to be passed to the Linux kernel.
+   Instead, it shall be converted to struct __timespec64 and time shall
+   be [sg]et via clock_[sg]ettime (which are now Y2038 safe).  */
+struct __timeval64
+{
+  __time64_t tv_sec;         /* Seconds */
+  __suseconds_t tv_usec;       /* Microseconds */
+};
+#endif
+
 #if __TIMESIZE == 64
 # define __ctime64 ctime
 #else