Message ID | CAK8P3a39BOUa_2pnHZ6URjv6uKgEtiySEExhE5qEegqfk-CDkg@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, 06 Nov 2017 17:33:26 +0100, Arnd Bergmann wrote: > > On Sun, Nov 5, 2017 at 5:59 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Sun, 05 Nov 2017 14:16:28 +0100, > >> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote: > >> > On Thu, 02 Nov 2017 12:06:57 +0100, > >> > 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. > > Right, you mentioned the mmap interface at the kernel summit. This > is certainly the most tricky part and will probably require source-level > changes. > > Can you clarify a few things about the mmap() interface? > Is this specifically about "struct snd_pcm_mmap_status" on the > pcm device, or are there others? > > From what I can see, it's already fairly limited: > - on most architectures, it's completely disabled, only x86, ppc and > alpha allow it to start with, and user space can work around > the mmap not being available by falling back to ioctl if I read > the comments correctly > - alpha is not affected since time_t is always 64-bit > - x86 and ppc disable the mmap() in compat mode already because > of the same issue. If it comes to the worst, we can probably do > the same for x86-32 and ppc32, disabling the existing status mmap > for them as well, and change SNDRV_PCM_MMAP_OFFSET_STATUS > to a new value for 32-bit kernels that exposes the same structure > as 64-bit kernels. > - I think that since we always use an offset that is defined in the > header file, we can use the same trick for mmap that we have > for the ioctl command number: > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index c227ccba60ae..bcdbdac097d9 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t; > > enum { > SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, > - SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, > + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000, > SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, > + SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000, > }; > > +#if __BITS_PER_LONG == 64 > +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD > +#else > +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) > > sizeof(__kernel_long_t)) ? \ > + SNDRV_PCM_MMAP_OFFSET_STATUS64 : \ > + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD) > +#endif > + > union snd_pcm_sync_id { > unsigned char id[16]; > unsigned short id16[8]; > > Does that make sense? Yeah, that should work. But can we make the flip without the dynamic sizeof() comparison but some ifdef? The above doesn't allow the usage with switch(), for example. IOW, is there any macro indicating the 64bit user time_t? In theory we can have the shadow mmap for the compat timespec, and convert it always when the status gets changed. But I guess disabling the mmap should work simply as is, judging from the 64bit compat status. So, basically speaking, I find all fine with the suggested conversions. But, some details look fairly ugly, like the dynamic const evaluation in the above. If we can tidy up these devils, the code will become more readable. thanks, Takashi
On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Mon, 06 Nov 2017 17:33:26 +0100, >> --- a/include/uapi/sound/asound.h >> +++ b/include/uapi/sound/asound.h >> @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t; >> >> enum { >> SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, >> - SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, >> + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000, >> SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, >> + SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000, >> }; >> >> +#if __BITS_PER_LONG == 64 >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD >> +#else >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) > >> sizeof(__kernel_long_t)) ? \ >> + SNDRV_PCM_MMAP_OFFSET_STATUS64 : \ >> + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD) >> +#endif >> + >> union snd_pcm_sync_id { >> unsigned char id[16]; >> unsigned short id16[8]; >> >> Does that make sense? > > Yeah, that should work. > > But can we make the flip without the dynamic sizeof() comparison but > some ifdef? The above doesn't allow the usage with switch(), for > example. > > IOW, is there any macro indicating the 64bit user time_t? There is a macro defined by the C library, but so far we have not started relying on it in kernel headers, because there is no guarantee that this symbol is visible before sys/time.h has been included, and there are some cases where it's possible to include a kernel header before sys/time.h. In case of sound/asound.h, that should be no problem since we rely on having seen the definition on 'struct timeval' already today, and that must come from sys/time.h. Then we just need to make sure that all C libraries define the same macro. Are you sure about the switch()/case problem? I thought that worked in C99, the only problem would be using the macro outside of a function, e.g. as initalizer for a variable > In theory we can have the shadow mmap for the compat timespec, and > convert it always when the status gets changed. But I guess disabling > the mmap should work simply as is, judging from the 64bit compat > status. Ok. Arnd
On Thu, 09 Nov 2017 18:01:47 +0100, Arnd Bergmann wrote: > > On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Mon, 06 Nov 2017 17:33:26 +0100, > > >> --- a/include/uapi/sound/asound.h > >> +++ b/include/uapi/sound/asound.h > >> @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t; > >> > >> enum { > >> SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, > >> - SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, > >> + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000, > >> SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, > >> + SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000, > >> }; > >> > >> +#if __BITS_PER_LONG == 64 > >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD > >> +#else > >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) > > >> sizeof(__kernel_long_t)) ? \ > >> + SNDRV_PCM_MMAP_OFFSET_STATUS64 : \ > >> + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD) > >> +#endif > >> + > >> union snd_pcm_sync_id { > >> unsigned char id[16]; > >> unsigned short id16[8]; > >> > >> Does that make sense? > > > > Yeah, that should work. > > > > But can we make the flip without the dynamic sizeof() comparison but > > some ifdef? The above doesn't allow the usage with switch(), for > > example. > > > > IOW, is there any macro indicating the 64bit user time_t? > > There is a macro defined by the C library, but so far we have not > started relying on it in kernel headers, because there is no guarantee > that this symbol is visible before sys/time.h has been included, > and there are some cases where it's possible to include a kernel > header before sys/time.h. > > In case of sound/asound.h, that should be no problem since we rely > on having seen the definition on 'struct timeval' already today, and > that must come from sys/time.h. Then we just need to make sure that > all C libraries define the same macro. > > Are you sure about the switch()/case problem? I thought that worked > in C99, the only problem would be using the macro outside of a > function, e.g. as initalizer for a variable Hmm, OK it seems working. But, honestly speaking, it's too scaring. I'm OK if it were only in the kernel local code. But it's the API/ABI definition, which is referred by user-space... A more solid condition would be really appreciated. thanks, Takashi
On Thu, Nov 9, 2017 at 7:11 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Thu, 09 Nov 2017 18:01:47 +0100, > Arnd Bergmann wrote: >> >> On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote: >> > >> > IOW, is there any macro indicating the 64bit user time_t? >> >> There is a macro defined by the C library, but so far we have not >> started relying on it in kernel headers, because there is no guarantee >> that this symbol is visible before sys/time.h has been included, >> and there are some cases where it's possible to include a kernel >> header before sys/time.h. >> >> In case of sound/asound.h, that should be no problem since we rely >> on having seen the definition on 'struct timeval' already today, and >> that must come from sys/time.h. Then we just need to make sure that >> all C libraries define the same macro. >> >> Are you sure about the switch()/case problem? I thought that worked >> in C99, the only problem would be using the macro outside of a >> function, e.g. as initalizer for a variable > > Hmm, OK it seems working. > > But, honestly speaking, it's too scaring. I'm OK if it were only in > the kernel local code. But it's the API/ABI definition, which is > referred by user-space... > > A more solid condition would be really appreciated. I understand your concern here and agree it's really ugly. It did take us many attempts to come up with this trick for other cases, so my initial reaction would be to use the same thing everywhere since I know it works, but we can use #ifdef instead if you prefer that. I think we can use a single #ifdef variant to cover all cases, but I'd have to think about the x32 and x86-32 some more here. With this trick, we can make user space with new glibc use data structures that are compatible with 64-bit kernels and avoid the additional translation helpers: enum { SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, #if (__BITS_PER_LONG == 64) || !defined(__USE_TIME_BITS64) SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, #else /* New snd_pcm_mmap_status layout on 32-bit architectures */ SNDRV_PCM_MMAP_OFFSET_STATUS = 0x82000000, #endif }; struct snd_pcm_mmap_status { snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ int pad1; /* Needed for 64 bit alignment */ #if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64) __u64 hw_ptr; /* for compatibility with 64-bit layout */ #else snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ #endif struct timespec tstamp; /* Timestamp */ snd_pcm_state_t suspended_state; /* RO: suspended stream state */ #if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64) int pad2; #endif struct timespec audio_tstamp; /* from sample counter or wall clock */ }; #endif struct snd_pcm_mmap_control { #if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64) __u64 appl_ptr; /* RW: appl ptr (0...boundary-1) */ __u64 avail_min; /* RW: min available frames for wakeup */ #else snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */ snd_pcm_uframes_t avail_min; /* RW: min available frames for wakeup */ #endif }; struct snd_pcm_sync_ptr { unsigned int flags; #if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64) __u32 __pad; /* for x86-32 */ #endif union { struct snd_pcm_mmap_status status; unsigned char reserved[64]; } s; union { struct snd_pcm_mmap_control control; unsigned char reserved[64]; } c; }; struct snd_timer_tread { int event; #if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64) int __pad0; /* make alignment sane on x86_32 */ #endif struct timespec tstamp; unsigned int val; #if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64) int __pad1; #endif }; /* note: on x32, the number will change with a new glibc, but the behavior will not */ #if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64) #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x62, int) #else #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x02, int) #endif
On Fri, 10 Nov 2017 00:20:10 +0100, Arnd Bergmann wrote: > > On Thu, Nov 9, 2017 at 7:11 PM, Takashi Iwai <tiwai@suse.de> wrote: > > On Thu, 09 Nov 2017 18:01:47 +0100, > > Arnd Bergmann wrote: > >> > >> On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote: > >> > > >> > IOW, is there any macro indicating the 64bit user time_t? > >> > >> There is a macro defined by the C library, but so far we have not > >> started relying on it in kernel headers, because there is no guarantee > >> that this symbol is visible before sys/time.h has been included, > >> and there are some cases where it's possible to include a kernel > >> header before sys/time.h. > >> > >> In case of sound/asound.h, that should be no problem since we rely > >> on having seen the definition on 'struct timeval' already today, and > >> that must come from sys/time.h. Then we just need to make sure that > >> all C libraries define the same macro. > >> > >> Are you sure about the switch()/case problem? I thought that worked > >> in C99, the only problem would be using the macro outside of a > >> function, e.g. as initalizer for a variable > > > > Hmm, OK it seems working. > > > > But, honestly speaking, it's too scaring. I'm OK if it were only in > > the kernel local code. But it's the API/ABI definition, which is > > referred by user-space... > > > > A more solid condition would be really appreciated. > > I understand your concern here and agree it's really ugly. It did take us > many attempts to come up with this trick for other cases, so my initial > reaction would be to use the same thing everywhere since I know > it works, but we can use #ifdef instead if you prefer that. I think we > can use a single #ifdef variant to cover all cases, but I'd have to think > about the x32 and x86-32 some more here. With this trick, we can > make user space with new glibc use data structures that are compatible > with 64-bit kernels and avoid the additional translation helpers: > > enum { > SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, > SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, > #if (__BITS_PER_LONG == 64) || !defined(__USE_TIME_BITS64) Yeah, it's definitely better, more understandable! thanks, Takashi
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index c227ccba60ae..bcdbdac097d9 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t; enum { SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000, - SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000, + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000, SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000, + SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000, }; +#if __BITS_PER_LONG == 64 +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD +#else +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) > sizeof(__kernel_long_t)) ? \ + SNDRV_PCM_MMAP_OFFSET_STATUS64 : \ + SNDRV_PCM_MMAP_OFFSET_STATUS_OLD) +#endif + union snd_pcm_sync_id { unsigned char id[16];