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);
>> spin_lock_irq(&tu->qlock);
>> while ((long)count - result >= unit) {
>> while (!tu->qused) {
>
> 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=y2038-syscalls&id=9005d4f4a44fc56bd0a1fe7c08e8e3f13eb75de7
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
