| Message ID | cover.1524570852.git.baolin.wang@linaro.org |
|---|---|
| Headers | show |
| Series | Fix year 2038 issue for sound subsystem | expand |
Dne 24.4.2018 v 14:06 Baolin Wang napsal(a): > Since many structures will use timespec type variables to record time stamp > in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset > tries to introduce new structures removing timespec type to compatible native > mode and compat mode. > > Moreover this patchset also converts the internal structrures to use timespec64 > type and related APIs. Thanks for your patchset. A few comments: It might be more nice to reuse the existing structures and put timespec64 to the reserved field and duplicate information (with the 32-bit wrapping for the old fields). It means that we do not need new ioctls and old libraries will be fine. It may make sense to define private snd_timespec32 and snd_timespec64 with s32/s64 types instead separate sec/usec fields. Jaroslav -- Jaroslav Kysela <perex@perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Apr 24, 2018 at 2:06 PM, Baolin Wang <baolin.wang@linaro.org> wrote: > Since many structures will use timespec type variables to record time stamp > in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset > tries to introduce new structures removing timespec type to compatible native > mode and compat mode. > > Moreover this patchset also converts the internal structrures to use timespec64 > type and related APIs. > > Arnd Bergmann (2): > ALSA: move snd_pcm_ioctl_sync_ptr_compat into pcm_native.c > ALSA: add new 32-bit layout for snd_pcm_mmap_status/control > > Baolin Wang (6): > ALSA: Replace timespec with timespec64 > ALSA: Avoid using timespec for struct snd_timer_status > ALSA: Avoid using timespec for struct snd_ctl_elem_value > ALSA: Avoid using timespec for struct snd_pcm_status > ALSA: Avoid using timespec for struct snd_rawmidi_status > ALSA: Avoid using timespec for struct snd_timer_tread Thanks a lot for picking this up again and including my two patches! I've done a private review yesterday, but will have another look either today or tomorrow to see if I missed anything. I've also applied the series to my randconfig build tree and will see if that finds any new bugs. The builder has already found a bug in one of my two patches, so there might be more. Arnd _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Apr 24, 2018 at 3:29 PM, Jaroslav Kysela <perex@perex.cz> wrote: > Dne 24.4.2018 v 14:06 Baolin Wang napsal(a): >> Since many structures will use timespec type variables to record time stamp >> in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset >> tries to introduce new structures removing timespec type to compatible native >> mode and compat mode. >> >> Moreover this patchset also converts the internal structrures to use timespec64 >> type and related APIs. > > Thanks for your patchset. A few comments: > > It might be more nice to reuse the existing structures and put > timespec64 to the reserved field and duplicate information (with the > 32-bit wrapping for the old fields). It means that we do not need new > ioctls and old libraries will be fine. The current approach is intended to make any user space work without source-level changes, i.e. you can still build an old alsa-lib package against a new glibc as long as you have the latest kernel headers (which the glibc requires for using 64-bit time_t). If we try to extend the structures in a different way, that requires user space changes, and existing source code would silently break on a future glibc. IMHO changing the source-level interface should only be done as a last resort for y2038. Note that most of the work is not required to keep working beyond 2038, but actually is required just to keep working with an updated glibc that redefines time_t to 64 bit. The audio timestamps should be in CLOCK_MONOTONIC for new user space anyway, and that doesn't overflow a 32-bit type. > It may make sense to define private snd_timespec32 and snd_timespec64 > with s32/s64 types instead separate sec/usec fields. Right, I have some ideas there as well as I saw one remaining issue in my patch for the status mmap: when building a program with 64-bit time_t, we still see a copy of the old structure but with an\ impossible layout. ARnd _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Wed, Apr 25, 2018 at 01:26:23PM +0200, Arnd Bergmann wrote: > On Wed, Apr 25, 2018 at 9:23 AM, Jaroslav Kysela <perex@perex.cz> wrote: > > We have almost everything hidden in the alsa-lib code for the > > applications and there is the protocol versioning, so we can detect the > > changes easily and handle the new fields in the library. > I think we are both misunderstanding each other here, let's try > to work out what changes are required in alsa-lib. The idea of > Baolin's patches that you can simply rebuild alsa-lib (or any other > library using the alsa kernel interface, if any exist) against a new > C library and still have working audio. It's probably also worth mentioning that there are userspaces that don't use alsa-lib at all, at least tinyalsa and salsa (if anyone still uses salsa). _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Apr 24, 2018 at 2:06 PM, Baolin Wang <baolin.wang@linaro.org> wrote: > -struct snd_pcm_mmap_status { > +/* > + * For mmap operations, we need the 64-bit layout, both for compat mode, > + * and for y2038 compatibility. For 64-bit applications, the two definitions > + * are identical, so we keep the traditional version. > + */ > +#ifdef __SND_STRUCT_TIME64 > +#define __snd_pcm_mmap_status64 snd_pcm_mmap_status > +#define __snd_pcm_mmap_control64 snd_pcm_mmap_control > +#define __snd_pcm_sync_ptr64 snd_pcm_sync_ptr > +#else > +#define __snd_pcm_mmap_status snd_pcm_mmap_status > +#define __snd_pcm_mmap_control snd_pcm_mmap_control > +#define __snd_pcm_sync_ptr snd_pcm_sync_ptr > +#endif > + > +struct __snd_pcm_mmap_status { > snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ > int pad1; /* Needed for 64 bit alignment */ > snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ One more thing here: this definition is slightly suboptimal because in an alsa-lib that gets built with 64-bit time_t, we end up with an unusable __snd_pcm_mmap_status structure (__snd_pcm_mmap_status64 works fine, and that would be the normal thing to use). Just in case we want to be able to build an alsa-lib that is capable of running both on new kernels with 64-bit time_t interfaces exposed to applications and also on old kernels that don't have the new ioctls, we probably want another fixup merged in: diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 18fbdcb2c7b6..638c717d3eb9 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -496,19 +496,30 @@ struct snd_pcm_status { #define __snd_pcm_mmap_status64 snd_pcm_mmap_status #define __snd_pcm_mmap_control64 snd_pcm_mmap_control #define __snd_pcm_sync_ptr64 snd_pcm_sync_ptr +#define __snd_timespec64 timespec +struct __snd_timespec { + __s32 tv_sec; + __s32 tv_nsec; +}; #else #define __snd_pcm_mmap_status snd_pcm_mmap_status #define __snd_pcm_mmap_control snd_pcm_mmap_control #define __snd_pcm_sync_ptr snd_pcm_sync_ptr +#define __snd_timespec timespec +struct __snd_timespec64 { + __s64 tv_sec; + __s64 tv_nsec; +}; + #endif struct __snd_pcm_mmap_status { snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ int pad1; /* Needed for 64 bit alignment */ snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ - struct timespec tstamp; /* Timestamp */ + struct __snd_timespec tstamp; /* Timestamp */ snd_pcm_state_t suspended_state; /* RO: suspended stream state */ - struct timespec audio_tstamp; /* from sample counter or wall clock */ + struct __snd_timespec audio_tstamp; /* from sample counter or wall clock */ }; struct __snd_pcm_mmap_control { @@ -532,11 +543,6 @@ struct __snd_pcm_sync_ptr { } c; }; -struct __snd_timespec64 { - __s64 tv_sec; - __s64 tv_nsec; -}; - #if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN) typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)]; typedef char __pad_after_uframe[0]; With this change, alsa-lib can either access whichever structure matches the glibc 'timespec' definition, or it can ask for __struct __snd_pcm_mmap_status and struct __snd_pcm_mmap_status64 explicitly, regardless of the time_t definition. Arnd _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel