Hi, Arnd
On 07/17/2015 08:59 PM, Arnd Bergmann wrote:
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.
Oh, sorry for doing this again. Originally, I want to follow your approach in "include/linux/time64.h" in patch[1] in order to work with(for y2038 safe) and without(for keeping abi unchaged) your patch[1]. On the other hand, when I write this commit, I found that there are some headers include the <linux/time.h>(most them are drivers): include/uapi/linux/videodev2.h, include/uapi/linux/input.h, include/uapi/linux/timex.h, include/uapi/linux/elfcore.h, include/uapi/linux/resource.h, include/uapi/linux/dvb/dmx.h, include/uapi/linux/dvb/video.h, include/uapi/linux/coda.h. CONFIG_COMPAT_TIME is used indirectly in uapi files. Do we need to fix these issues?
- 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?
Yes. It was what in my head. But I do not whether it ought to be.
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.
Do you mean we leave snd_timer_tread unchanged and introduce a new struct? (assuming we drop the #define above). struct snd_timer_tread64 { int event; struct timespec tstamp; struct __kernel_timespec tstamp; unsigned int val; };
#if BITS_PER_TIME_T == BITS_PER_LONG #define SNDRV_TIMER_IOCTL_TREAD _IOW('T', 0x02, int) #else #define SNDRV_TIMER_IOCTL_TREAD64 _IOW('T', 0x15, int) #endif
snd_timer_tread64 is used by SNDRV_TIMER_IOCTL_TREAD64.
IIUC, we should define SNDRV_TIMER_IOCTL_TREAD64 instead keep the same name for differenct ioctl, otherwise it is hard to support the old and new application binary in new kernel.
+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.
Yes, understand. I want to keep the function name unchanged too. I guess I will do it when pcm part of y2038 patches is ready. Otherwise, the sound subsystem is broken.
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.
Got you.
@@ -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.
Oh, I miss it, I will try to add it in my next version.
@@ -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).
It seems that the current tread flag select the read or tread. How about choose the following you mentioned to deal with it? if (tu->tread) if (BITS_PER_TIME_T == BITS_PER_LONG) unit = sizeof(struct snd_timer_tread); else unit = sizeof(struct snd_timer_tread64); else unit = sizeof(struct snd_timer_read);
I would recommend separating the tread changes from the user_status changes, as both of them are getting more complex now.
Ok.
Arnd
regards
bamvor [1] http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=y2...