On Friday 17 July 2015 15:21:07 Bamvor Zhang Jian wrote:
diff --git a/include/sound/timer.h b/include/sound/timer.h index 7990469..2cfee32 100644 --- a/include/sound/timer.h +++ b/include/sound/timer.h @@ -120,6 +120,12 @@ struct snd_timer_instance { struct snd_timer_instance *master; }; +struct snd_timer_tread {
- int event;
- struct timespec64 tstamp;
- unsigned int val;
+};
/*
- Registering
*/ diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index a45be6b..f7e3793 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -29,6 +29,9 @@ #include <stdlib.h> #endif +#ifndef CONFIG_COMPAT_TIME +# define __kernel_timespec timespec +#endif
CONFIG_COMPAT_TIME cannot be used in a uapi header: whether user space uses a 64-bit or 32-bit time_t is independent of what gets implemented in the kernel.
- protocol version
*/ @@ -739,7 +742,7 @@ struct snd_timer_params { }; struct snd_timer_status {
- struct timespec tstamp; /* Timestamp - last update */
- struct __kernel_timespec tstamp;/* Timestamp - last update */ unsigned int resolution; /* current period resolution in ns */ unsigned int lost; /* counter of master tick lost */ unsigned int overrun; /* count of read queue overruns */
@@ -787,9 +790,9 @@ enum { SNDRV_TIMER_EVENT_MRESUME = SNDRV_TIMER_EVENT_RESUME + 10, }; -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.
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.
+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.
I can see six callers of snd_timer_notify, but they are all in the same file, so I'd expect it to be possible to convert them all together, e.g. by adding a patch that changes the prototype in all these callers after changing the ccallback prototype.
@@ -1702,7 +1712,8 @@ static int snd_timer_user_status(struct file *file, if (!tu->timeri) return -EBADFD; memset(&status, 0, sizeof(status));
- status.tstamp = tu->tstamp;
- status.tstamp.tv_sec = tu->tstamp.tv_sec;
- status.tstamp.tv_nsec = tu->tstamp.tv_nsec; status.resolution = snd_timer_resolution(tu->timeri); status.lost = tu->timeri->lost; status.overrun = tu->overrun;
With the change to the structure definition, this will now only handle the new structure size on patched kernels, but not work with old user space on native 32-bit kernels any more.
Your patch 2 fixes the case of handling both old compat 32-bit user space on 64-bit kernels as well as new compat 32-bit user space with 64-bit time_t, but I think you are missing the case of handling old 32-bit user space.
Note that we cannot use compat_ioctl() for native 32-bit kernels, so snd_timer_user_ioctl will now have to be changed to handle both cases.
@@ -1843,9 +1854,12 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer, struct snd_timer_user *tu; long result = 0, unit; int err = 0;
- struct __kernel_snd_timer_tread kttr;
- struct snd_timer_tread *ttrp;
tu = file->private_data;
- unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);
- unit = tu->tread ? sizeof(struct __kernel_snd_timer_tread) :
sizeof(struct snd_timer_read);
Now this is the part that gets really tricky: Instead of two cases (read and tread), we now have to handle three cases. Any user space that is compiled with 64-bit time_t needs to get the new structure, while old user space needs to get the old structure.
It looks like we already get this wrong for existing compat user space: running a 32-bit program on a 64-bit kernel will currently get the 64-bit version of struct snd_timer_tread and misinterpret that. We can probably fix both issues at the same time after introducing turning the tread flag into a three-way enum (or something along that lines).
I would recommend separating the tread changes from the user_status changes, as both of them are getting more complex now.
Arnd