Re: [PATCH v5 02/23] kernel: Define gettimeofday vdso common code

[Vincenzo Frascino <vincenzo.frascino@arm.com>]

Hi Arnd,

thank you for your review.

On 22/02/2019 13:49, Arnd Bergmann wrote:
> On Fri, Feb 22, 2019 at 1:25 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
> 
>> +/*
>> + * The definitions below are required to overcome the limitations
>> + * of time_t on 32 bit architectures, which overflows in 2038.
>> + * The new code should use the replacements based on time64_t and
>> + * timespec64.
>> + *
>> + * The abstraction below will be updated once the migration to
>> + * time64_t is complete.
>> + */
>> +#ifdef CONFIG_GENERIC_VDSO_32
>> +#define __vdso_timespec                old_timespec32
>> +#define __vdso_timeval         old_timeval32
>> +#else
>> +#ifdef ENABLE_COMPAT_VDSO
>> +#define __vdso_timespec                old_timespec32
>> +#define __vdso_timeval         old_timeval32
>> +#else
>> +#define __vdso_timespec                __kernel_timespec
>> +#define __vdso_timeval         __kernel_old_timeval
>> +#endif /* CONFIG_COMPAT_VDSO */
>> +#endif /* CONFIG_GENERIC_VDSO_32 */
> 
> I don't think we need __vdso_timeval at all, just use
> __kernel_old_timeval everywhere. There is no generic
> 64-bit timeval type in the kernel, and there won't be because
> gettimeofday() is deprecated.
> 

Ok, I will update my implementation accordingly in v6.

> For __vdso_timespec, I see how you ended up with this
> redefinition, and it makes the current version of your patches
> easier, but I fear it will in turn make it harder to add the
> __kernel_old_timeval based variant.
> 

What is __kernel_old_timespec (based on you next email)? Why do you think it
will make harder to add the new variants?

-- 
Regards,
Vincenzo

> What I'd prefer to see here is to always use the fixed length
> types everywhere in the code, such as
> 
> static notrace int __cvdso_clock_gettime64(clockid_t clock,
>                                         struct __kernel_timespec *ts)
> {
> ... /* normal clock_gettime using 64-bit times */
> }
> 
> static notrace int __cvdso_clock_gettime32(clockid_t clock,
>                                         struct old_timespec32 *ts32)
> {
>      struct __kernel_timespec ts;
>      int ret = __cvdso_clock_gettime64(clock, &ts);
> 
>      ts32->tv_sec = ts->tv_sec;
>      ts32->tv_nsec = ts->tv_nsec;
> 
>      return ret;
> }
> 
> and then have two different versions for 32-bit and 64-bit
> architectures, calling one or the other function of the
> generic code.
> 
>         Arnd
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel