On Friday 17 July 2015 14:09:47 Mark Brown wrote:
On Fri, Jul 17, 2015 at 02:59:48PM +0200, Arnd Bergmann wrote:
-struct snd_timer_tread { +struct __kernel_snd_timer_tread { int event;
- struct timespec tstamp;
- struct __kernel_timespec tstamp; unsigned int val;
};
Also, __kernel_timespec is defined to be always 64-bit wide. This means if we do this change (assuming we drop the #define above), then user space will always see the new definition of this structure, and programs compiled against the latest header will no longer work on older kernels.
Is this what you had in mind?
We could decide to do it like this, and we have historically done changes to the ioctl interface this way, but I'm not sure if we want to do it for all ioctls.
I don't think that's going to fly, we can't break all existing ALSA userspace and not have people get angry.
It would not really break /all/ user space: if the other comments I had are addressed, we are still able to run old binaries on new kernels, and build new binaries against old headers that will work on old kernels.
Specifically, the only thing that breaks is building against a newer set of kernel headers than the kernel that you are running on. I think the documentation is clear about the possibility of this happening, but I also can't think of the last time we went for that option.
The alternative is to leave the 'timespec' visible here for user space, so user programs will see either the old or the new definition of struct depending on their timespec definition, and only programs built with 64-bit time_t will require new kernels.
Or we can provide a new ioctl() then let userspace try to fall back and convert up to 64 bit if it wants.
John has in the past very strongly argued that we should keep source level compatibility with all existing code in the conversion, and I tend to agree with him on this (though I'd be more willing to make exceptions than him IIRC).
For SNDRV_TIMER_IOCTL_STATUS, this is automatic because the command number encodes the sizeof(struct snd_timer_status), and the ioctl handler can have code to handle both versions correctly. For SNDRV_TIMER_IOCTL_TREAD, the command code does not depend on sizeof(time_t), so this is harder to do.
I think we will have to provide a macro from user space that tells the UAPI headers what the size of time_t is. This means that here we'd end up with something like
#if BITS_PER_TIME_T == BITS_PER_LONG #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x02, int) #else #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x15, int) #endif
this way we'll be able to let user space implicitly do the correct setting that matches its timespec format.
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp) +{
- struct timespec64 tstamp64;
- tstamp64.tv_sec = tstamp->tv_sec;
- tstamp64.tv_nsec = tstamp->tv_nsec;
- snd_timer_notify64(timer, event, &tstamp64);
+}
This works, but I'd leave it up to Mark if he'd prefer to do the conversion bit by bit and change over all users of snd_timer_notify to use snd_timer_notify64, or to move them all at once and leave the function name unchanged.
That's more a question for Takashi than me, this is all generic userspace stuff that is common to all sound.
Right, of course.
Arnd