From: Takashi Iwai <tiwai@suse.de> To: Arnd Bergmann <arnd@arndb.de> Cc: Baolin Wang <baolin.wang@linaro.org>, Jaroslav Kysela <perex@perex.cz>, Fabian Frederick <fabf@skynet.be>, Arvind Yadav <arvind.yadav.cs@gmail.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, alsa-devel@alsa-project.org, Vinod Koul <vinod.koul@intel.com>, hardik.t.shah@intel.com, guneshwor.o.singh@intel.com, Liam Girdwood <lgirdwood@gmail.com>, SF Markus Elfring <elfring@users.sourceforge.net>, gudishax.kranthikumar@intel.com, Mark Brown <broonie@kernel.org>, Bhumika Goyal <bhumirks@gmail.com>, Naveen M <naveen.m@intel.com>, jeeja.kp@intel.com, Takashi Sakamoto <o-takashi@sakamocchi.jp>, subhransu.s.prusty@intel.com, Ingo Molnar <mingo@kernel.org>, Dan Carpenter <dan.carpenter@oracle.com> Subject: Re: [alsa-devel] [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread Date: Sun, 05 Nov 2017 17:59:18 +0100 [thread overview] Message-ID: <s5hr2tcy849.wl-tiwai@suse.de> (raw) In-Reply-To: <CAK8P3a3B_QVJwQGQ5Uf07XzgZ3QBaybq_+K_wNeBH5-h5ThoLg@mail.gmail.com> On Sun, 05 Nov 2017 14:16:28 +0100, Arnd Bergmann wrote: > > On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 02 Nov 2017 12:06:57 +0100, > > Baolin Wang wrote: > >> > >> The struct snd_timer_tread will use 'timespec' type variables to record > >> timestamp, which is not year 2038 safe on 32bits system. > >> > >> Since the struct snd_timer_tread is passed through read() rather than > >> ioctl(), and the read syscall has no command number that lets us pick > >> between the 32-bit or 64-bit version of this structure. > >> > >> Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new > >> struct snd_timer_tread64 replacing timespec with s64 type to handle > >> 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to > >> handle 64bit time_t with 32bit alignment. That means we will set > >> tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t, > >> then we will copy to user with struct snd_timer_tread64. For x86_32 > >> mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy > >> struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will > >> use 32bit time_t variables when copying to user. > > > > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where > > user-space can tell which protocol version it understands. If the > > protocol version is higher than some definition, we can assume it's > > 64-bit ready. The *_USER_PVERSION is issued from alsa-lib side. > > In that way, we can extend the ABI more flexibly. A similar trick was > > already used in PCM ABI. (Ditto for control and rawmidi API; we > > should have the same mechanism for all relevant APIs). > > > > Moreover, once when the new protocol is used, we can use the standard > > 64bit monotonic nsecs as a timestamp, so that we don't need to care > > about 32/64bit compatibility. > > I think that's fine, we can do that too, but I don't see how we get around > to doing something like Baolin's patch first. Without this, we will get > existing user space code compiling against our kernel headers using a > new glibc release that will inadvertently change the structure layout > on the read file descriptor. But it won't work in anyway in multiple ways, e.g. this timer read stuff and another the structs embedded in the mmappged page. If you do rebuild things with new glibc, it should tell kernel about the new ABI in anyway more or less explicitly. And if you need it, it means that some source-code level API change would be possible. Of course, passing which data type is another question. Maybe 64bit nsecs wouldn't fit all places, and timespec64 style would be still required. But still, the current patch looks still too unnecessarily complex to me. (Yeah I know that the problem is complex, but the code can be simpler, I hope!) > The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that > configuration lets the kernel know what API the user space expects > without requiring source-level changes. Right, it works for this case, but not always. If jumping the API would give a cleaner way of implementation, I'd prefer that over too hackeries, IMO. > If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move > to a new interface for y2038-safety, we'd have to redefined the structure > to avoid the libc-provided 'struct timespec' on 32-bit architectures, like: > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 299a822d2c4e..f93cace4cd24 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -801,7 +801,14 @@ enum { > > struct snd_timer_tread { > int event; > +#if __BITS_PER_LONG == 32 > + struct { > + __kernel_long_t tv_sec; > + __kernel_long_t tv_usec; > + }; > +#else > struct timespec tstamp; > +#endif > unsigned int val; > }; > > This has a somewhat higher risk of breaking existing code (since the type > changes), and it doesn't solve the overflow. Hm, how to define the timestamp type is one of the biggest questions indeed. In general, there can't be any guarantee that just rebuilding with the 64bit timespec would work for all existing codes. In theory it shouldn't break, but who knows... thanks, Takashi
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Arnd Bergmann <arnd@arndb.de> Cc: jeeja.kp@intel.com, gudishax.kranthikumar@intel.com, alsa-devel@alsa-project.org, Liam Girdwood <lgirdwood@gmail.com>, Baolin Wang <baolin.wang@linaro.org>, Vinod Koul <vinod.koul@intel.com>, hardik.t.shah@intel.com, Ingo Molnar <mingo@kernel.org>, guneshwor.o.singh@intel.com, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Fabian Frederick <fabf@skynet.be>, Mark Brown <broonie@kernel.org>, Dan Carpenter <dan.carpenter@oracle.com>, Naveen M <naveen.m@intel.com>, Arvind Yadav <arvind.yadav.cs@gmail.com>, Takashi Sakamoto <o-takashi@sakamocchi.jp>, subhransu.s.prusty@intel.com, SF Markus Elfring <elfring@users.sourceforge.net>, Bhumika Goyal <bhumirks@gmail.com> Subject: Re: [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread Date: Sun, 05 Nov 2017 17:59:18 +0100 [thread overview] Message-ID: <s5hr2tcy849.wl-tiwai@suse.de> (raw) In-Reply-To: <CAK8P3a3B_QVJwQGQ5Uf07XzgZ3QBaybq_+K_wNeBH5-h5ThoLg@mail.gmail.com> On Sun, 05 Nov 2017 14:16:28 +0100, Arnd Bergmann wrote: > > On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 02 Nov 2017 12:06:57 +0100, > > Baolin Wang wrote: > >> > >> The struct snd_timer_tread will use 'timespec' type variables to record > >> timestamp, which is not year 2038 safe on 32bits system. > >> > >> Since the struct snd_timer_tread is passed through read() rather than > >> ioctl(), and the read syscall has no command number that lets us pick > >> between the 32-bit or 64-bit version of this structure. > >> > >> Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new > >> struct snd_timer_tread64 replacing timespec with s64 type to handle > >> 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to > >> handle 64bit time_t with 32bit alignment. That means we will set > >> tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t, > >> then we will copy to user with struct snd_timer_tread64. For x86_32 > >> mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy > >> struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will > >> use 32bit time_t variables when copying to user. > > > > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where > > user-space can tell which protocol version it understands. If the > > protocol version is higher than some definition, we can assume it's > > 64-bit ready. The *_USER_PVERSION is issued from alsa-lib side. > > In that way, we can extend the ABI more flexibly. A similar trick was > > already used in PCM ABI. (Ditto for control and rawmidi API; we > > should have the same mechanism for all relevant APIs). > > > > Moreover, once when the new protocol is used, we can use the standard > > 64bit monotonic nsecs as a timestamp, so that we don't need to care > > about 32/64bit compatibility. > > I think that's fine, we can do that too, but I don't see how we get around > to doing something like Baolin's patch first. Without this, we will get > existing user space code compiling against our kernel headers using a > new glibc release that will inadvertently change the structure layout > on the read file descriptor. But it won't work in anyway in multiple ways, e.g. this timer read stuff and another the structs embedded in the mmappged page. If you do rebuild things with new glibc, it should tell kernel about the new ABI in anyway more or less explicitly. And if you need it, it means that some source-code level API change would be possible. Of course, passing which data type is another question. Maybe 64bit nsecs wouldn't fit all places, and timespec64 style would be still required. But still, the current patch looks still too unnecessarily complex to me. (Yeah I know that the problem is complex, but the code can be simpler, I hope!) > The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that > configuration lets the kernel know what API the user space expects > without requiring source-level changes. Right, it works for this case, but not always. If jumping the API would give a cleaner way of implementation, I'd prefer that over too hackeries, IMO. > If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move > to a new interface for y2038-safety, we'd have to redefined the structure > to avoid the libc-provided 'struct timespec' on 32-bit architectures, like: > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index 299a822d2c4e..f93cace4cd24 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -801,7 +801,14 @@ enum { > > struct snd_timer_tread { > int event; > +#if __BITS_PER_LONG == 32 > + struct { > + __kernel_long_t tv_sec; > + __kernel_long_t tv_usec; > + }; > +#else > struct timespec tstamp; > +#endif > unsigned int val; > }; > > This has a somewhat higher risk of breaking existing code (since the type > changes), and it doesn't solve the overflow. Hm, how to define the timestamp type is one of the biggest questions indeed. In general, there can't be any guarantee that just rebuilding with the 64bit timespec would work for all existing codes. In theory it shouldn't break, but who knows... thanks, Takashi
next prev parent reply other threads:[~2017-11-05 16:59 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-02 11:06 [RFC PATCH v2 0/7] Fix year 2038 issue for sound subsystem Baolin Wang 2017-11-02 11:06 ` [RFC PATCH v2 1/7] sound: Replace timespec with timespec64 Baolin Wang 2017-11-02 11:06 ` [RFC PATCH v2 2/7] sound: core: Avoid using timespec for struct snd_pcm_status Baolin Wang 2017-11-05 10:23 ` [alsa-devel] " Takashi Iwai 2017-11-05 10:23 ` Takashi Iwai 2017-11-05 13:48 ` [alsa-devel] " Arnd Bergmann 2017-11-02 11:06 ` [RFC PATCH v2 3/7] sound: core: Avoid using timespec for struct snd_rawmidi_status Baolin Wang 2017-11-02 11:06 ` [RFC PATCH v2 4/7] sound: core: Avoid using timespec for struct snd_timer_status Baolin Wang 2017-11-02 11:06 ` [RFC PATCH v2 5/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value Baolin Wang 2017-11-08 13:44 ` Takashi Sakamoto 2017-11-08 13:44 ` Takashi Sakamoto 2017-11-02 11:06 ` [RFC PATCH v2 6/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr Baolin Wang 2017-11-02 11:06 ` [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread Baolin Wang 2017-11-05 10:29 ` [alsa-devel] " Takashi Iwai 2017-11-05 13:16 ` Arnd Bergmann 2017-11-05 16:59 ` Takashi Iwai [this message] 2017-11-05 16:59 ` Takashi Iwai 2017-11-06 16:33 ` [alsa-devel] " Arnd Bergmann 2017-11-09 16:52 ` Takashi Iwai 2017-11-09 16:52 ` Takashi Iwai 2017-11-09 17:01 ` [alsa-devel] " Arnd Bergmann 2017-11-09 17:01 ` Arnd Bergmann 2017-11-09 18:11 ` [alsa-devel] " Takashi Iwai 2017-11-09 18:11 ` Takashi Iwai 2017-11-09 23:20 ` [alsa-devel] " Arnd Bergmann 2017-11-10 7:19 ` Takashi Iwai 2017-11-10 7:19 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=s5hr2tcy849.wl-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=alsa-devel@alsa-project.org \ --cc=arnd@arndb.de \ --cc=arvind.yadav.cs@gmail.com \ --cc=baolin.wang@linaro.org \ --cc=bhumirks@gmail.com \ --cc=broonie@kernel.org \ --cc=dan.carpenter@oracle.com \ --cc=elfring@users.sourceforge.net \ --cc=fabf@skynet.be \ --cc=gudishax.kranthikumar@intel.com \ --cc=guneshwor.o.singh@intel.com \ --cc=hardik.t.shah@intel.com \ --cc=jeeja.kp@intel.com \ --cc=lgirdwood@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mingo@kernel.org \ --cc=naveen.m@intel.com \ --cc=o-takashi@sakamocchi.jp \ --cc=perex@perex.cz \ --cc=subhransu.s.prusty@intel.com \ --cc=vinod.koul@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.