[RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Baolin Wang]

Since many structures will use timespec type variables to record time stamp
in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset
tries to introduce new structures removing timespec type to compatible native
mode and compat mode.

Moreover this patchset also converts the internal structrures to use timespec64
type and related APIs. 

Baolin Wang (7):
  sound: Replace timespec with timespec64
  sound: core: Avoid using timespec for struct snd_pcm_status
  sound: core: Avoid using timespec for struct snd_pcm_sync_ptr
  sound: core: Avoid using timespec for struct snd_rawmidi_status
  sound: core: Avoid using timespec for struct snd_timer_status
  uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
  sound: core: Avoid using timespec for struct snd_timer_tread

 include/sound/pcm.h               |  113 ++++++++-
 include/sound/timer.h             |    4 +-
 include/uapi/sound/asound.h       |   15 +-
 sound/core/pcm.c                  |   14 +-
 sound/core/pcm_compat.c           |  466 +++++++++++++++++++++++++++++--------
 sound/core/pcm_lib.c              |   33 +--
 sound/core/pcm_native.c           |  227 ++++++++++++++----
 sound/core/rawmidi.c              |   74 +++++-
 sound/core/rawmidi_compat.c       |   90 +++++--
 sound/core/timer.c                |  247 ++++++++++++++++----
 sound/core/timer_compat.c         |   25 +-
 sound/pci/hda/hda_controller.c    |   10 +-
 sound/soc/intel/skylake/skl-pcm.c |    4 +-
 13 files changed, 1046 insertions(+), 276 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/7] sound: Replace timespec with timespec64

[Baolin Wang]

Since timespec is not year 2038 safe on 32bit system, and we need to
convert all timespec variables to timespec64 type for sound subsystem.

This patch is used to do preparation for following patches, that will
convert all structures defined in uapi/sound/asound.h to use 64-bit
time_t.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/sound/pcm.h               |   18 +++++++++---------
 include/sound/timer.h             |    4 ++--
 sound/core/pcm_lib.c              |   30 +++++++++++++++++-------------
 sound/core/pcm_native.c           |   12 ++++++++----
 sound/core/timer.c                |   28 ++++++++++++++--------------
 sound/pci/hda/hda_controller.c    |   10 +++++-----
 sound/soc/intel/skylake/skl-pcm.c |    4 ++--
 7 files changed, 57 insertions(+), 49 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 24febf9..cd1ecd6 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -75,7 +75,7 @@ struct snd_pcm_ops {
 	int (*trigger)(struct snd_pcm_substream *substream, int cmd);
 	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
 	int (*get_time_info)(struct snd_pcm_substream *substream,
-			struct timespec *system_ts, struct timespec *audio_ts,
+			struct timespec64 *system_ts, struct timespec64 *audio_ts,
 			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
 			struct snd_pcm_audio_tstamp_report *audio_tstamp_report);
 	int (*fill_silence)(struct snd_pcm_substream *substream, int channel,
@@ -343,7 +343,7 @@ static inline void snd_pcm_pack_audio_tstamp_report(__u32 *data, __u32 *accuracy
 struct snd_pcm_runtime {
 	/* -- Status -- */
 	struct snd_pcm_substream *trigger_master;
-	struct timespec trigger_tstamp;	/* trigger timestamp */
+	struct timespec64 trigger_tstamp;	/* trigger timestamp */
 	bool trigger_tstamp_latched;     /* trigger timestamp latched in low-level driver/hardware */
 	int overrange;
 	snd_pcm_uframes_t avail_max;
@@ -419,7 +419,7 @@ struct snd_pcm_runtime {
 	/* -- audio timestamp config -- */
 	struct snd_pcm_audio_tstamp_config audio_tstamp_config;
 	struct snd_pcm_audio_tstamp_report audio_tstamp_report;
-	struct timespec driver_tstamp;
+	struct timespec64 driver_tstamp;
 
 #if IS_ENABLED(CONFIG_SND_PCM_OSS)
 	/* -- OSS things -- */
@@ -1167,22 +1167,22 @@ static inline void snd_pcm_set_runtime_buffer(struct snd_pcm_substream *substrea
 }
 
 /**
- * snd_pcm_gettime - Fill the timespec depending on the timestamp mode
+ * snd_pcm_gettime - Fill the timespec64 depending on the timestamp mode
  * @runtime: PCM runtime instance
- * @tv: timespec to fill
+ * @tv: timespec64 to fill
  */
 static inline void snd_pcm_gettime(struct snd_pcm_runtime *runtime,
-				   struct timespec *tv)
+				   struct timespec64 *tv)
 {
 	switch (runtime->tstamp_type) {
 	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC:
-		ktime_get_ts(tv);
+		ktime_get_ts64(tv);
 		break;
 	case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
-		getrawmonotonic(tv);
+		getrawmonotonic64(tv);
 		break;
 	default:
-		getnstimeofday(tv);
+		ktime_get_real_ts64(tv);
 		break;
 	}
 }
diff --git a/include/sound/timer.h b/include/sound/timer.h
index c4d76ff..c196c07 100644
--- a/include/sound/timer.h
+++ b/include/sound/timer.h
@@ -102,7 +102,7 @@ struct snd_timer_instance {
 			  unsigned long ticks, unsigned long resolution);
 	void (*ccallback) (struct snd_timer_instance * timeri,
 			   int event,
-			   struct timespec * tstamp,
+			   struct timespec64 * tstamp,
 			   unsigned long resolution);
 	void (*disconnect)(struct snd_timer_instance *timeri);
 	void *callback_data;
@@ -126,7 +126,7 @@ struct snd_timer_instance {
  */
 
 int snd_timer_new(struct snd_card *card, char *id, struct snd_timer_id *tid, struct snd_timer **rtimer);
-void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp);
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec64 *tstamp);
 int snd_timer_global_new(char *id, int device, struct snd_timer **rtimer);
 int snd_timer_global_free(struct snd_timer *timer);
 int snd_timer_global_register(struct snd_timer *timer);
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a93a423..5ca9dc3 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -158,8 +158,12 @@ static void xrun(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
 	trace_xrun(substream);
-	if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE)
-		snd_pcm_gettime(runtime, (struct timespec *)&runtime->status->tstamp);
+	if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
+		struct timespec64 tstamp;
+
+		snd_pcm_gettime(runtime, &tstamp);
+		runtime->status->tstamp = timespec64_to_timespec(tstamp);
+	}
 	snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
 	if (xrun_debug(substream, XRUN_DEBUG_BASIC)) {
 		char name[16];
@@ -217,12 +221,12 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream,
 }
 
 static void update_audio_tstamp(struct snd_pcm_substream *substream,
-				struct timespec *curr_tstamp,
-				struct timespec *audio_tstamp)
+				struct timespec64 *curr_tstamp,
+				struct timespec64 *audio_tstamp)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	u64 audio_frames, audio_nsecs;
-	struct timespec driver_tstamp;
+	struct timespec64 driver_tstamp;
 
 	if (runtime->tstamp_mode != SNDRV_PCM_TSTAMP_ENABLE)
 		return;
@@ -246,16 +250,16 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream,
 		}
 		audio_nsecs = div_u64(audio_frames * 1000000000LL,
 				runtime->rate);
-		*audio_tstamp = ns_to_timespec(audio_nsecs);
+		*audio_tstamp = ns_to_timespec64(audio_nsecs);
 	}
-	runtime->status->audio_tstamp = *audio_tstamp;
-	runtime->status->tstamp = *curr_tstamp;
+	runtime->status->audio_tstamp = timespec64_to_timespec(*audio_tstamp);
+	runtime->status->tstamp = timespec64_to_timespec(*curr_tstamp);
 
 	/*
 	 * re-take a driver timestamp to let apps detect if the reference tstamp
 	 * read by low-level hardware was provided with a delay
 	 */
-	snd_pcm_gettime(substream->runtime, (struct timespec *)&driver_tstamp);
+	snd_pcm_gettime(substream->runtime, &driver_tstamp);
 	runtime->driver_tstamp = driver_tstamp;
 }
 
@@ -268,8 +272,8 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	snd_pcm_sframes_t hdelta, delta;
 	unsigned long jdelta;
 	unsigned long curr_jiffies;
-	struct timespec curr_tstamp;
-	struct timespec audio_tstamp;
+	struct timespec64 curr_tstamp;
+	struct timespec64 audio_tstamp;
 	int crossed_boundary = 0;
 
 	old_hw_ptr = runtime->status->hw_ptr;
@@ -292,9 +296,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 
 			/* re-test in case tstamp type is not supported in hardware and was demoted to DEFAULT */
 			if (runtime->audio_tstamp_report.actual_type == SNDRV_PCM_AUDIO_TSTAMP_TYPE_DEFAULT)
-				snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
+				snd_pcm_gettime(runtime, &curr_tstamp);
 		} else
-			snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp);
+			snd_pcm_gettime(runtime, &curr_tstamp);
 	}
 
 	if (pos == SNDRV_PCM_POS_XRUN) {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 2fec2fe..60bc303 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -881,12 +881,12 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	status->suspended_state = runtime->status->suspended_state;
 	if (status->state == SNDRV_PCM_STATE_OPEN)
 		goto _end;
-	status->trigger_tstamp = runtime->trigger_tstamp;
+	status->trigger_tstamp = timespec64_to_timespec(runtime->trigger_tstamp);
 	if (snd_pcm_running(substream)) {
 		snd_pcm_update_hw_ptr(substream);
 		if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
 			status->tstamp = runtime->status->tstamp;
-			status->driver_tstamp = runtime->driver_tstamp;
+			status->driver_tstamp = timespec64_to_timespec(runtime->driver_tstamp);
 			status->audio_tstamp =
 				runtime->status->audio_tstamp;
 			if (runtime->audio_tstamp_report.valid == 1)
@@ -899,8 +899,12 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 		}
 	} else {
 		/* get tstamp only in fallback mode and only if enabled */
-		if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE)
-			snd_pcm_gettime(runtime, &status->tstamp);
+		if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
+			struct timespec64 tstamp;
+
+			snd_pcm_gettime(runtime, &tstamp);
+			status->tstamp = timespec64_to_timespec(tstamp);
+		}
 	}
  _tstamp_end:
 	status->appl_ptr = runtime->control->appl_ptr;
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 6cdd04a..f44d702 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -73,7 +73,7 @@ struct snd_timer_user {
 	spinlock_t qlock;
 	unsigned long last_resolution;
 	unsigned int filter;
-	struct timespec tstamp;		/* trigger tstamp */
+	struct timespec64 tstamp;		/* trigger tstamp */
 	wait_queue_head_t qchange_sleep;
 	struct fasync_struct *fasync;
 	struct mutex ioctl_lock;
@@ -408,12 +408,12 @@ static void snd_timer_notify1(struct snd_timer_instance *ti, int event)
 	struct snd_timer *timer;
 	unsigned long resolution = 0;
 	struct snd_timer_instance *ts;
-	struct timespec tstamp;
+	struct timespec64 tstamp;
 
 	if (timer_tstamp_monotonic)
-		ktime_get_ts(&tstamp);
+		ktime_get_ts64(&tstamp);
 	else
-		getnstimeofday(&tstamp);
+		ktime_get_real_ts64(&tstamp);
 	if (snd_BUG_ON(event < SNDRV_TIMER_EVENT_START ||
 		       event > SNDRV_TIMER_EVENT_PAUSE))
 		return;
@@ -957,7 +957,7 @@ static int snd_timer_dev_disconnect(struct snd_device *device)
 	return 0;
 }
 
-void snd_timer_notify(struct snd_timer *timer, int event, struct timespec *tstamp)
+void snd_timer_notify(struct snd_timer *timer, int event, struct timespec64 *tstamp)
 {
 	unsigned long flags;
 	unsigned long resolution = 0;
@@ -1251,7 +1251,7 @@ static void snd_timer_user_append_to_tqueue(struct snd_timer_user *tu,
 
 static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 				     int event,
-				     struct timespec *tstamp,
+				     struct timespec64 *tstamp,
 				     unsigned long resolution)
 {
 	struct snd_timer_user *tu = timeri->callback_data;
@@ -1265,7 +1265,7 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 		return;
 	memset(&r1, 0, sizeof(r1));
 	r1.event = event;
-	r1.tstamp = *tstamp;
+	r1.tstamp = timespec64_to_timespec(*tstamp);
 	r1.val = resolution;
 	spin_lock_irqsave(&tu->qlock, flags);
 	snd_timer_user_append_to_tqueue(tu, &r1);
@@ -1288,7 +1288,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 {
 	struct snd_timer_user *tu = timeri->callback_data;
 	struct snd_timer_tread *r, r1;
-	struct timespec tstamp;
+	struct timespec64 tstamp;
 	int prev, append = 0;
 
 	memset(&r1, 0, sizeof(r1));
@@ -1301,14 +1301,14 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 	}
 	if (tu->last_resolution != resolution || ticks > 0) {
 		if (timer_tstamp_monotonic)
-			ktime_get_ts(&tstamp);
+			ktime_get_ts64(&tstamp);
 		else
-			getnstimeofday(&tstamp);
+			ktime_get_real_ts64(&tstamp);
 	}
 	if ((tu->filter & (1 << SNDRV_TIMER_EVENT_RESOLUTION)) &&
 	    tu->last_resolution != resolution) {
 		r1.event = SNDRV_TIMER_EVENT_RESOLUTION;
-		r1.tstamp = tstamp;
+		r1.tstamp = timespec64_to_timespec(tstamp);
 		r1.val = resolution;
 		snd_timer_user_append_to_tqueue(tu, &r1);
 		tu->last_resolution = resolution;
@@ -1322,14 +1322,14 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 		prev = tu->qtail == 0 ? tu->queue_size - 1 : tu->qtail - 1;
 		r = &tu->tqueue[prev];
 		if (r->event == SNDRV_TIMER_EVENT_TICK) {
-			r->tstamp = tstamp;
+			r->tstamp = timespec64_to_timespec(tstamp);
 			r->val += ticks;
 			append++;
 			goto __wake;
 		}
 	}
 	r1.event = SNDRV_TIMER_EVENT_TICK;
-	r1.tstamp = tstamp;
+	r1.tstamp = timespec64_to_timespec(tstamp);
 	r1.val = ticks;
 	snd_timer_user_append_to_tqueue(tu, &r1);
 	append++;
@@ -1808,7 +1808,7 @@ 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 = timespec64_to_timespec(tu->tstamp);
 	status.resolution = snd_timer_resolution(tu->timeri);
 	status.lost = tu->timeri->lost;
 	status.overrun = tu->overrun;
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index d1eb148..c3e4516 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -502,7 +502,7 @@ static inline bool is_link_time_supported(struct snd_pcm_runtime *runtime,
 }
 
 static int azx_get_time_info(struct snd_pcm_substream *substream,
-			struct timespec *system_ts, struct timespec *audio_ts,
+			struct timespec64 *system_ts, struct timespec64 *audio_ts,
 			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
 			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
 {
@@ -522,7 +522,7 @@ static int azx_get_time_info(struct snd_pcm_substream *substream,
 		if (audio_tstamp_config->report_delay)
 			nsec = azx_adjust_codec_delay(substream, nsec);
 
-		*audio_ts = ns_to_timespec(nsec);
+		*audio_ts = ns_to_timespec64(nsec);
 
 		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK;
 		audio_tstamp_report->accuracy_report = 1; /* rest of structure is valid */
@@ -539,16 +539,16 @@ static int azx_get_time_info(struct snd_pcm_substream *substream,
 			return -EINVAL;
 
 		case SNDRV_PCM_TSTAMP_TYPE_MONOTONIC_RAW:
-			*system_ts = ktime_to_timespec(xtstamp.sys_monoraw);
+			*system_ts = ktime_to_timespec64(xtstamp.sys_monoraw);
 			break;
 
 		default:
-			*system_ts = ktime_to_timespec(xtstamp.sys_realtime);
+			*system_ts = ktime_to_timespec64(xtstamp.sys_realtime);
 			break;
 
 		}
 
-		*audio_ts = ktime_to_timespec(xtstamp.device);
+		*audio_ts = ktime_to_timespec64(xtstamp.device);
 
 		audio_tstamp_report->actual_type =
 			SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK_SYNCHRONIZED;
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 2b1e513..9f905c4 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1141,7 +1141,7 @@ static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream,
 }
 
 static int skl_get_time_info(struct snd_pcm_substream *substream,
-			struct timespec *system_ts, struct timespec *audio_ts,
+			struct timespec64 *system_ts, struct timespec64 *audio_ts,
 			struct snd_pcm_audio_tstamp_config *audio_tstamp_config,
 			struct snd_pcm_audio_tstamp_report *audio_tstamp_report)
 {
@@ -1159,7 +1159,7 @@ static int skl_get_time_info(struct snd_pcm_substream *substream,
 		if (audio_tstamp_config->report_delay)
 			nsec = skl_adjust_codec_delay(substream, nsec);
 
-		*audio_ts = ns_to_timespec(nsec);
+		*audio_ts = ns_to_timespec64(nsec);
 
 		audio_tstamp_report->actual_type = SNDRV_PCM_AUDIO_TSTAMP_TYPE_LINK;
 		audio_tstamp_report->accuracy_report = 1; /* rest of struct is valid */
-- 
1.7.9.5

Re: [RFC PATCH 1/7] sound: Replace timespec with timespec64

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> Since timespec is not year 2038 safe on 32bit system, and we need to
> convert all timespec variables to timespec64 type for sound subsystem.
>
> This patch is used to do preparation for following patches, that will
> convert all structures defined in uapi/sound/asound.h to use 64-bit
> time_t.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

Looks good to me. This could perhaps be split up further, but it
seems small enough to get merged as a single patch.

     Arnd

[RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Baolin Wang]

The struct snd_pcm_status will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
as commands to issue ioctl() to fill the 'snd_pcm_status' structure in
userspace. The command number is always defined through _IOR/_IOW/IORW,
so when userspace changes the definition of 'struct timespec' to use
64-bit types, the command number also changes.

Thus in the kernel, we now need to define two versions of each such ioctl
and corresponding ioctl commands to handle 32bit time_t and 64bit time_t
in native mode:
struct snd_pcm_status32 {
	......
	struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;
	struct { s32 tv_sec; s32 tv_nsec; } tstamp;
	......
}

struct snd_pcm_status64 {
	......
	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
	......
}

Moreover in compat file, we renamed or introduced new structures to handle
32bit/64bit time_t in compatible mode. 'struct compat_snd_pcm_status32' and
snd_pcm_status_user_compat() are used to handle 32bit time_t in compat mode.
'struct compat_snd_pcm_status64' and snd_pcm_status_user_compat64() are used
to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_status64_x86_32'
and snd_pcm_status_user_compat64_x86_32() are used to handle 64bit time_t with
32bit alignment.

Finally we can replace SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
with new commands and introduce new functions to fill new 'struct snd_pcm_status64'
instead of using unsafe 'struct snd_pcm_status'. Then in future, the new
commands can be matched when userspace changes 'timespec' to 64bit type
to make a size change of 'struct snd_pcm_status'. When glibc changes time_t
to 64-bit, any recompiled program will issue ioctl commands that the kernel
does not understand without this patch.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/sound/pcm.h     |   49 +++++++++-
 sound/core/pcm.c        |    8 +-
 sound/core/pcm_compat.c |  238 ++++++++++++++++++++++++++++++++++++-----------
 sound/core/pcm_native.c |  108 +++++++++++++++++----
 4 files changed, 324 insertions(+), 79 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index cd1ecd6..114cc29 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -58,6 +58,7 @@ struct snd_pcm_hardware {
 	size_t fifo_size;		/* fifo size in bytes */
 };
 
+struct snd_pcm_status64;
 struct snd_pcm_substream;
 
 struct snd_pcm_audio_tstamp_config; /* definitions further down */
@@ -565,8 +566,8 @@ struct snd_pcm_notify {
 int snd_pcm_info(struct snd_pcm_substream *substream, struct snd_pcm_info *info);
 int snd_pcm_info_user(struct snd_pcm_substream *substream,
 		      struct snd_pcm_info __user *info);
-int snd_pcm_status(struct snd_pcm_substream *substream,
-		   struct snd_pcm_status *status);
+int snd_pcm_status64(struct snd_pcm_substream *substream,
+		     struct snd_pcm_status64 *status);
 int snd_pcm_start(struct snd_pcm_substream *substream);
 int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status);
 int snd_pcm_drain_done(struct snd_pcm_substream *substream);
@@ -1440,4 +1441,48 @@ static inline u64 pcm_format_to_bits(snd_pcm_format_t pcm_format)
 #define pcm_dbg(pcm, fmt, args...) \
 	dev_dbg((pcm)->card->dev, fmt, ##args)
 
+struct snd_pcm_status64 {
+	snd_pcm_state_t state;		/* stream state */
+	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;	/* time when stream was started/stopped/paused */
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;		/* reference timestamp */
+	snd_pcm_uframes_t appl_ptr;	/* appl ptr */
+	snd_pcm_uframes_t hw_ptr;	/* hw ptr */
+	snd_pcm_sframes_t delay;	/* current delay in frames */
+	snd_pcm_uframes_t avail;	/* number of frames available */
+	snd_pcm_uframes_t avail_max;	/* max frames available on hw since last status */
+	snd_pcm_uframes_t overrange;	/* count of ADC (capture) overrange detections from last status */
+	snd_pcm_state_t suspended_state; /* suspended stream state */
+	__u32 audio_tstamp_data;	 /* needed for 64-bit alignment, used for configs/report to/from userspace */
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;	/* sample counter, wall clock, PHC or on-demand sync'ed */
+	struct { s64 tv_sec; s64 tv_nsec; } driver_tstamp;	/* useful in case reference system tstamp is reported with delay */
+	__u32 audio_tstamp_accuracy;	/* in ns units, only valid if indicated in audio_tstamp_data */
+	unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */
+};
+
+#define SNDRV_PCM_IOCTL_STATUS64	_IOR('A', 0x20, struct snd_pcm_status64)
+#define SNDRV_PCM_IOCTL_STATUS_EXT64	_IOWR('A', 0x24, struct snd_pcm_status64)
+
+#if __BITS_PER_LONG == 32
+struct snd_pcm_status32 {
+	snd_pcm_state_t state;		/* stream state */
+	struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;	/* time when stream was started/stopped/paused */
+	struct { s32 tv_sec; s32 tv_nsec; } tstamp;		/* reference timestamp */
+	snd_pcm_uframes_t appl_ptr;	/* appl ptr */
+	snd_pcm_uframes_t hw_ptr;	/* hw ptr */
+	snd_pcm_sframes_t delay;	/* current delay in frames */
+	snd_pcm_uframes_t avail;	/* number of frames available */
+	snd_pcm_uframes_t avail_max;	/* max frames available on hw since last status */
+	snd_pcm_uframes_t overrange;	/* count of ADC (capture) overrange detections from last status */
+	snd_pcm_state_t suspended_state; /* suspended stream state */
+	__u32 audio_tstamp_data;	 /* needed for 64-bit alignment, used for configs/report to/from userspace */
+	struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp;	/* sample counter, wall clock, PHC or on-demand sync'ed */
+	struct { s32 tv_sec; s32 tv_nsec; } driver_tstamp;	/* useful in case reference system tstamp is reported with delay */
+	__u32 audio_tstamp_accuracy;	/* in ns units, only valid if indicated in audio_tstamp_data */
+	unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */
+};
+
+#define SNDRV_PCM_IOCTL_STATUS32	_IOR('A', 0x20, struct snd_pcm_status32)
+#define SNDRV_PCM_IOCTL_STATUS_EXT32	_IOWR('A', 0x24, struct snd_pcm_status32)
+#endif
+
 #endif /* __SOUND_PCM_H */
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 7eadb7f..2d990d9 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -453,7 +453,7 @@ static void snd_pcm_substream_proc_status_read(struct snd_info_entry *entry,
 {
 	struct snd_pcm_substream *substream = entry->private_data;
 	struct snd_pcm_runtime *runtime;
-	struct snd_pcm_status status;
+	struct snd_pcm_status64 status;
 	int err;
 
 	mutex_lock(&substream->pcm->open_mutex);
@@ -463,16 +463,16 @@ static void snd_pcm_substream_proc_status_read(struct snd_info_entry *entry,
 		goto unlock;
 	}
 	memset(&status, 0, sizeof(status));
-	err = snd_pcm_status(substream, &status);
+	err = snd_pcm_status64(substream, &status);
 	if (err < 0) {
 		snd_iprintf(buffer, "error %d\n", err);
 		goto unlock;
 	}
 	snd_iprintf(buffer, "state: %s\n", snd_pcm_state_name(status.state));
 	snd_iprintf(buffer, "owner_pid   : %d\n", pid_vnr(substream->pid));
-	snd_iprintf(buffer, "trigger_time: %ld.%09ld\n",
+	snd_iprintf(buffer, "trigger_time: %lld.%09lld\n",
 		status.trigger_tstamp.tv_sec, status.trigger_tstamp.tv_nsec);
-	snd_iprintf(buffer, "tstamp      : %ld.%09ld\n",
+	snd_iprintf(buffer, "tstamp      : %lld.%09lld\n",
 		status.tstamp.tv_sec, status.tstamp.tv_nsec);
 	snd_iprintf(buffer, "delay       : %ld\n", status.delay);
 	snd_iprintf(buffer, "avail       : %ld\n", status.avail);
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index b719d0b..79e7475 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -187,7 +187,7 @@ static int snd_pcm_channel_info_user(struct snd_pcm_substream *substream,
 	snd_pcm_channel_info_user(s, p)
 #endif /* CONFIG_X86_X32 */
 
-struct snd_pcm_status32 {
+struct compat_snd_pcm_status32 {
 	s32 state;
 	struct compat_timespec trigger_tstamp;
 	struct compat_timespec tstamp;
@@ -207,13 +207,15 @@ struct snd_pcm_status32 {
 
 
 static int snd_pcm_status_user_compat(struct snd_pcm_substream *substream,
-				      struct snd_pcm_status32 __user *src,
+				      struct compat_snd_pcm_status32 __user *src,
 				      bool ext)
 {
-	struct snd_pcm_status status;
+	struct snd_pcm_status64 status;
+	struct compat_snd_pcm_status32 compat_status32;
 	int err;
 
 	memset(&status, 0, sizeof(status));
+	memset(&compat_status32, 0, sizeof(compat_status32));
 	/*
 	 * with extension, parameters are read/write,
 	 * get audio_tstamp_data from user,
@@ -222,38 +224,53 @@ static int snd_pcm_status_user_compat(struct snd_pcm_substream *substream,
 	if (ext && get_user(status.audio_tstamp_data,
 				(u32 __user *)(&src->audio_tstamp_data)))
 		return -EFAULT;
-	err = snd_pcm_status(substream, &status);
+	err = snd_pcm_status64(substream, &status);
 	if (err < 0)
 		return err;
 
 	if (clear_user(src, sizeof(*src)))
 		return -EFAULT;
-	if (put_user(status.state, &src->state) ||
-	    compat_put_timespec(&status.trigger_tstamp, &src->trigger_tstamp) ||
-	    compat_put_timespec(&status.tstamp, &src->tstamp) ||
-	    put_user(status.appl_ptr, &src->appl_ptr) ||
-	    put_user(status.hw_ptr, &src->hw_ptr) ||
-	    put_user(status.delay, &src->delay) ||
-	    put_user(status.avail, &src->avail) ||
-	    put_user(status.avail_max, &src->avail_max) ||
-	    put_user(status.overrange, &src->overrange) ||
-	    put_user(status.suspended_state, &src->suspended_state) ||
-	    put_user(status.audio_tstamp_data, &src->audio_tstamp_data) ||
-	    compat_put_timespec(&status.audio_tstamp, &src->audio_tstamp) ||
-	    compat_put_timespec(&status.driver_tstamp, &src->driver_tstamp) ||
-	    put_user(status.audio_tstamp_accuracy, &src->audio_tstamp_accuracy))
+
+	compat_status32 = (struct compat_snd_pcm_status32) {
+		.state = status.state,
+		.trigger_tstamp = {
+			.tv_sec = status.trigger_tstamp.tv_sec,
+			.tv_nsec = status.trigger_tstamp.tv_nsec,
+		},
+		.tstamp = {
+			.tv_sec = status.tstamp.tv_sec,
+			.tv_nsec = status.tstamp.tv_nsec,
+		},
+		.appl_ptr = status.appl_ptr,
+		.hw_ptr = status.hw_ptr,
+		.delay = status.delay,
+		.avail = status.avail,
+		.avail_max = status.avail_max,
+		.overrange = status.overrange,
+		.suspended_state = status.suspended_state,
+		.audio_tstamp_data = status.audio_tstamp_data,
+		.audio_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.driver_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.audio_tstamp_accuracy = status.audio_tstamp_accuracy,
+	};
+
+	if (copy_to_user(src, &compat_status32, sizeof(compat_status32)))
 		return -EFAULT;
 
 	return err;
 }
 
-#ifdef CONFIG_X86_X32
-/* X32 ABI has 64bit timespec and 64bit alignment */
-struct snd_pcm_status_x32 {
+struct compat_snd_pcm_status64 {
 	s32 state;
 	u32 rsvd; /* alignment */
-	struct timespec trigger_tstamp;
-	struct timespec tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
 	u32 appl_ptr;
 	u32 hw_ptr;
 	s32 delay;
@@ -262,22 +279,24 @@ struct snd_pcm_status_x32 {
 	u32 overrange;
 	s32 suspended_state;
 	u32 audio_tstamp_data;
-	struct timespec audio_tstamp;
-	struct timespec driver_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } driver_tstamp;
 	u32 audio_tstamp_accuracy;
-	unsigned char reserved[52-2*sizeof(struct timespec)];
+	unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
 } __packed;
 
 #define put_timespec(src, dst) copy_to_user(dst, src, sizeof(*dst))
 
-static int snd_pcm_status_user_x32(struct snd_pcm_substream *substream,
-				   struct snd_pcm_status_x32 __user *src,
-				   bool ext)
+static int snd_pcm_status_user_compat64(struct snd_pcm_substream *substream,
+					struct compat_snd_pcm_status64 __user *src,
+					bool ext)
 {
-	struct snd_pcm_status status;
+	struct snd_pcm_status64 status;
+	struct compat_snd_pcm_status64 compat_status64;
 	int err;
 
 	memset(&status, 0, sizeof(status));
+	memset(&compat_status64, 0, sizeof(compat_status64));
 	/*
 	 * with extension, parameters are read/write,
 	 * get audio_tstamp_data from user,
@@ -286,31 +305,128 @@ static int snd_pcm_status_user_x32(struct snd_pcm_substream *substream,
 	if (ext && get_user(status.audio_tstamp_data,
 				(u32 __user *)(&src->audio_tstamp_data)))
 		return -EFAULT;
-	err = snd_pcm_status(substream, &status);
+	err = snd_pcm_status64(substream, &status);
 	if (err < 0)
 		return err;
 
 	if (clear_user(src, sizeof(*src)))
 		return -EFAULT;
-	if (put_user(status.state, &src->state) ||
-	    put_timespec(&status.trigger_tstamp, &src->trigger_tstamp) ||
-	    put_timespec(&status.tstamp, &src->tstamp) ||
-	    put_user(status.appl_ptr, &src->appl_ptr) ||
-	    put_user(status.hw_ptr, &src->hw_ptr) ||
-	    put_user(status.delay, &src->delay) ||
-	    put_user(status.avail, &src->avail) ||
-	    put_user(status.avail_max, &src->avail_max) ||
-	    put_user(status.overrange, &src->overrange) ||
-	    put_user(status.suspended_state, &src->suspended_state) ||
-	    put_user(status.audio_tstamp_data, &src->audio_tstamp_data) ||
-	    put_timespec(&status.audio_tstamp, &src->audio_tstamp) ||
-	    put_timespec(&status.driver_tstamp, &src->driver_tstamp) ||
-	    put_user(status.audio_tstamp_accuracy, &src->audio_tstamp_accuracy))
+
+	compat_status64 = (struct compat_snd_pcm_status64) {
+		.state = status.state,
+		.trigger_tstamp = {
+			.tv_sec = status.trigger_tstamp.tv_sec,
+			.tv_nsec = status.trigger_tstamp.tv_nsec,
+		},
+		.tstamp = {
+			.tv_sec = status.tstamp.tv_sec,
+			.tv_nsec = status.tstamp.tv_nsec,
+		},
+		.appl_ptr = status.appl_ptr,
+		.hw_ptr = status.hw_ptr,
+		.delay = status.delay,
+		.avail = status.avail,
+		.avail_max = status.avail_max,
+		.overrange = status.overrange,
+		.suspended_state = status.suspended_state,
+		.audio_tstamp_data = status.audio_tstamp_data,
+		.audio_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.driver_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.audio_tstamp_accuracy = status.audio_tstamp_accuracy,
+	};
+
+	if (copy_to_user(src, &compat_status64, sizeof(compat_status64)))
 		return -EFAULT;
 
 	return err;
 }
-#endif /* CONFIG_X86_X32 */
+
+#ifdef IA32_EMULATION
+struct compat_snd_pcm_status64_x86_32 {
+	s32 state;
+	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	u32 appl_ptr;
+	u32 hw_ptr;
+	s32 delay;
+	u32 avail;
+	u32 avail_max;
+	u32 overrange;
+	s32 suspended_state;
+	u32 audio_tstamp_data;
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } driver_tstamp;
+	u32 audio_tstamp_accuracy;
+	unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
+} __packed;
+
+static int
+snd_pcm_status_user_compat64_x86_32(struct snd_pcm_substream *substream,
+				    struct compat_snd_pcm_status64_x86_32 __user *src,
+				    bool ext)
+{
+	struct snd_pcm_status64 status;
+	struct compat_snd_pcm_status64_x86_32 status_x86_32;
+	int err;
+
+	memset(&status, 0, sizeof(status));
+	memset(&status_x86_32, 0, sizeof(status_x86_32));
+	/*
+	 * with extension, parameters are read/write,
+	 * get audio_tstamp_data from user,
+	 * ignore rest of status structure
+	 */
+	if (ext && get_user(status.audio_tstamp_data,
+				(u32 __user *)(&src->audio_tstamp_data)))
+		return -EFAULT;
+	err = snd_pcm_status64(substream, &status);
+	if (err < 0)
+		return err;
+
+	if (clear_user(src, sizeof(*src)))
+		return -EFAULT;
+
+	status_x86_32 = (struct compat_snd_pcm_status64_x86_32) {
+		.state = status.state,
+		.trigger_tstamp = {
+			.tv_sec = status.trigger_tstamp.tv_sec,
+			.tv_nsec = status.trigger_tstamp.tv_nsec,
+		},
+		.tstamp = {
+			.tv_sec = status.tstamp.tv_sec,
+			.tv_nsec = status.tstamp.tv_nsec,
+		},
+		.appl_ptr = status.appl_ptr,
+		.hw_ptr = status.hw_ptr,
+		.delay = status.delay,
+		.avail = status.avail,
+		.avail_max = status.avail_max,
+		.overrange = status.overrange,
+		.suspended_state = status.suspended_state,
+		.audio_tstamp_data = status.audio_tstamp_data,
+		.audio_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.driver_tstamp = {
+			.tv_sec = status.audio_tstamp.tv_sec,
+			.tv_nsec = status.audio_tstamp.tv_nsec,
+		},
+		.audio_tstamp_accuracy = status.audio_tstamp_accuracy,
+	};
+
+	if (copy_to_user(src, &status_x86_32, sizeof(status_x86_32)))
+		return -EFAULT;
+
+	return err;
+}
+#endif
 
 /* both for HW_PARAMS and HW_REFINE */
 static int snd_pcm_ioctl_hw_params_compat(struct snd_pcm_substream *substream,
@@ -633,8 +749,8 @@ enum {
 	SNDRV_PCM_IOCTL_HW_REFINE32 = _IOWR('A', 0x10, struct snd_pcm_hw_params32),
 	SNDRV_PCM_IOCTL_HW_PARAMS32 = _IOWR('A', 0x11, struct snd_pcm_hw_params32),
 	SNDRV_PCM_IOCTL_SW_PARAMS32 = _IOWR('A', 0x13, struct snd_pcm_sw_params32),
-	SNDRV_PCM_IOCTL_STATUS32 = _IOR('A', 0x20, struct snd_pcm_status32),
-	SNDRV_PCM_IOCTL_STATUS_EXT32 = _IOWR('A', 0x24, struct snd_pcm_status32),
+	SNDRV_PCM_IOCTL_STATUS_COMPAT32 = _IOR('A', 0x20, struct compat_snd_pcm_status32),
+	SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32 = _IOWR('A', 0x24, struct compat_snd_pcm_status32),
 	SNDRV_PCM_IOCTL_DELAY32 = _IOR('A', 0x21, s32),
 	SNDRV_PCM_IOCTL_CHANNEL_INFO32 = _IOR('A', 0x32, struct snd_pcm_channel_info32),
 	SNDRV_PCM_IOCTL_REWIND32 = _IOW('A', 0x46, u32),
@@ -644,10 +760,14 @@ enum {
 	SNDRV_PCM_IOCTL_WRITEN_FRAMES32 = _IOW('A', 0x52, struct snd_xfern32),
 	SNDRV_PCM_IOCTL_READN_FRAMES32 = _IOR('A', 0x53, struct snd_xfern32),
 	SNDRV_PCM_IOCTL_SYNC_PTR32 = _IOWR('A', 0x23, struct snd_pcm_sync_ptr32),
+	SNDRV_PCM_IOCTL_STATUS_COMPAT64 = _IOR('A', 0x20, struct compat_snd_pcm_status64),
+	SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64 = _IOWR('A', 0x24, struct compat_snd_pcm_status64),
+#ifdef IA32_EMULATION
+	SNDRV_PCM_IOCTL_STATUS_COMPAT64_X86_32 = _IOR('A', 0x20, struct compat_snd_pcm_status64_x86_32),
+	SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32 = _IOWR('A', 0x24, struct compat_snd_pcm_status64_x86_32),
+#endif
 #ifdef CONFIG_X86_X32
 	SNDRV_PCM_IOCTL_CHANNEL_INFO_X32 = _IOR('A', 0x32, struct snd_pcm_channel_info),
-	SNDRV_PCM_IOCTL_STATUS_X32 = _IOR('A', 0x20, struct snd_pcm_status_x32),
-	SNDRV_PCM_IOCTL_STATUS_EXT_X32 = _IOWR('A', 0x24, struct snd_pcm_status_x32),
 	SNDRV_PCM_IOCTL_SYNC_PTR_X32 = _IOWR('A', 0x23, struct snd_pcm_sync_ptr_x32),
 #endif /* CONFIG_X86_X32 */
 };
@@ -697,9 +817,9 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 		return snd_pcm_ioctl_hw_params_compat(substream, 0, argp);
 	case SNDRV_PCM_IOCTL_SW_PARAMS32:
 		return snd_pcm_ioctl_sw_params_compat(substream, argp);
-	case SNDRV_PCM_IOCTL_STATUS32:
+	case SNDRV_PCM_IOCTL_STATUS_COMPAT32:
 		return snd_pcm_status_user_compat(substream, argp, false);
-	case SNDRV_PCM_IOCTL_STATUS_EXT32:
+	case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32:
 		return snd_pcm_status_user_compat(substream, argp, true);
 	case SNDRV_PCM_IOCTL_SYNC_PTR32:
 		return snd_pcm_ioctl_sync_ptr_compat(substream, argp);
@@ -719,11 +839,17 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 		return snd_pcm_ioctl_rewind_compat(substream, argp);
 	case SNDRV_PCM_IOCTL_FORWARD32:
 		return snd_pcm_ioctl_forward_compat(substream, argp);
+	case SNDRV_PCM_IOCTL_STATUS_COMPAT64:
+		return snd_pcm_status_user_compat64(substream, argp, false);
+	case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64:
+		return snd_pcm_status_user_compat64(substream, argp, true);
+#ifdef IA32_EMULATION
+	case SNDRV_PCM_IOCTL_STATUS_COMPAT64_X86_32:
+		return snd_pcm_status_user_compat64_x86_32(substream, argp, false);
+	case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32:
+		return snd_pcm_status_user_compat64_x86_32(substream, argp, true);
+#endif
 #ifdef CONFIG_X86_X32
-	case SNDRV_PCM_IOCTL_STATUS_X32:
-		return snd_pcm_status_user_x32(substream, argp, false);
-	case SNDRV_PCM_IOCTL_STATUS_EXT_X32:
-		return snd_pcm_status_user_x32(substream, argp, true);
 	case SNDRV_PCM_IOCTL_SYNC_PTR_X32:
 		return snd_pcm_ioctl_sync_ptr_x32(substream, argp);
 	case SNDRV_PCM_IOCTL_CHANNEL_INFO_X32:
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 60bc303..7f1f60d 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -854,8 +854,8 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream,
 	return err;
 }
 
-int snd_pcm_status(struct snd_pcm_substream *substream,
-		   struct snd_pcm_status *status)
+int snd_pcm_status64(struct snd_pcm_substream *substream,
+		     struct snd_pcm_status64 *status)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
@@ -881,14 +881,22 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	status->suspended_state = runtime->status->suspended_state;
 	if (status->state == SNDRV_PCM_STATE_OPEN)
 		goto _end;
-	status->trigger_tstamp = timespec64_to_timespec(runtime->trigger_tstamp);
+	status->trigger_tstamp.tv_sec = runtime->trigger_tstamp.tv_sec;
+	status->trigger_tstamp.tv_nsec = runtime->trigger_tstamp.tv_nsec;
 	if (snd_pcm_running(substream)) {
 		snd_pcm_update_hw_ptr(substream);
 		if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) {
-			status->tstamp = runtime->status->tstamp;
-			status->driver_tstamp = timespec64_to_timespec(runtime->driver_tstamp);
-			status->audio_tstamp =
-				runtime->status->audio_tstamp;
+			status->tstamp.tv_sec = runtime->status->tstamp.tv_sec;
+			status->tstamp.tv_nsec =
+				runtime->status->tstamp.tv_nsec;
+			status->driver_tstamp.tv_sec =
+				runtime->driver_tstamp.tv_sec;
+			status->driver_tstamp.tv_nsec =
+				runtime->driver_tstamp.tv_nsec;
+			status->audio_tstamp.tv_sec =
+				runtime->status->audio_tstamp.tv_sec;
+			status->audio_tstamp.tv_nsec =
+				runtime->status->audio_tstamp.tv_nsec;
 			if (runtime->audio_tstamp_report.valid == 1)
 				/* backwards compatibility, no report provided in COMPAT mode */
 				snd_pcm_pack_audio_tstamp_report(&status->audio_tstamp_data,
@@ -903,7 +911,8 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 			struct timespec64 tstamp;
 
 			snd_pcm_gettime(runtime, &tstamp);
-			status->tstamp = timespec64_to_timespec(tstamp);
+			status->tstamp.tv_sec = tstamp.tv_sec;
+			status->tstamp.tv_nsec = tstamp.tv_nsec;
 		}
 	}
  _tstamp_end:
@@ -933,11 +942,11 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-static int snd_pcm_status_user(struct snd_pcm_substream *substream,
-			       struct snd_pcm_status __user * _status,
-			       bool ext)
+static int snd_pcm_status_user64(struct snd_pcm_substream *substream,
+				 struct snd_pcm_status64 __user * _status,
+				 bool ext)
 {
-	struct snd_pcm_status status;
+	struct snd_pcm_status64 status;
 	int res;
 
 	memset(&status, 0, sizeof(status));
@@ -949,7 +958,7 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
 	if (ext && get_user(status.audio_tstamp_data,
 				(u32 __user *)(&_status->audio_tstamp_data)))
 		return -EFAULT;
-	res = snd_pcm_status(substream, &status);
+	res = snd_pcm_status64(substream, &status);
 	if (res < 0)
 		return res;
 	if (copy_to_user(_status, &status, sizeof(status)))
@@ -957,6 +966,65 @@ static int snd_pcm_status_user(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+#if __BITS_PER_LONG == 32
+static int snd_pcm_status_user32(struct snd_pcm_substream *substream,
+				 struct snd_pcm_status32 __user * _status,
+				 bool ext)
+{
+	struct snd_pcm_status64 status64;
+	struct snd_pcm_status32 status32;
+	int res;
+
+	memset(&status64, 0, sizeof(status64));
+	memset(&status32, 0, sizeof(status32));
+	/*
+	 * with extension, parameters are read/write,
+	 * get audio_tstamp_data from user,
+	 * ignore rest of status structure
+	 */
+	if (ext && get_user(status64.audio_tstamp_data,
+			    (u32 __user *)(&_status->audio_tstamp_data)))
+		return -EFAULT;
+	res = snd_pcm_status64(substream, &status64);
+	if (res < 0)
+		return res;
+
+	status32 = (struct snd_pcm_status32) {
+		.state = status64.state,
+		.trigger_tstamp = {
+			.tv_sec = status64.trigger_tstamp.tv_sec,
+			.tv_nsec = status64.trigger_tstamp.tv_nsec,
+		},
+		.tstamp = {
+			.tv_sec = status64.tstamp.tv_sec,
+			.tv_nsec = status64.tstamp.tv_nsec,
+		},
+		.appl_ptr = status64.appl_ptr,
+		.hw_ptr = status64.hw_ptr,
+		.delay = status64.delay,
+		.avail = status64.avail,
+		.avail_max = status64.avail_max,
+		.overrange = status64.overrange,
+		.suspended_state = status64.suspended_state,
+		.audio_tstamp_data = status64.audio_tstamp_data,
+		.audio_tstamp = {
+			.tv_sec = status64.audio_tstamp.tv_sec,
+			.tv_nsec = status64.audio_tstamp.tv_nsec,
+		},
+		.driver_tstamp = {
+			.tv_sec = status64.audio_tstamp.tv_sec,
+			.tv_nsec = status64.audio_tstamp.tv_nsec,
+		},
+		.audio_tstamp_accuracy = status64.audio_tstamp_accuracy,
+	};
+
+	if (copy_to_user(_status, &status32, sizeof(status32)))
+		return -EFAULT;
+
+	return 0;
+}
+#endif
+
 static int snd_pcm_channel_info(struct snd_pcm_substream *substream,
 				struct snd_pcm_channel_info * info)
 {
@@ -2888,10 +2956,16 @@ static int snd_pcm_common_ioctl(struct file *file,
 		return snd_pcm_hw_free(substream);
 	case SNDRV_PCM_IOCTL_SW_PARAMS:
 		return snd_pcm_sw_params_user(substream, arg);
-	case SNDRV_PCM_IOCTL_STATUS:
-		return snd_pcm_status_user(substream, arg, false);
-	case SNDRV_PCM_IOCTL_STATUS_EXT:
-		return snd_pcm_status_user(substream, arg, true);
+#if __BITS_PER_LONG == 32
+	case SNDRV_PCM_IOCTL_STATUS32:
+		return snd_pcm_status_user32(substream, arg, false);
+	case SNDRV_PCM_IOCTL_STATUS_EXT32:
+		return snd_pcm_status_user32(substream, arg, true);
+#endif
+	case SNDRV_PCM_IOCTL_STATUS64:
+		return snd_pcm_status_user64(substream, arg, false);
+	case SNDRV_PCM_IOCTL_STATUS_EXT64:
+		return snd_pcm_status_user64(substream, arg, true);
 	case SNDRV_PCM_IOCTL_CHANNEL_INFO:
 		return snd_pcm_channel_info_user(substream, arg);
 	case SNDRV_PCM_IOCTL_PREPARE:
-- 
1.7.9.5

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Takashi Iwai]

On Thu, 21 Sep 2017 08:18:04 +0200,
Baolin Wang wrote:
> 
> The struct snd_pcm_status will use 'timespec' type variables to record
> timestamp, which is not year 2038 safe on 32bits system.
> 
> Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
> as commands to issue ioctl() to fill the 'snd_pcm_status' structure in
> userspace. The command number is always defined through _IOR/_IOW/IORW,
> so when userspace changes the definition of 'struct timespec' to use
> 64-bit types, the command number also changes.
> 
> Thus in the kernel, we now need to define two versions of each such ioctl
> and corresponding ioctl commands to handle 32bit time_t and 64bit time_t
> in native mode:
> struct snd_pcm_status32 {
> 	......
> 	struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;
> 	struct { s32 tv_sec; s32 tv_nsec; } tstamp;
> 	......
> }
> 
> struct snd_pcm_status64 {
> 	......
> 	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
> 	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
> 	......
> }

I'm confused.  It's different from timespec64?  So 32bit user-space
would need to use a new own-type timespec instead of the standard
timespec that is compliant with y2038?


thanks,

Takashi

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Takashi Iwai]

On Thu, 21 Sep 2017 08:18:04 +0200,
Baolin Wang wrote:
> 
> The struct snd_pcm_status will use 'timespec' type variables to record
> timestamp, which is not year 2038 safe on 32bits system.
> 
> Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
> as commands to issue ioctl() to fill the 'snd_pcm_status' structure in
> userspace. The command number is always defined through _IOR/_IOW/IORW,
> so when userspace changes the definition of 'struct timespec' to use
> 64-bit types, the command number also changes.
> 
> Thus in the kernel, we now need to define two versions of each such ioctl
> and corresponding ioctl commands to handle 32bit time_t and 64bit time_t
> in native mode:
> struct snd_pcm_status32 {
> 	......
> 	struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;
> 	struct { s32 tv_sec; s32 tv_nsec; } tstamp;
> 	......
> }
> 
> struct snd_pcm_status64 {
> 	......
> 	struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
> 	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
> 	......
> }

I'm confused.  It's different from timespec64?  So 32bit user-space
would need to use a new own-type timespec instead of the standard
timespec that is compliant with y2038?


thanks,

Takashi

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 11:31 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 21 Sep 2017 08:18:04 +0200,
> Baolin Wang wrote:
>>
>> The struct snd_pcm_status will use 'timespec' type variables to record
>> timestamp, which is not year 2038 safe on 32bits system.
>>
>> Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
>> as commands to issue ioctl() to fill the 'snd_pcm_status' structure in
>> userspace. The command number is always defined through _IOR/_IOW/IORW,
>> so when userspace changes the definition of 'struct timespec' to use
>> 64-bit types, the command number also changes.
>>
>> Thus in the kernel, we now need to define two versions of each such ioctl
>> and corresponding ioctl commands to handle 32bit time_t and 64bit time_t
>> in native mode:
>> struct snd_pcm_status32 {
>>       ......
>>       struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;
>>       struct { s32 tv_sec; s32 tv_nsec; } tstamp;
>>       ......
>> }
>>
>> struct snd_pcm_status64 {
>>       ......
>>       struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
>>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;
>>       ......
>> }
>
> I'm confused.  It's different from timespec64?  So 32bit user-space
> would need to use a new own-type timespec instead of the standard
> timespec that is compliant with y2038?

It's complicated:

The definition of 'timespec' that user space sees comes from glibc,
and while that currently uses the traditional '{ long tv_sec;
long tv_nsec; }' definition, it will have to change to something like
(still simplified):

#if __32BIT && __64_BIT_TIME_T
typedef long long time_t;
#else
typedef long time_t;
#endif
struct timespec {
        time_t tv_sec;
#if __BIG_ENDIAN && __32BIT && __64_BIT_TIME_T
        unsigned int :32;
#endif
       long tv_nsec;
#if __LITTLE_ENDIAN && __32BIT && __64_BIT_TIME_T
        unsigned int pad;
#endif
} __attribute__((aligned(8)));

which matches the layout that a 64-bit kernel uses, aside
from the nanosecond padding.

The kernel uses timespec64 internally, which is defined as
"{ s64 tv_sec; long tv_nsec };", so this has the padding
in a different place on big-endian architectures, and has a
different alignment and size on i386. We plan to introduce
a 'struct __kernel_timespec' that is compatible with the
__64_BIT_TIME_T version of the user timespec, but that
doesn't exist yet.

If you prefer, we can probably introduce it now with Baolin's
series, I think Deepa was planning to post a patch to add
it soon anyway.

       Arnd

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Takashi Iwai]

On Fri, 22 Sep 2017 12:14:55 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Sep 22, 2017 at 11:31 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Thu, 21 Sep 2017 08:18:04 +0200,
> > Baolin Wang wrote:
> >>
> >> The struct snd_pcm_status will use 'timespec' type variables to record
> >> timestamp, which is not year 2038 safe on 32bits system.
> >>
> >> Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT
> >> as commands to issue ioctl() to fill the 'snd_pcm_status' structure in
> >> userspace. The command number is always defined through _IOR/_IOW/IORW,
> >> so when userspace changes the definition of 'struct timespec' to use
> >> 64-bit types, the command number also changes.
> >>
> >> Thus in the kernel, we now need to define two versions of each such ioctl
> >> and corresponding ioctl commands to handle 32bit time_t and 64bit time_t
> >> in native mode:
> >> struct snd_pcm_status32 {
> >>       ......
> >>       struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp;
> >>       struct { s32 tv_sec; s32 tv_nsec; } tstamp;
> >>       ......
> >> }
> >>
> >> struct snd_pcm_status64 {
> >>       ......
> >>       struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp;
> >>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;
> >>       ......
> >> }
> >
> > I'm confused.  It's different from timespec64?  So 32bit user-space
> > would need to use a new own-type timespec instead of the standard
> > timespec that is compliant with y2038?
> 
> It's complicated:
> 
> The definition of 'timespec' that user space sees comes from glibc,
> and while that currently uses the traditional '{ long tv_sec;
> long tv_nsec; }' definition, it will have to change to something like
> (still simplified):
> 
> #if __32BIT && __64_BIT_TIME_T
> typedef long long time_t;
> #else
> typedef long time_t;
> #endif
> struct timespec {
>         time_t tv_sec;
> #if __BIG_ENDIAN && __32BIT && __64_BIT_TIME_T
>         unsigned int :32;
> #endif
>        long tv_nsec;
> #if __LITTLE_ENDIAN && __32BIT && __64_BIT_TIME_T
>         unsigned int pad;
> #endif
> } __attribute__((aligned(8)));
> 
> which matches the layout that a 64-bit kernel uses, aside
> from the nanosecond padding.

Wow, that is really messy.


> The kernel uses timespec64 internally, which is defined as
> "{ s64 tv_sec; long tv_nsec };", so this has the padding
> in a different place on big-endian architectures, and has a
> different alignment and size on i386. We plan to introduce
> a 'struct __kernel_timespec' that is compatible with the
> __64_BIT_TIME_T version of the user timespec, but that
> doesn't exist yet.
> 
> If you prefer, we can probably introduce it now with Baolin's
> series, I think Deepa was planning to post a patch to add
> it soon anyway.

Yes, this sounds like a saner solution than defining the own timespec
at each place individually.  Then we can have better conversion
macros, too, I suppose.

And, if we have kernel_timespec (or kernel_timespec64 or such), can
this deprecate the existing timespec64 usages, too?  I see that
timespec64 is internal only, so consolidation would be beneficial for
code simplification.


Thanks!

Takashi

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 12:49 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Fri, 22 Sep 2017 12:14:55 +0200,
> Arnd Bergmann wrote:
>> The kernel uses timespec64 internally, which is defined as
>> "{ s64 tv_sec; long tv_nsec };", so this has the padding
>> in a different place on big-endian architectures, and has a
>> different alignment and size on i386. We plan to introduce
>> a 'struct __kernel_timespec' that is compatible with the
>> __64_BIT_TIME_T version of the user timespec, but that
>> doesn't exist yet.
>>
>> If you prefer, we can probably introduce it now with Baolin's
>> series, I think Deepa was planning to post a patch to add
>> it soon anyway.
>
> Yes, this sounds like a saner solution than defining the own timespec
> at each place individually.  Then we can have better conversion
> macros, too, I suppose.

Thinking about it again, we unfortunately can't use
__kernel_timespec until after all 32-bit architectures have
been converted to use the new syscalls that we still need
to introduce: In the meantime the plan is that '__kernel_timespec'
is an alias for the usual 'timespec' in user space and may still
be 32-bit wide.

I definitely agree that open-coding 'struct { s64 tv_sec;
s64 tv_nsec}' in a dozen locations is not overly helpful.

I suggested a different alternative in my reply to patch 3/7.
Can you have a look at that? The idea would be that we just
flatten all the structures in the ioctl implementation and make
the structure definition very explicit using u32/s32/u64/s64
members with no implied padding or architecture-specific
types.

> And, if we have kernel_timespec (or kernel_timespec64 or such), can
> this deprecate the existing timespec64 usages, too?  I see that
> timespec64 is internal only, so consolidation would be beneficial for
> code simplification.

Our current longterm plan is to only use __kernel_timespec on the
ABI side, where we have to watch out for the tricky conversion of
tv_nsec: Any timespec copied from a 32-bit process into the kernel
must ignore the upper half of the nanoseconds, while copying the
same structure from a 64-bit process must return an error if the
64-bit nanoseconds are larger than 999999999. When copying a
timespec into user space, we have to be careful to zero the upper
half of tv_nsec to avoid leaking uninitialized kernel data.

Inside of the kernel, we can ignore those constraints, so I'd keep
using the timespec64. We certainly don't want to use the 64-bit
nanoseconds field for internal uses on 32-bit kernels, as that
would introduce expensive 64-bit arithmetic in a lot of places
that don't need it.

My hope is also that we can eventually deprecate any use of the
plain 'timespec' in the kernel: all internal users should migrate
to timespec64 (one at a time, so we can properly review the
changes), and the uapi uses should either have the 64-bit
version of __kernel_timespec, or use compat_timespec once
that becomes usable on 32-bit architectures.

        Arnd

Re: [RFC PATCH 2/7] sound: core: Avoid using timespec for struct snd_pcm_status

[Takashi Iwai]

On Fri, 22 Sep 2017 13:43:16 +0200,
Arnd Bergmann wrote:
> 
> On Fri, Sep 22, 2017 at 12:49 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > On Fri, 22 Sep 2017 12:14:55 +0200,
> > Arnd Bergmann wrote:
> >> The kernel uses timespec64 internally, which is defined as
> >> "{ s64 tv_sec; long tv_nsec };", so this has the padding
> >> in a different place on big-endian architectures, and has a
> >> different alignment and size on i386. We plan to introduce
> >> a 'struct __kernel_timespec' that is compatible with the
> >> __64_BIT_TIME_T version of the user timespec, but that
> >> doesn't exist yet.
> >>
> >> If you prefer, we can probably introduce it now with Baolin's
> >> series, I think Deepa was planning to post a patch to add
> >> it soon anyway.
> >
> > Yes, this sounds like a saner solution than defining the own timespec
> > at each place individually.  Then we can have better conversion
> > macros, too, I suppose.
> 
> Thinking about it again, we unfortunately can't use
> __kernel_timespec until after all 32-bit architectures have
> been converted to use the new syscalls that we still need
> to introduce: In the meantime the plan is that '__kernel_timespec'
> is an alias for the usual 'timespec' in user space and may still
> be 32-bit wide.

OK.

> I definitely agree that open-coding 'struct { s64 tv_sec;
> s64 tv_nsec}' in a dozen locations is not overly helpful.
> 
> I suggested a different alternative in my reply to patch 3/7.
> Can you have a look at that? The idea would be that we just
> flatten all the structures in the ioctl implementation and make
> the structure definition very explicit using u32/s32/u64/s64
> members with no implied padding or architecture-specific
> types.

Thanks, I took a look at it, and I agree with you about the flatten
struct being easier to handle.


Though, one thing is still considered: snd_pcm_mmap_status struct is
not only passed via ioctl but it's embedded in a mmapped context.
So, differentiation by the struct size can't work with it (as already
you pointed about union, too).

It might sound like a contradiction what I wrote, but another possible
way would be to define 64bit timespec for the sound (both 64bit sec
and nsec), and use it consistently in all places no matter which
architecture is used.  It'd need the changes in alsa-lib or
tiny-alsa, of course.  But that's one of the reasons of the presence
of alsa-lib layer after all.

Recently we introduced the ioctl for user-space to inform the
supported ABI version (SNDRV_PCM_IOCTL_USER_PVERSION).  With this, we
can limit the 64bit timespec support only for the programs that issue
this ioctl with the new protocol version.  If not set or older ABI
version is given, it means still old 32bit timespec.  This can be used
for distinguish which mmapped struct is referred, too.

> > And, if we have kernel_timespec (or kernel_timespec64 or such), can
> > this deprecate the existing timespec64 usages, too?  I see that
> > timespec64 is internal only, so consolidation would be beneficial for
> > code simplification.
> 
> Our current longterm plan is to only use __kernel_timespec on the
> ABI side, where we have to watch out for the tricky conversion of
> tv_nsec: Any timespec copied from a 32-bit process into the kernel
> must ignore the upper half of the nanoseconds, while copying the
> same structure from a 64-bit process must return an error if the
> 64-bit nanoseconds are larger than 999999999. When copying a
> timespec into user space, we have to be careful to zero the upper
> half of tv_nsec to avoid leaking uninitialized kernel data.

Right.

> Inside of the kernel, we can ignore those constraints, so I'd keep
> using the timespec64. We certainly don't want to use the 64-bit
> nanoseconds field for internal uses on 32-bit kernels, as that
> would introduce expensive 64-bit arithmetic in a lot of places
> that don't need it.

OK, that's the reason of timespec64.  But this can be eventually
identical with __kernel_timespec?  It holds 64bit sec and 32bit nsec,
right?  The difference is the presence of padding, but having a pad
makes things arch-agnostic, so its cost looks paying well, I guess.

> My hope is also that we can eventually deprecate any use of the
> plain 'timespec' in the kernel: all internal users should migrate
> to timespec64 (one at a time, so we can properly review the
> changes), and the uapi uses should either have the 64-bit
> version of __kernel_timespec, or use compat_timespec once
> that becomes usable on 32-bit architectures.

Heh, let's keep our hope :)


thanks,

Takashi

[RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Baolin Wang]

The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64'
to handle 32bit time_t and 64bit time_t in native mode, which replace
timespec with s64 type.

In compat mode, we renamed or introduced new structures to handle 32bit/64bit
time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and
snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode.
'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used
to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32'
and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with
32bit alignment.

When glibc changes time_t to 64bit, any recompiled program will issue ioctl
commands that the kernel does not understand without this patch.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/sound/pcm.h     |   46 +++++++++-
 sound/core/pcm.c        |    6 +-
 sound/core/pcm_compat.c |  228 ++++++++++++++++++++++++++++++++++++++---------
 sound/core/pcm_lib.c    |    9 +-
 sound/core/pcm_native.c |  113 ++++++++++++++++++-----
 5 files changed, 329 insertions(+), 73 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 114cc29..c253cbf 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -64,6 +64,15 @@ struct snd_pcm_hardware {
 struct snd_pcm_audio_tstamp_config; /* definitions further down */
 struct snd_pcm_audio_tstamp_report;
 
+struct snd_pcm_mmap_status64 {
+	snd_pcm_state_t state;		/* RO: state - SNDRV_PCM_STATE_XXXX */
+	int pad1;			/* Needed for 64 bit alignment */
+	snd_pcm_uframes_t hw_ptr;	/* RO: hw ptr (0...boundary-1) */
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;		/* Timestamp */
+	snd_pcm_state_t suspended_state; /* RO: suspended stream state */
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;	/* from sample counter or wall clock */
+};
+
 struct snd_pcm_ops {
 	int (*open)(struct snd_pcm_substream *substream);
 	int (*close)(struct snd_pcm_substream *substream);
@@ -389,7 +398,7 @@ struct snd_pcm_runtime {
 	union snd_pcm_sync_id sync;	/* hardware synchronization ID */
 
 	/* -- mmap -- */
-	struct snd_pcm_mmap_status *status;
+	struct snd_pcm_mmap_status64 *status;
 	struct snd_pcm_mmap_control *control;
 
 	/* -- locking / scheduling -- */
@@ -1459,8 +1468,21 @@ struct snd_pcm_status64 {
 	unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */
 };
 
+struct snd_pcm_sync_ptr64 {
+	unsigned int flags;
+	union {
+		struct snd_pcm_mmap_status64 status;
+		unsigned char reserved[64];
+	} s;
+	union {
+		struct snd_pcm_mmap_control control;
+		unsigned char reserved[64];
+	} c;
+};
+
 #define SNDRV_PCM_IOCTL_STATUS64	_IOR('A', 0x20, struct snd_pcm_status64)
 #define SNDRV_PCM_IOCTL_STATUS_EXT64	_IOWR('A', 0x24, struct snd_pcm_status64)
+#define SNDRV_PCM_IOCTL_SYNC_PTR64	_IOWR('A', 0x23, struct snd_pcm_sync_ptr64)
 
 #if __BITS_PER_LONG == 32
 struct snd_pcm_status32 {
@@ -1481,8 +1503,30 @@ struct snd_pcm_status32 {
 	unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */
 };
 
+struct snd_pcm_mmap_status32 {
+	snd_pcm_state_t state;		/* RO: state - SNDRV_PCM_STATE_XXXX */
+	int pad1;			/* Needed for 64 bit alignment */
+	snd_pcm_uframes_t hw_ptr;	/* RO: hw ptr (0...boundary-1) */
+	struct { s32 tv_sec; s32 tv_nsec; } tstamp;		/* Timestamp */
+	snd_pcm_state_t suspended_state; /* RO: suspended stream state */
+	struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp;	/* from sample counter or wall clock */
+};
+
+struct snd_pcm_sync_ptr32 {
+	unsigned int flags;
+	union {
+		struct snd_pcm_mmap_status32 status;
+		unsigned char reserved[64];
+	} s;
+	union {
+		struct snd_pcm_mmap_control control;
+		unsigned char reserved[64];
+	} c;
+};
+
 #define SNDRV_PCM_IOCTL_STATUS32	_IOR('A', 0x20, struct snd_pcm_status32)
 #define SNDRV_PCM_IOCTL_STATUS_EXT32	_IOWR('A', 0x24, struct snd_pcm_status32)
+#define SNDRV_PCM_IOCTL_SYNC_PTR32	_IOWR('A', 0x23, struct snd_pcm_sync_ptr32)
 #endif
 
 #endif /* __SOUND_PCM_H */
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 2d990d9..ca8a7df 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -1001,7 +1001,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 	if (runtime == NULL)
 		return -ENOMEM;
 
-	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status));
+	size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64));
 	runtime->status = snd_malloc_pages(size, GFP_KERNEL);
 	if (runtime->status == NULL) {
 		kfree(runtime);
@@ -1013,7 +1013,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream,
 	runtime->control = snd_malloc_pages(size, GFP_KERNEL);
 	if (runtime->control == NULL) {
 		snd_free_pages((void*)runtime->status,
-			       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
+			       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64)));
 		kfree(runtime);
 		return -ENOMEM;
 	}
@@ -1044,7 +1044,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream)
 	if (runtime->private_free != NULL)
 		runtime->private_free(runtime);
 	snd_free_pages((void*)runtime->status,
-		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)));
+		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64)));
 	snd_free_pages((void*)runtime->control,
 		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control)));
 	kfree(runtime->hw_constraints.rules);
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 79e7475..7d690b2 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -570,7 +570,7 @@ static int snd_pcm_ioctl_xfern_compat(struct snd_pcm_substream *substream,
 }
 
 
-struct snd_pcm_mmap_status32 {
+struct compat_snd_pcm_mmap_status32 {
 	s32 state;
 	s32 pad1;
 	u32 hw_ptr;
@@ -579,32 +579,33 @@ struct snd_pcm_mmap_status32 {
 	struct compat_timespec audio_tstamp;
 } __attribute__((packed));
 
-struct snd_pcm_mmap_control32 {
+struct compat_snd_pcm_mmap_control32 {
 	u32 appl_ptr;
 	u32 avail_min;
 };
 
-struct snd_pcm_sync_ptr32 {
+struct compat_snd_pcm_sync_ptr32 {
 	u32 flags;
 	union {
-		struct snd_pcm_mmap_status32 status;
+		struct compat_snd_pcm_mmap_status32 status;
 		unsigned char reserved[64];
 	} s;
 	union {
-		struct snd_pcm_mmap_control32 control;
+		struct compat_snd_pcm_mmap_control32 control;
 		unsigned char reserved[64];
 	} c;
 } __attribute__((packed));
 
 static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream,
-					 struct snd_pcm_sync_ptr32 __user *src)
+					 struct compat_snd_pcm_sync_ptr32 __user *src)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	volatile struct snd_pcm_mmap_status *status;
+	volatile struct snd_pcm_mmap_status64 *status;
 	volatile struct snd_pcm_mmap_control *control;
 	u32 sflags;
 	struct snd_pcm_mmap_control scontrol;
-	struct snd_pcm_mmap_status sstatus;
+	struct snd_pcm_mmap_status64 sstatus;
+	struct compat_snd_pcm_sync_ptr32 sync_ptr;
 	snd_pcm_uframes_t boundary;
 	int err;
 
@@ -637,63 +638,78 @@ static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream,
 		scontrol.avail_min = control->avail_min;
 	sstatus.state = status->state;
 	sstatus.hw_ptr = status->hw_ptr % boundary;
-	sstatus.tstamp = status->tstamp;
+	sstatus.tstamp.tv_sec = status->tstamp.tv_sec;
+	sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec;
 	sstatus.suspended_state = status->suspended_state;
-	sstatus.audio_tstamp = status->audio_tstamp;
+	sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec;
+	sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec;
 	snd_pcm_stream_unlock_irq(substream);
-	if (put_user(sstatus.state, &src->s.status.state) ||
-	    put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) ||
-	    compat_put_timespec(&sstatus.tstamp, &src->s.status.tstamp) ||
-	    put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
-	    compat_put_timespec(&sstatus.audio_tstamp,
-		    &src->s.status.audio_tstamp) ||
-	    put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
-	    put_user(scontrol.avail_min, &src->c.control.avail_min))
+
+	memset(&sync_ptr, 0, sizeof(sync_ptr));
+	sync_ptr.s.status = (struct compat_snd_pcm_mmap_status32) {
+		.state = sstatus.state,
+		.hw_ptr = sstatus.hw_ptr,
+		.tstamp = {
+			.tv_sec = sstatus.tstamp.tv_sec,
+			.tv_nsec = sstatus.tstamp.tv_nsec,
+		},
+		.suspended_state = sstatus.suspended_state,
+		.audio_tstamp = {
+			.tv_sec = sstatus.audio_tstamp.tv_sec,
+			.tv_nsec = sstatus.audio_tstamp.tv_nsec,
+		},
+	};
+
+	sync_ptr.c.control = (struct compat_snd_pcm_mmap_control32) {
+		.appl_ptr = scontrol.appl_ptr,
+		.avail_min = scontrol.avail_min,
+	};
+
+	if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr)))
 		return -EFAULT;
 
 	return 0;
 }
 
-#ifdef CONFIG_X86_X32
-/* X32 ABI has 64bit timespec and 64bit alignment */
-struct snd_pcm_mmap_status_x32 {
+struct compat_snd_pcm_mmap_status64 {
 	s32 state;
 	s32 pad1;
 	u32 hw_ptr;
 	u32 pad2; /* alignment */
-	struct timespec tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
 	s32 suspended_state;
 	s32 pad3;
-	struct timespec audio_tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;
 } __packed;
 
-struct snd_pcm_mmap_control_x32 {
+struct compat_snd_pcm_mmap_control64 {
 	u32 appl_ptr;
 	u32 avail_min;
 };
 
-struct snd_pcm_sync_ptr_x32 {
+struct compat_snd_pcm_sync_ptr64 {
 	u32 flags;
 	u32 rsvd; /* alignment */
 	union {
-		struct snd_pcm_mmap_status_x32 status;
+		struct compat_snd_pcm_mmap_status64 status;
 		unsigned char reserved[64];
 	} s;
 	union {
-		struct snd_pcm_mmap_control_x32 control;
+		struct compat_snd_pcm_mmap_control64 control;
 		unsigned char reserved[64];
 	} c;
 } __packed;
 
-static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
-				      struct snd_pcm_sync_ptr_x32 __user *src)
+static int snd_pcm_ioctl_sync_ptr_compat64(struct snd_pcm_substream *substream,
+				struct compat_snd_pcm_sync_ptr64 __user *src)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	volatile struct snd_pcm_mmap_status *status;
+	volatile struct snd_pcm_mmap_status64 *status;
 	volatile struct snd_pcm_mmap_control *control;
 	u32 sflags;
 	struct snd_pcm_mmap_control scontrol;
-	struct snd_pcm_mmap_status sstatus;
+	struct snd_pcm_mmap_status64 sstatus;
+	struct compat_snd_pcm_sync_ptr64 sync_ptr;
 	snd_pcm_uframes_t boundary;
 	int err;
 
@@ -726,22 +742,142 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
 		scontrol.avail_min = control->avail_min;
 	sstatus.state = status->state;
 	sstatus.hw_ptr = status->hw_ptr % boundary;
-	sstatus.tstamp = status->tstamp;
+	sstatus.tstamp.tv_sec = status->tstamp.tv_sec;
+	sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec;
 	sstatus.suspended_state = status->suspended_state;
-	sstatus.audio_tstamp = status->audio_tstamp;
+	sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec;
+	sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec;
 	snd_pcm_stream_unlock_irq(substream);
-	if (put_user(sstatus.state, &src->s.status.state) ||
-	    put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) ||
-	    put_timespec(&sstatus.tstamp, &src->s.status.tstamp) ||
-	    put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
-	    put_timespec(&sstatus.audio_tstamp, &src->s.status.audio_tstamp) ||
-	    put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
-	    put_user(scontrol.avail_min, &src->c.control.avail_min))
+
+	memset(&sync_ptr, 0, sizeof(sync_ptr));
+	sync_ptr.s.status = (struct compat_snd_pcm_mmap_status64) {
+		.state = sstatus.state,
+		.hw_ptr = sstatus.hw_ptr,
+		.tstamp = {
+			.tv_sec = sstatus.tstamp.tv_sec,
+			.tv_nsec = sstatus.tstamp.tv_nsec,
+		},
+		.suspended_state = sstatus.suspended_state,
+		.audio_tstamp = {
+			.tv_sec = sstatus.audio_tstamp.tv_sec,
+			.tv_nsec = sstatus.audio_tstamp.tv_nsec,
+		},
+	};
+
+	sync_ptr.c.control = (struct compat_snd_pcm_mmap_control64) {
+		.appl_ptr = scontrol.appl_ptr,
+		.avail_min = scontrol.avail_min,
+	};
+
+	if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr)))
 		return -EFAULT;
 
 	return 0;
 }
-#endif /* CONFIG_X86_X32 */
+
+#ifdef IA32_EMULATION
+struct compat_snd_pcm_mmap_status64_x86_32 {
+	s32 state;
+	s32 pad1;
+	u32 hw_ptr;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	s32 suspended_state;
+	struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;
+} __packed;
+
+struct compat_snd_pcm_mmap_control64_x86_32 {
+	u32 appl_ptr;
+	u32 avail_min;
+};
+
+struct compat_snd_pcm_sync_ptr64_x86_32 {
+	u32 flags;
+	union {
+		struct compat_snd_pcm_mmap_status64_x86_32 status;
+		unsigned char reserved[64];
+	} s;
+	union {
+		struct compat_snd_pcm_mmap_control64_x86_32 control;
+		unsigned char reserved[64];
+	} c;
+} __packed;
+
+static int
+snd_pcm_ioctl_sync_ptr_compat64_x86_32(struct snd_pcm_substream *substream,
+			struct compat_snd_pcm_sync_ptr64_x86_32 __user *src)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	volatile struct snd_pcm_mmap_status64 *status;
+	volatile struct snd_pcm_mmap_control *control;
+	u32 sflags;
+	struct snd_pcm_mmap_control scontrol;
+	struct snd_pcm_mmap_status64 sstatus;
+	struct compat_snd_pcm_sync_ptr64_x86_32 sync_ptr;
+	snd_pcm_uframes_t boundary;
+	int err;
+
+	if (snd_BUG_ON(!runtime))
+		return -EINVAL;
+
+	if (get_user(sflags, &src->flags) ||
+	    get_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
+	    get_user(scontrol.avail_min, &src->c.control.avail_min))
+		return -EFAULT;
+	if (sflags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
+		err = snd_pcm_hwsync(substream);
+		if (err < 0)
+			return err;
+	}
+	status = runtime->status;
+	control = runtime->control;
+	boundary = recalculate_boundary(runtime);
+	if (!boundary)
+		boundary = 0x7fffffff;
+	snd_pcm_stream_lock_irq(substream);
+	/* FIXME: we should consider the boundary for the sync from app */
+	if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL))
+		control->appl_ptr = scontrol.appl_ptr;
+	else
+		scontrol.appl_ptr = control->appl_ptr % boundary;
+	if (!(sflags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
+		control->avail_min = scontrol.avail_min;
+	else
+		scontrol.avail_min = control->avail_min;
+	sstatus.state = status->state;
+	sstatus.hw_ptr = status->hw_ptr % boundary;
+	sstatus.tstamp.tv_sec = status->tstamp.tv_sec;
+	sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec;
+	sstatus.suspended_state = status->suspended_state;
+	sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec;
+	sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec;
+	snd_pcm_stream_unlock_irq(substream);
+
+	memset(&sync_ptr, 0, sizeof(sync_ptr));
+	sync_ptr.s.status = (struct compat_snd_pcm_mmap_status64_x86_32) {
+		.state = sstatus.state,
+		.hw_ptr = sstatus.hw_ptr,
+		.tstamp = {
+			.tv_sec = sstatus.tstamp.tv_sec,
+			.tv_nsec = sstatus.tstamp.tv_nsec,
+		},
+		.suspended_state = sstatus.suspended_state,
+		.audio_tstamp = {
+			.tv_sec = sstatus.audio_tstamp.tv_sec,
+			.tv_nsec = sstatus.audio_tstamp.tv_nsec,
+		},
+	};
+
+	sync_ptr.c.control = (struct compat_snd_pcm_mmap_control64_x86_32) {
+		.appl_ptr = scontrol.appl_ptr,
+		.avail_min = scontrol.avail_min,
+	};
+
+	if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr)))
+		return -EFAULT;
+
+	return 0;
+}
+#endif
 
 /*
  */
@@ -759,12 +895,14 @@ enum {
 	SNDRV_PCM_IOCTL_READI_FRAMES32 = _IOR('A', 0x51, struct snd_xferi32),
 	SNDRV_PCM_IOCTL_WRITEN_FRAMES32 = _IOW('A', 0x52, struct snd_xfern32),
 	SNDRV_PCM_IOCTL_READN_FRAMES32 = _IOR('A', 0x53, struct snd_xfern32),
-	SNDRV_PCM_IOCTL_SYNC_PTR32 = _IOWR('A', 0x23, struct snd_pcm_sync_ptr32),
+	SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT32 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr32),
+	SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr64),
 	SNDRV_PCM_IOCTL_STATUS_COMPAT64 = _IOR('A', 0x20, struct compat_snd_pcm_status64),
 	SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64 = _IOWR('A', 0x24, struct compat_snd_pcm_status64),
 #ifdef IA32_EMULATION
 	SNDRV_PCM_IOCTL_STATUS_COMPAT64_X86_32 = _IOR('A', 0x20, struct compat_snd_pcm_status64_x86_32),
 	SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32 = _IOWR('A', 0x24, struct compat_snd_pcm_status64_x86_32),
+	SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64_X86_32 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr64_x86_32),
 #endif
 #ifdef CONFIG_X86_X32
 	SNDRV_PCM_IOCTL_CHANNEL_INFO_X32 = _IOR('A', 0x32, struct snd_pcm_channel_info),
@@ -821,8 +959,10 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 		return snd_pcm_status_user_compat(substream, argp, false);
 	case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32:
 		return snd_pcm_status_user_compat(substream, argp, true);
-	case SNDRV_PCM_IOCTL_SYNC_PTR32:
+	case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT32:
 		return snd_pcm_ioctl_sync_ptr_compat(substream, argp);
+	case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64:
+		return snd_pcm_ioctl_sync_ptr_compat64(substream, argp);
 	case SNDRV_PCM_IOCTL_CHANNEL_INFO32:
 		return snd_pcm_ioctl_channel_info_compat(substream, argp);
 	case SNDRV_PCM_IOCTL_WRITEI_FRAMES32:
@@ -848,6 +988,8 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
 		return snd_pcm_status_user_compat64_x86_32(substream, argp, false);
 	case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32:
 		return snd_pcm_status_user_compat64_x86_32(substream, argp, true);
+	case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64_X86_32:
+		return snd_pcm_ioctl_sync_ptr_compat64_x86_32(substream, argp);
 #endif
 #ifdef CONFIG_X86_X32
 	case SNDRV_PCM_IOCTL_SYNC_PTR_X32:
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 5ca9dc3..96047cc 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -162,7 +162,8 @@ static void xrun(struct snd_pcm_substream *substream)
 		struct timespec64 tstamp;
 
 		snd_pcm_gettime(runtime, &tstamp);
-		runtime->status->tstamp = timespec64_to_timespec(tstamp);
+		runtime->status->tstamp.tv_sec = tstamp.tv_sec;
+		runtime->status->tstamp.tv_nsec = tstamp.tv_nsec;
 	}
 	snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);
 	if (xrun_debug(substream, XRUN_DEBUG_BASIC)) {
@@ -252,8 +253,10 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream,
 				runtime->rate);
 		*audio_tstamp = ns_to_timespec64(audio_nsecs);
 	}
-	runtime->status->audio_tstamp = timespec64_to_timespec(*audio_tstamp);
-	runtime->status->tstamp = timespec64_to_timespec(*curr_tstamp);
+	runtime->status->audio_tstamp.tv_sec = audio_tstamp->tv_sec;
+	runtime->status->audio_tstamp.tv_nsec = audio_tstamp->tv_nsec;
+	runtime->status->tstamp.tv_sec = curr_tstamp->tv_sec;
+	runtime->status->tstamp.tv_nsec = curr_tstamp->tv_nsec;
 
 	/*
 	 * re-take a driver timestamp to let apps detect if the reference tstamp
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7f1f60d..287f20a 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2774,50 +2774,113 @@ static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream)
 	snd_pcm_stream_unlock_irq(substream);
 	return err < 0 ? err : n;
 }
-		
+
 static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
-			    struct snd_pcm_sync_ptr __user *_sync_ptr)
+			    struct snd_pcm_sync_ptr64 *sync_ptr)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct snd_pcm_sync_ptr sync_ptr;
-	volatile struct snd_pcm_mmap_status *status;
+	volatile struct snd_pcm_mmap_status64 *status;
 	volatile struct snd_pcm_mmap_control *control;
 	int err;
 
-	memset(&sync_ptr, 0, sizeof(sync_ptr));
-	if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags)))
-		return -EFAULT;
-	if (copy_from_user(&sync_ptr.c.control, &(_sync_ptr->c.control), sizeof(struct snd_pcm_mmap_control)))
-		return -EFAULT;	
 	status = runtime->status;
 	control = runtime->control;
-	if (sync_ptr.flags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
+	if (sync_ptr->flags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
 		err = snd_pcm_hwsync(substream);
 		if (err < 0)
 			return err;
 	}
 	snd_pcm_stream_lock_irq(substream);
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+	if (!(sync_ptr->flags & SNDRV_PCM_SYNC_PTR_APPL)) {
 		err = pcm_lib_apply_appl_ptr(substream,
-					     sync_ptr.c.control.appl_ptr);
+					     sync_ptr->c.control.appl_ptr);
 		if (err < 0) {
 			snd_pcm_stream_unlock_irq(substream);
 			return err;
 		}
 	} else {
-		sync_ptr.c.control.appl_ptr = control->appl_ptr;
+		sync_ptr->c.control.appl_ptr = control->appl_ptr;
 	}
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
-		control->avail_min = sync_ptr.c.control.avail_min;
+	if (!(sync_ptr->flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
+		control->avail_min = sync_ptr->c.control.avail_min;
 	else
-		sync_ptr.c.control.avail_min = control->avail_min;
-	sync_ptr.s.status.state = status->state;
-	sync_ptr.s.status.hw_ptr = status->hw_ptr;
-	sync_ptr.s.status.tstamp = status->tstamp;
-	sync_ptr.s.status.suspended_state = status->suspended_state;
+		sync_ptr->c.control.avail_min = control->avail_min;
+	sync_ptr->s.status.state = status->state;
+	sync_ptr->s.status.hw_ptr = status->hw_ptr;
+	sync_ptr->s.status.tstamp.tv_sec = status->tstamp.tv_sec;
+	sync_ptr->s.status.tstamp.tv_nsec = status->tstamp.tv_nsec;
+	sync_ptr->s.status.suspended_state = status->suspended_state;
 	snd_pcm_stream_unlock_irq(substream);
+
+	return 0;
+}
+
+#if __BITS_PER_LONG == 32
+static int snd_pcm_sync_ptr32(struct snd_pcm_substream *substream,
+			      struct snd_pcm_sync_ptr32 __user *_sync_ptr)
+{
+	struct snd_pcm_sync_ptr64 sync_ptr64;
+	struct snd_pcm_sync_ptr32 sync_ptr32;
+	int ret;
+
+	memset(&sync_ptr64, 0, sizeof(sync_ptr64));
+	memset(&sync_ptr32, 0, sizeof(sync_ptr32));
+	if (get_user(sync_ptr64.flags, (unsigned __user *)&(_sync_ptr->flags)))
+		return -EFAULT;
+	if (copy_from_user(&sync_ptr64.c.control, &(_sync_ptr->c.control),
+			   sizeof(struct snd_pcm_mmap_control)))
+		return -EFAULT;
+
+	ret = snd_pcm_sync_ptr(substream, &sync_ptr64);
+	if (ret)
+		return ret;
+
+	sync_ptr32.s.status = (struct snd_pcm_mmap_status32) {
+		.state = sync_ptr64.s.status.state,
+		.hw_ptr = sync_ptr64.s.status.hw_ptr,
+		.tstamp = {
+			.tv_sec = sync_ptr64.s.status.tstamp.tv_sec,
+			.tv_nsec = sync_ptr64.s.status.tstamp.tv_nsec,
+		},
+		.suspended_state = sync_ptr64.s.status.suspended_state,
+		.audio_tstamp = {
+			.tv_sec = sync_ptr64.s.status.audio_tstamp.tv_sec,
+			.tv_nsec = sync_ptr64.s.status.audio_tstamp.tv_nsec,
+		},
+	};
+
+	sync_ptr32.c.control = (struct snd_pcm_mmap_control) {
+		.appl_ptr = sync_ptr64.c.control.appl_ptr,
+		.avail_min = sync_ptr64.c.control.avail_min,
+	};
+
+	if (copy_to_user(_sync_ptr, &sync_ptr32, sizeof(sync_ptr32)))
+		return -EFAULT;
+
+	return 0;
+}
+#endif
+
+static int snd_pcm_sync_ptr64(struct snd_pcm_substream *substream,
+			      struct snd_pcm_sync_ptr64 __user *_sync_ptr)
+{
+	struct snd_pcm_sync_ptr64 sync_ptr;
+	int ret;
+
+	memset(&sync_ptr, 0, sizeof(sync_ptr));
+	if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags)))
+		return -EFAULT;
+	if (copy_from_user(&sync_ptr.c.control, &(_sync_ptr->c.control),
+			   sizeof(struct snd_pcm_mmap_control)))
+		return -EFAULT;
+
+	ret = snd_pcm_sync_ptr(substream, &sync_ptr);
+	if (ret)
+		return ret;
+
 	if (copy_to_user(_sync_ptr, &sync_ptr, sizeof(sync_ptr)))
 		return -EFAULT;
+
 	return 0;
 }
 
@@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file,
 			return -EFAULT;
 		return 0;
 	}
-	case SNDRV_PCM_IOCTL_SYNC_PTR:
-		return snd_pcm_sync_ptr(substream, arg);
+#if __BITS_PER_LONG == 32
+	case SNDRV_PCM_IOCTL_SYNC_PTR32:
+		return snd_pcm_sync_ptr32(substream, arg);
+#endif
+	case SNDRV_PCM_IOCTL_SYNC_PTR64:
+		return snd_pcm_sync_ptr64(substream, arg);
 #ifdef CONFIG_SND_SUPPORT_OLD_API
 	case SNDRV_PCM_IOCTL_HW_REFINE_OLD:
 		return snd_pcm_hw_refine_old_user(substream, arg);
@@ -3329,7 +3396,7 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file
 	if (!(area->vm_flags & VM_READ))
 		return -EINVAL;
 	size = area->vm_end - area->vm_start;
-	if (size != PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)))
+	if (size != PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64)))
 		return -EINVAL;
 	area->vm_ops = &snd_pcm_vm_ops_status;
 	area->vm_private_data = substream;
-- 
1.7.9.5

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
> timestamp, which is not year 2038 safe on 32bits system.
>
> Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64'
> to handle 32bit time_t and 64bit time_t in native mode, which replace
> timespec with s64 type.
>
> In compat mode, we renamed or introduced new structures to handle 32bit/64bit
> time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and
> snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode.
> 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used
> to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32'
> and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with
> 32bit alignment.
>
> When glibc changes time_t to 64bit, any recompiled program will issue ioctl
> commands that the kernel does not understand without this patch.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  include/sound/pcm.h     |   46 +++++++++-
>  sound/core/pcm.c        |    6 +-
>  sound/core/pcm_compat.c |  228 ++++++++++++++++++++++++++++++++++++++---------
>  sound/core/pcm_lib.c    |    9 +-
>  sound/core/pcm_native.c |  113 ++++++++++++++++++-----
>  5 files changed, 329 insertions(+), 73 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 114cc29..c253cbf 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -64,6 +64,15 @@ struct snd_pcm_hardware {
>  struct snd_pcm_audio_tstamp_config; /* definitions further down */
>  struct snd_pcm_audio_tstamp_report;
>
> +struct snd_pcm_mmap_status64 {
> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
> +       int pad1;                       /* Needed for 64 bit alignment */
> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> +       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> +       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
> +};

This looks correct, but there is a subtlety here to note about x86-32
that we discussed in a previous (private) review. To recall my earlier
thoughts:

Normal architectures insert 32 bit padding after 'suspended_state',
and 32-bit architectures (including x32) also after hw_ptr,
but x86-32 does not. You make that explicit in the compat code,
this version just relies on the compiler using identical padding
in user and kernel space. We could make that explicit using

struct snd_pcm_mmap_status64 {
       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
       int pad1;                       /* Needed for 64 bit alignment */
       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
#if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
       int pad2;
#endif
      struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
#if !defined(CONFIG_X86_32)
       int pad3;
#endif
      struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
sample counter or wall clock */
};

To make it clear that the layout is architecture specific. As a third
alternative, we could define binary version of the structure explicitly,
and have handlers for each layout, then call the correct handlers for
both native and compat mode. This could use e.g.

struct snd_pcm_mmap_status_time32 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       s32 tstamp_sec;
       s32 tstamp_usec;
       u32 suspended_state;
       s32 audio_tstamp_sec;
       s32 audio_tstamp_usec;
};

struct snd_pcm_mmap_status_time64 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       u32 pad2;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       u32 pad3;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
};

struct snd_pcm_mmap_status_time64_i386 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
} __packed __aligned(4);

struct snd_pcm_mmap_status_64bit {
       u32 state;
       u32 pad1;
       u64 hw_ptr;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       u32 pad3;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
};

My personal preference would be the third approach, but I'd like
to hear from Takashi if he has a preference. The downside of that
is that it requires the most changes to the existing code.

> @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 {
>         unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */
>  };
>
> +struct snd_pcm_sync_ptr64 {
> +       unsigned int flags;
> +       union {
> +               struct snd_pcm_mmap_status64 status;
> +               unsigned char reserved[64];
> +       } s;
> +       union {
> +               struct snd_pcm_mmap_control control;
> +               unsigned char reserved[64];
> +       } c;
> +};
> +
>  #define SNDRV_PCM_IOCTL_STATUS64       _IOR('A', 0x20, struct snd_pcm_status64)
>  #define SNDRV_PCM_IOCTL_STATUS_EXT64   _IOWR('A', 0x24, struct snd_pcm_status64)
> +#define SNDRV_PCM_IOCTL_SYNC_PTR64     _IOWR('A', 0x23, struct snd_pcm_sync_ptr64)
>
>  #if __BITS_PER_LONG == 32
>  struct snd_pcm_status32 {
> @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 {
>         unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */
>  };
>
> +struct snd_pcm_mmap_status32 {
> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
> +       int pad1;                       /* Needed for 64 bit alignment */
> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> +       struct { s32 tv_sec; s32 tv_nsec; } tstamp;             /* Timestamp */
> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> +       struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
> +};
> +
> +struct snd_pcm_sync_ptr32 {
> +       unsigned int flags;
> +       union {
> +               struct snd_pcm_mmap_status32 status;
> +               unsigned char reserved[64];
> +       } s;
> +       union {
> +               struct snd_pcm_mmap_control control;
> +               unsigned char reserved[64];
> +       } c;
> +};
> +
>  #define SNDRV_PCM_IOCTL_STATUS32       _IOR('A', 0x20, struct snd_pcm_status32)
>  #define SNDRV_PCM_IOCTL_STATUS_EXT32   _IOWR('A', 0x24, struct snd_pcm_status32)
> +#define SNDRV_PCM_IOCTL_SYNC_PTR32     _IOWR('A', 0x23, struct snd_pcm_sync_ptr32)
>  #endif

Unfortunately, this approach doesn't quite work in this particular
case: it depends on snd_pcm_sync_ptr32 and snd_pcm_sync_ptr64
having different sizes in order to result in distinct command codes
for SNDRV_PCM_IOCTL_SYNC_PTR64 and
SNDRV_PCM_IOCTL_SYNC_PTR32.

However, the 64-byte 'reserved' fields mean that the unions are always
the same size, and only the padding between 'flags' and 's' is different.

Again, I suppose this almost works: 64-bit architectures use only
one possible layout in the .unlocked_ioctl handler, and on most
32-bit architectures the added padding will make the structure 4 bytes
longer, but not on i386, which now doesn't know whether user
space passed a snd_pcm_sync_ptr32  or a snd_pcm_sync_ptr64
structure.

Fixing this will require changing the uapi header file, in one of two
ways:

a) make the command number conditional:

#if __BITS_PER_LONG == 64 || !defined(__i386__)
#define SNDRV_PCM_IOCTL_SYNC_PTR SNDRV_PCM_IOCTL_SYNC_PTR_OLD
#else
#define SNDRV_PCM_IOCTL_SYNC_PTR (sizeof(__kernel_long_t) == sizeof(time_t) \
     ? SNDRV_PCM_IOCTL_SYNC_PTR_OLD
     :  SNDRV_PCM_IOCTL_SYNC_PTR_64)
#endif

b) change the user-visible structure definition to contain the
    correct explicit padding on all architectures including i386

 struct snd_pcm_mmap_status {
        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
        int pad1;                       /* Needed for 64 bit alignment */
        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
+      char pad2[sizeof(time_t) - sizeof(snd_pcm_uframes_t)];
        struct timespec tstamp;         /* Timestamp */
        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
+      char pad2[sizeof(time_t) - sizeof(snd_pcm_state_t)];
        struct timespec audio_tstamp;   /* from sample counter or wall clock */
 };

 struct snd_pcm_mmap_control {
        snd_pcm_uframes_t appl_ptr;     /* RW: appl ptr (0...boundary-1) */
        snd_pcm_uframes_t avail_min;    /* RW: min available frames
for wakeup */
 };

 struct snd_pcm_sync_ptr32 {
       unsigned int flags;
+     char __pad[sizeof(time_t) - sizeof(unsigned int)];
       union {
               struct snd_pcm_mmap_status status;
               unsigned char reserved[64];
       } s;
       union {
               struct snd_pcm_mmap_control control;
               unsigned char reserved[64];
       } c;
 };

> @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file,
>                         return -EFAULT;
>                 return 0;
>         }
> -       case SNDRV_PCM_IOCTL_SYNC_PTR:
> -               return snd_pcm_sync_ptr(substream, arg);
> +#if __BITS_PER_LONG == 32
> +       case SNDRV_PCM_IOCTL_SYNC_PTR32:
> +               return snd_pcm_sync_ptr32(substream, arg);
> +#endif
> +       case SNDRV_PCM_IOCTL_SYNC_PTR64:
> +               return snd_pcm_sync_ptr64(substream, arg);
>  #ifdef CONFIG_SND_SUPPORT_OLD_API
>         case SNDRV_PCM_IOCTL_HW_REFINE_OLD:
>                 return snd_pcm_hw_refine_old_user(substream, arg);

Without either of the two changes, this function should cause
a build error on i386 since SNDRV_PCM_IOCTL_SYNC_PTR32
has the same value as SNDRV_PCM_IOCTL_SYNC_PTR64.

        Arnd

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
> timestamp, which is not year 2038 safe on 32bits system.
>
> Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64'
> to handle 32bit time_t and 64bit time_t in native mode, which replace
> timespec with s64 type.
>
> In compat mode, we renamed or introduced new structures to handle 32bit/64bit
> time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and
> snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode.
> 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used
> to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32'
> and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with
> 32bit alignment.
>
> When glibc changes time_t to 64bit, any recompiled program will issue ioctl
> commands that the kernel does not understand without this patch.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  include/sound/pcm.h     |   46 +++++++++-
>  sound/core/pcm.c        |    6 +-
>  sound/core/pcm_compat.c |  228 ++++++++++++++++++++++++++++++++++++++---------
>  sound/core/pcm_lib.c    |    9 +-
>  sound/core/pcm_native.c |  113 ++++++++++++++++++-----
>  5 files changed, 329 insertions(+), 73 deletions(-)
>
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 114cc29..c253cbf 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -64,6 +64,15 @@ struct snd_pcm_hardware {
>  struct snd_pcm_audio_tstamp_config; /* definitions further down */
>  struct snd_pcm_audio_tstamp_report;
>
> +struct snd_pcm_mmap_status64 {
> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
> +       int pad1;                       /* Needed for 64 bit alignment */
> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> +       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> +       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
> +};

This looks correct, but there is a subtlety here to note about x86-32
that we discussed in a previous (private) review. To recall my earlier
thoughts:

Normal architectures insert 32 bit padding after 'suspended_state',
and 32-bit architectures (including x32) also after hw_ptr,
but x86-32 does not. You make that explicit in the compat code,
this version just relies on the compiler using identical padding
in user and kernel space. We could make that explicit using

struct snd_pcm_mmap_status64 {
       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
       int pad1;                       /* Needed for 64 bit alignment */
       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
#if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
       int pad2;
#endif
      struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
#if !defined(CONFIG_X86_32)
       int pad3;
#endif
      struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
sample counter or wall clock */
};

To make it clear that the layout is architecture specific. As a third
alternative, we could define binary version of the structure explicitly,
and have handlers for each layout, then call the correct handlers for
both native and compat mode. This could use e.g.

struct snd_pcm_mmap_status_time32 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       s32 tstamp_sec;
       s32 tstamp_usec;
       u32 suspended_state;
       s32 audio_tstamp_sec;
       s32 audio_tstamp_usec;
};

struct snd_pcm_mmap_status_time64 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       u32 pad2;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       u32 pad3;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
};

struct snd_pcm_mmap_status_time64_i386 {
       u32 state;
       u32 pad1;
       u32 hw_ptr;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
} __packed __aligned(4);

struct snd_pcm_mmap_status_64bit {
       u32 state;
       u32 pad1;
       u64 hw_ptr;
       s64 tstamp_sec;
       s64 tstamp_usec;
       u32 suspended_state;
       u32 pad3;
       s64 audio_tstamp_sec;
       s64 audio_tstamp_usec;
};

My personal preference would be the third approach, but I'd like
to hear from Takashi if he has a preference. The downside of that
is that it requires the most changes to the existing code.

> @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 {
>         unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */
>  };
>
> +struct snd_pcm_sync_ptr64 {
> +       unsigned int flags;
> +       union {
> +               struct snd_pcm_mmap_status64 status;
> +               unsigned char reserved[64];
> +       } s;
> +       union {
> +               struct snd_pcm_mmap_control control;
> +               unsigned char reserved[64];
> +       } c;
> +};
> +
>  #define SNDRV_PCM_IOCTL_STATUS64       _IOR('A', 0x20, struct snd_pcm_status64)
>  #define SNDRV_PCM_IOCTL_STATUS_EXT64   _IOWR('A', 0x24, struct snd_pcm_status64)
> +#define SNDRV_PCM_IOCTL_SYNC_PTR64     _IOWR('A', 0x23, struct snd_pcm_sync_ptr64)
>
>  #if __BITS_PER_LONG == 32
>  struct snd_pcm_status32 {
> @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 {
>         unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */
>  };
>
> +struct snd_pcm_mmap_status32 {
> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
> +       int pad1;                       /* Needed for 64 bit alignment */
> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> +       struct { s32 tv_sec; s32 tv_nsec; } tstamp;             /* Timestamp */
> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> +       struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
> +};
> +
> +struct snd_pcm_sync_ptr32 {
> +       unsigned int flags;
> +       union {
> +               struct snd_pcm_mmap_status32 status;
> +               unsigned char reserved[64];
> +       } s;
> +       union {
> +               struct snd_pcm_mmap_control control;
> +               unsigned char reserved[64];
> +       } c;
> +};
> +
>  #define SNDRV_PCM_IOCTL_STATUS32       _IOR('A', 0x20, struct snd_pcm_status32)
>  #define SNDRV_PCM_IOCTL_STATUS_EXT32   _IOWR('A', 0x24, struct snd_pcm_status32)
> +#define SNDRV_PCM_IOCTL_SYNC_PTR32     _IOWR('A', 0x23, struct snd_pcm_sync_ptr32)
>  #endif

Unfortunately, this approach doesn't quite work in this particular
case: it depends on snd_pcm_sync_ptr32 and snd_pcm_sync_ptr64
having different sizes in order to result in distinct command codes
for SNDRV_PCM_IOCTL_SYNC_PTR64 and
SNDRV_PCM_IOCTL_SYNC_PTR32.

However, the 64-byte 'reserved' fields mean that the unions are always
the same size, and only the padding between 'flags' and 's' is different.

Again, I suppose this almost works: 64-bit architectures use only
one possible layout in the .unlocked_ioctl handler, and on most
32-bit architectures the added padding will make the structure 4 bytes
longer, but not on i386, which now doesn't know whether user
space passed a snd_pcm_sync_ptr32  or a snd_pcm_sync_ptr64
structure.

Fixing this will require changing the uapi header file, in one of two
ways:

a) make the command number conditional:

#if __BITS_PER_LONG == 64 || !defined(__i386__)
#define SNDRV_PCM_IOCTL_SYNC_PTR SNDRV_PCM_IOCTL_SYNC_PTR_OLD
#else
#define SNDRV_PCM_IOCTL_SYNC_PTR (sizeof(__kernel_long_t) == sizeof(time_t) \
     ? SNDRV_PCM_IOCTL_SYNC_PTR_OLD
     :  SNDRV_PCM_IOCTL_SYNC_PTR_64)
#endif

b) change the user-visible structure definition to contain the
    correct explicit padding on all architectures including i386

 struct snd_pcm_mmap_status {
        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
        int pad1;                       /* Needed for 64 bit alignment */
        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
+      char pad2[sizeof(time_t) - sizeof(snd_pcm_uframes_t)];
        struct timespec tstamp;         /* Timestamp */
        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
+      char pad2[sizeof(time_t) - sizeof(snd_pcm_state_t)];
        struct timespec audio_tstamp;   /* from sample counter or wall clock */
 };

 struct snd_pcm_mmap_control {
        snd_pcm_uframes_t appl_ptr;     /* RW: appl ptr (0...boundary-1) */
        snd_pcm_uframes_t avail_min;    /* RW: min available frames
for wakeup */
 };

 struct snd_pcm_sync_ptr32 {
       unsigned int flags;
+     char __pad[sizeof(time_t) - sizeof(unsigned int)];
       union {
               struct snd_pcm_mmap_status status;
               unsigned char reserved[64];
       } s;
       union {
               struct snd_pcm_mmap_control control;
               unsigned char reserved[64];
       } c;
 };

> @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file,
>                         return -EFAULT;
>                 return 0;
>         }
> -       case SNDRV_PCM_IOCTL_SYNC_PTR:
> -               return snd_pcm_sync_ptr(substream, arg);
> +#if __BITS_PER_LONG == 32
> +       case SNDRV_PCM_IOCTL_SYNC_PTR32:
> +               return snd_pcm_sync_ptr32(substream, arg);
> +#endif
> +       case SNDRV_PCM_IOCTL_SYNC_PTR64:
> +               return snd_pcm_sync_ptr64(substream, arg);
>  #ifdef CONFIG_SND_SUPPORT_OLD_API
>         case SNDRV_PCM_IOCTL_HW_REFINE_OLD:
>                 return snd_pcm_hw_refine_old_user(substream, arg);

Without either of the two changes, this function should cause
a build error on i386 since SNDRV_PCM_IOCTL_SYNC_PTR32
has the same value as SNDRV_PCM_IOCTL_SYNC_PTR64.

        Arnd

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Baolin Wang]

On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>> timestamp, which is not year 2038 safe on 32bits system.
>>
>> Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64'
>> to handle 32bit time_t and 64bit time_t in native mode, which replace
>> timespec with s64 type.
>>
>> In compat mode, we renamed or introduced new structures to handle 32bit/64bit
>> time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and
>> snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode.
>> 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used
>> to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32'
>> and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with
>> 32bit alignment.
>>
>> When glibc changes time_t to 64bit, any recompiled program will issue ioctl
>> commands that the kernel does not understand without this patch.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  include/sound/pcm.h     |   46 +++++++++-
>>  sound/core/pcm.c        |    6 +-
>>  sound/core/pcm_compat.c |  228 ++++++++++++++++++++++++++++++++++++++---------
>>  sound/core/pcm_lib.c    |    9 +-
>>  sound/core/pcm_native.c |  113 ++++++++++++++++++-----
>>  5 files changed, 329 insertions(+), 73 deletions(-)
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index 114cc29..c253cbf 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -64,6 +64,15 @@ struct snd_pcm_hardware {
>>  struct snd_pcm_audio_tstamp_config; /* definitions further down */
>>  struct snd_pcm_audio_tstamp_report;
>>
>> +struct snd_pcm_mmap_status64 {
>> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>> +       int pad1;                       /* Needed for 64 bit alignment */
>> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
>> +       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
>> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>> +       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
>> +};
>
> This looks correct, but there is a subtlety here to note about x86-32
> that we discussed in a previous (private) review. To recall my earlier
> thoughts:
>
> Normal architectures insert 32 bit padding after 'suspended_state',
> and 32-bit architectures (including x32) also after hw_ptr,
> but x86-32 does not. You make that explicit in the compat code,
> this version just relies on the compiler using identical padding
> in user and kernel space. We could make that explicit using
>
> struct snd_pcm_mmap_status64 {
>        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>        int pad1;                       /* Needed for 64 bit alignment */
>        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>        int pad2;
> #endif
>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
>        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> #if !defined(CONFIG_X86_32)
>        int pad3;
> #endif
>       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
> sample counter or wall clock */
> };

I am sorry I did not get you here, why we do not need pad2 and pad3
for x86_32? You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
condition?

>
> To make it clear that the layout is architecture specific. As a third
> alternative, we could define binary version of the structure explicitly,
> and have handlers for each layout, then call the correct handlers for
> both native and compat mode. This could use e.g.
>
> struct snd_pcm_mmap_status_time32 {
>        u32 state;
>        u32 pad1;
>        u32 hw_ptr;
>        s32 tstamp_sec;
>        s32 tstamp_usec;
>        u32 suspended_state;
>        s32 audio_tstamp_sec;
>        s32 audio_tstamp_usec;
> };
>
> struct snd_pcm_mmap_status_time64 {
>        u32 state;
>        u32 pad1;
>        u32 hw_ptr;
>        u32 pad2;
>        s64 tstamp_sec;
>        s64 tstamp_usec;
>        u32 suspended_state;
>        u32 pad3;
>        s64 audio_tstamp_sec;
>        s64 audio_tstamp_usec;
> };
>
> struct snd_pcm_mmap_status_time64_i386 {
>        u32 state;
>        u32 pad1;
>        u32 hw_ptr;
>        s64 tstamp_sec;
>        s64 tstamp_usec;
>        u32 suspended_state;
>        s64 audio_tstamp_sec;
>        s64 audio_tstamp_usec;
> } __packed __aligned(4);
>
> struct snd_pcm_mmap_status_64bit {
>        u32 state;
>        u32 pad1;
>        u64 hw_ptr;
>        s64 tstamp_sec;
>        s64 tstamp_usec;
>        u32 suspended_state;
>        u32 pad3;
>        s64 audio_tstamp_sec;
>        s64 audio_tstamp_usec;
> };
>
> My personal preference would be the third approach, but I'd like
> to hear from Takashi if he has a preference. The downside of that
> is that it requires the most changes to the existing code.

OK.

>
>> @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 {
>>         unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */
>>  };
>>
>> +struct snd_pcm_sync_ptr64 {
>> +       unsigned int flags;
>> +       union {
>> +               struct snd_pcm_mmap_status64 status;
>> +               unsigned char reserved[64];
>> +       } s;
>> +       union {
>> +               struct snd_pcm_mmap_control control;
>> +               unsigned char reserved[64];
>> +       } c;
>> +};
>> +
>>  #define SNDRV_PCM_IOCTL_STATUS64       _IOR('A', 0x20, struct snd_pcm_status64)
>>  #define SNDRV_PCM_IOCTL_STATUS_EXT64   _IOWR('A', 0x24, struct snd_pcm_status64)
>> +#define SNDRV_PCM_IOCTL_SYNC_PTR64     _IOWR('A', 0x23, struct snd_pcm_sync_ptr64)
>>
>>  #if __BITS_PER_LONG == 32
>>  struct snd_pcm_status32 {
>> @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 {
>>         unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */
>>  };
>>
>> +struct snd_pcm_mmap_status32 {
>> +       snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>> +       int pad1;                       /* Needed for 64 bit alignment */
>> +       snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
>> +       struct { s32 tv_sec; s32 tv_nsec; } tstamp;             /* Timestamp */
>> +       snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>> +       struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp;       /* from sample counter or wall clock */
>> +};
>> +
>> +struct snd_pcm_sync_ptr32 {
>> +       unsigned int flags;
>> +       union {
>> +               struct snd_pcm_mmap_status32 status;
>> +               unsigned char reserved[64];
>> +       } s;
>> +       union {
>> +               struct snd_pcm_mmap_control control;
>> +               unsigned char reserved[64];
>> +       } c;
>> +};
>> +
>>  #define SNDRV_PCM_IOCTL_STATUS32       _IOR('A', 0x20, struct snd_pcm_status32)
>>  #define SNDRV_PCM_IOCTL_STATUS_EXT32   _IOWR('A', 0x24, struct snd_pcm_status32)
>> +#define SNDRV_PCM_IOCTL_SYNC_PTR32     _IOWR('A', 0x23, struct snd_pcm_sync_ptr32)
>>  #endif
>
> Unfortunately, this approach doesn't quite work in this particular
> case: it depends on snd_pcm_sync_ptr32 and snd_pcm_sync_ptr64
> having different sizes in order to result in distinct command codes
> for SNDRV_PCM_IOCTL_SYNC_PTR64 and
> SNDRV_PCM_IOCTL_SYNC_PTR32.
>
> However, the 64-byte 'reserved' fields mean that the unions are always
> the same size, and only the padding between 'flags' and 's' is different.

Ah, make sense.

>
> Again, I suppose this almost works: 64-bit architectures use only
> one possible layout in the .unlocked_ioctl handler, and on most
> 32-bit architectures the added padding will make the structure 4 bytes
> longer, but not on i386, which now doesn't know whether user
> space passed a snd_pcm_sync_ptr32  or a snd_pcm_sync_ptr64
> structure.
>
> Fixing this will require changing the uapi header file, in one of two
> ways:
>
> a) make the command number conditional:
>
> #if __BITS_PER_LONG == 64 || !defined(__i386__)
> #define SNDRV_PCM_IOCTL_SYNC_PTR SNDRV_PCM_IOCTL_SYNC_PTR_OLD
> #else
> #define SNDRV_PCM_IOCTL_SYNC_PTR (sizeof(__kernel_long_t) == sizeof(time_t) \
>      ? SNDRV_PCM_IOCTL_SYNC_PTR_OLD
>      :  SNDRV_PCM_IOCTL_SYNC_PTR_64)
> #endif
>
> b) change the user-visible structure definition to contain the
>     correct explicit padding on all architectures including i386
>
>  struct snd_pcm_mmap_status {
>         snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>         int pad1;                       /* Needed for 64 bit alignment */
>         snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
> +      char pad2[sizeof(time_t) - sizeof(snd_pcm_uframes_t)];
>         struct timespec tstamp;         /* Timestamp */
>         snd_pcm_state_t suspended_state; /* RO: suspended stream state */
> +      char pad2[sizeof(time_t) - sizeof(snd_pcm_state_t)];
>         struct timespec audio_tstamp;   /* from sample counter or wall clock */
>  };
>
>  struct snd_pcm_mmap_control {
>         snd_pcm_uframes_t appl_ptr;     /* RW: appl ptr (0...boundary-1) */
>         snd_pcm_uframes_t avail_min;    /* RW: min available frames
> for wakeup */
>  };
>
>  struct snd_pcm_sync_ptr32 {
>        unsigned int flags;
> +     char __pad[sizeof(time_t) - sizeof(unsigned int)];
>        union {
>                struct snd_pcm_mmap_status status;
>                unsigned char reserved[64];
>        } s;
>        union {
>                struct snd_pcm_mmap_control control;
>                unsigned char reserved[64];
>        } c;
>  };

OK. I will check here again.

>
>> @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file,
>>                         return -EFAULT;
>>                 return 0;
>>         }
>> -       case SNDRV_PCM_IOCTL_SYNC_PTR:
>> -               return snd_pcm_sync_ptr(substream, arg);
>> +#if __BITS_PER_LONG == 32
>> +       case SNDRV_PCM_IOCTL_SYNC_PTR32:
>> +               return snd_pcm_sync_ptr32(substream, arg);
>> +#endif
>> +       case SNDRV_PCM_IOCTL_SYNC_PTR64:
>> +               return snd_pcm_sync_ptr64(substream, arg);
>>  #ifdef CONFIG_SND_SUPPORT_OLD_API
>>         case SNDRV_PCM_IOCTL_HW_REFINE_OLD:
>>                 return snd_pcm_hw_refine_old_user(substream, arg);
>
> Without either of the two changes, this function should cause
> a build error on i386 since SNDRV_PCM_IOCTL_SYNC_PTR32
> has the same value as SNDRV_PCM_IOCTL_SYNC_PTR64.
>

Yes. Thanks for your useful comments.


-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>>
>> This looks correct, but there is a subtlety here to note about x86-32
>> that we discussed in a previous (private) review. To recall my earlier
>> thoughts:
>>
>> Normal architectures insert 32 bit padding after 'suspended_state',
>> and 32-bit architectures (including x32) also after hw_ptr,
>> but x86-32 does not. You make that explicit in the compat code,
>> this version just relies on the compiler using identical padding
>> in user and kernel space. We could make that explicit using
>>
>> struct snd_pcm_mmap_status64 {
>>        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>>        int pad1;                       /* Needed for 64 bit alignment */
>>        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>>        int pad2;
>> #endif
>>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
>>        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>> #if !defined(CONFIG_X86_32)
>>        int pad3;
>> #endif
>>       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
>> sample counter or wall clock */
>> };
>
> I am sorry I did not get you here, why we do not need pad2 and pad3
> for x86_32?

This is again the x86-32 alignment quirk: the structure as defined
in the uapi header does not have padding, and the new s64 fields
have 32-bit alignment on x86, so the compiler does not add implicit
padding in user space.

On all other architectures, the fields do get padded implicitly
in user space, I'm just listing the padding explicitly.

> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
> condition?

No, that was intentional:

snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures
we have no padding between two 64-bit values (hw_ptr and tstamp),
and on x86-32 we have no padding because both have 32-bit
alignment.

However, snd_pcm_state_t is 'int', which is always 32-bit wide,
so we do have padding on both 32-bit and 64-bit architectures
between syspended_state and audio_tstamp, with the exception
of x86-32.

      Arnd

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Baolin Wang]

On 22 September 2017 at 16:48, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>>>
>>> This looks correct, but there is a subtlety here to note about x86-32
>>> that we discussed in a previous (private) review. To recall my earlier
>>> thoughts:
>>>
>>> Normal architectures insert 32 bit padding after 'suspended_state',
>>> and 32-bit architectures (including x32) also after hw_ptr,
>>> but x86-32 does not. You make that explicit in the compat code,
>>> this version just relies on the compiler using identical padding
>>> in user and kernel space. We could make that explicit using
>>>
>>> struct snd_pcm_mmap_status64 {
>>>        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>>>        int pad1;                       /* Needed for 64 bit alignment */
>>>        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
>>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>>>        int pad2;
>>> #endif
>>>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
>>>        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>>> #if !defined(CONFIG_X86_32)
>>>        int pad3;
>>> #endif
>>>       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
>>> sample counter or wall clock */
>>> };
>>
>> I am sorry I did not get you here, why we do not need pad2 and pad3
>> for x86_32?
>
> This is again the x86-32 alignment quirk: the structure as defined
> in the uapi header does not have padding, and the new s64 fields
> have 32-bit alignment on x86, so the compiler does not add implicit
> padding in user space.
>
> On all other architectures, the fields do get padded implicitly
> in user space, I'm just listing the padding explicitly.

Make sense.

>
>> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
>> condition?
>
> No, that was intentional:
>
> snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures
> we have no padding between two 64-bit values (hw_ptr and tstamp),
> and on x86-32 we have no padding because both have 32-bit
> alignment.
>
> However, snd_pcm_state_t is 'int', which is always 32-bit wide,
> so we do have padding on both 32-bit and 64-bit architectures
> between syspended_state and audio_tstamp, with the exception
> of x86-32.

Thanks, I can understand now.

-- 
Baolin.wang
Best Regards
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [RFC PATCH 3/7] sound: core: Avoid using timespec for struct snd_pcm_sync_ptr

[Baolin Wang]

On 22 September 2017 at 16:48, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record
>>>
>>> This looks correct, but there is a subtlety here to note about x86-32
>>> that we discussed in a previous (private) review. To recall my earlier
>>> thoughts:
>>>
>>> Normal architectures insert 32 bit padding after 'suspended_state',
>>> and 32-bit architectures (including x32) also after hw_ptr,
>>> but x86-32 does not. You make that explicit in the compat code,
>>> this version just relies on the compiler using identical padding
>>> in user and kernel space. We could make that explicit using
>>>
>>> struct snd_pcm_mmap_status64 {
>>>        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
>>>        int pad1;                       /* Needed for 64 bit alignment */
>>>        snd_pcm_uframes_t hw_ptr;       /* RO: hw ptr (0...boundary-1) */
>>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32)
>>>        int pad2;
>>> #endif
>>>       struct { s64 tv_sec; s64 tv_nsec; } tstamp;             /* Timestamp */
>>>        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
>>> #if !defined(CONFIG_X86_32)
>>>        int pad3;
>>> #endif
>>>       struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp;       /* from
>>> sample counter or wall clock */
>>> };
>>
>> I am sorry I did not get you here, why we do not need pad2 and pad3
>> for x86_32?
>
> This is again the x86-32 alignment quirk: the structure as defined
> in the uapi header does not have padding, and the new s64 fields
> have 32-bit alignment on x86, so the compiler does not add implicit
> padding in user space.
>
> On all other architectures, the fields do get padded implicitly
> in user space, I'm just listing the padding explicitly.

Make sense.

>
>> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if
>> condition?
>
> No, that was intentional:
>
> snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures
> we have no padding between two 64-bit values (hw_ptr and tstamp),
> and on x86-32 we have no padding because both have 32-bit
> alignment.
>
> However, snd_pcm_state_t is 'int', which is always 32-bit wide,
> so we do have padding on both 32-bit and 64-bit architectures
> between syspended_state and audio_tstamp, with the exception
> of x86-32.

Thanks, I can understand now.

-- 
Baolin.wang
Best Regards

[RFC PATCH 4/7] sound: core: Avoid using timespec for struct snd_rawmidi_status

[Baolin Wang]

The struct snd_rawmidi_status will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Thus we introduced 'struct snd_rawmidi_status32' and 'struct snd_rawmidi_status64'
to handle 32bit time_t and 64bit time_t in native mode, which replace
timespec with s64 type.

In compat mode, we renamed or introduced new structures to handle 32bit/64bit
time_t in compatible mode. 'struct compat_snd_rawmidi_status32' and
snd_rawmidi_ioctl_status_compat() are used to handle 32bit time_t in compat mode.
'struct compat_snd_rawmidi_status64' and snd_rawmidi_ioctl_status_compat64() are used
to handle 64bit time_t with 64bit alignment. 'struct compat_snd_rawmidi_status64_x86_32'
and snd_rawmidi_ioctl_status_compat64_x86_32() are used to handle 64bit time_t with
32bit alignment.

When glibc changes time_t to 64-bit, any recompiled program will issue ioctl
commands that the kernel does not understand without this patch.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 sound/core/rawmidi.c        |   74 ++++++++++++++++++++++++++++++++---
 sound/core/rawmidi_compat.c |   90 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 135 insertions(+), 29 deletions(-)

diff --git a/sound/core/rawmidi.c b/sound/core/rawmidi.c
index b3b353d..7afb1a4 100644
--- a/sound/core/rawmidi.c
+++ b/sound/core/rawmidi.c
@@ -63,6 +63,28 @@
 #define rmidi_dbg(rmidi, fmt, args...) \
 	dev_dbg(&(rmidi)->dev, fmt, ##args)
 
+#if __BITS_PER_LONG == 32
+struct snd_rawmidi_status32 {
+	int stream;
+	struct { s32 tv_sec; s32 tv_nsec; } tstamp;		/* Timestamp */
+	size_t avail;			/* available bytes */
+	size_t xruns;			/* count of overruns since last status (in bytes) */
+	unsigned char reserved[16];	/* reserved for future use */
+};
+
+#define SNDRV_RAWMIDI_IOCTL_STATUS32	_IOWR('W', 0x20, struct snd_rawmidi_status32)
+#endif
+
+struct snd_rawmidi_status64 {
+	int stream;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;		/* Timestamp */
+	size_t avail;			/* available bytes */
+	size_t xruns;			/* count of overruns since last status (in bytes) */
+	unsigned char reserved[16];	/* reserved for future use */
+};
+
+#define SNDRV_RAWMIDI_IOCTL_STATUS64	_IOWR('W', 0x20, struct snd_rawmidi_status64)
+
 static struct snd_rawmidi *snd_rawmidi_search(struct snd_card *card, int device)
 {
 	struct snd_rawmidi *rawmidi;
@@ -680,7 +702,7 @@ int snd_rawmidi_input_params(struct snd_rawmidi_substream *substream,
 EXPORT_SYMBOL(snd_rawmidi_input_params);
 
 static int snd_rawmidi_output_status(struct snd_rawmidi_substream *substream,
-				     struct snd_rawmidi_status * status)
+				     struct snd_rawmidi_status64 * status)
 {
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 
@@ -693,7 +715,7 @@ static int snd_rawmidi_output_status(struct snd_rawmidi_substream *substream,
 }
 
 static int snd_rawmidi_input_status(struct snd_rawmidi_substream *substream,
-				    struct snd_rawmidi_status * status)
+				    struct snd_rawmidi_status64 * status)
 {
 	struct snd_rawmidi_runtime *runtime = substream->runtime;
 
@@ -751,11 +773,50 @@ static long snd_rawmidi_ioctl(struct file *file, unsigned int cmd, unsigned long
 			return -EINVAL;
 		}
 	}
-	case SNDRV_RAWMIDI_IOCTL_STATUS:
+#if __BITS_PER_LONG == 32
+	case SNDRV_RAWMIDI_IOCTL_STATUS32:
+	{
+		int err = 0;
+		struct snd_rawmidi_status32 __user *status = argp;
+		struct snd_rawmidi_status32 status32;
+		struct snd_rawmidi_status64 status64;
+
+		if (copy_from_user(&status32, argp,
+				   sizeof(struct snd_rawmidi_status32)))
+			return -EFAULT;
+		switch (status32.stream) {
+		case SNDRV_RAWMIDI_STREAM_OUTPUT:
+			if (rfile->output == NULL)
+				return -EINVAL;
+			err = snd_rawmidi_output_status(rfile->output, &status64);
+			break;
+		case SNDRV_RAWMIDI_STREAM_INPUT:
+			if (rfile->input == NULL)
+				return -EINVAL;
+			err = snd_rawmidi_input_status(rfile->input, &status64);
+			break;
+		default:
+			return -EINVAL;
+		}
+		if (err < 0)
+			return err;
+
+		if (put_user(status64.stream, &status->stream) ||
+		    put_user(status64.tstamp.tv_sec, &status->tstamp.tv_sec) ||
+		    put_user(status64.tstamp.tv_nsec, &status->tstamp.tv_nsec) ||
+		    put_user(status64.avail, &status->avail) ||
+		    put_user(status64.xruns, &status->xruns))
+			return -EFAULT;
+		return 0;
+	}
+#endif
+	case SNDRV_RAWMIDI_IOCTL_STATUS64:
 	{
 		int err = 0;
-		struct snd_rawmidi_status status;
-		if (copy_from_user(&status, argp, sizeof(struct snd_rawmidi_status)))
+		struct snd_rawmidi_status64 status;
+
+		if (copy_from_user(&status, argp,
+				   sizeof(struct snd_rawmidi_status64)))
 			return -EFAULT;
 		switch (status.stream) {
 		case SNDRV_RAWMIDI_STREAM_OUTPUT:
@@ -773,7 +834,8 @@ static long snd_rawmidi_ioctl(struct file *file, unsigned int cmd, unsigned long
 		}
 		if (err < 0)
 			return err;
-		if (copy_to_user(argp, &status, sizeof(struct snd_rawmidi_status)))
+		if (copy_to_user(argp, &status,
+				 sizeof(struct snd_rawmidi_status64)))
 			return -EFAULT;
 		return 0;
 	}
diff --git a/sound/core/rawmidi_compat.c b/sound/core/rawmidi_compat.c
index f69764d..4b5a612 100644
--- a/sound/core/rawmidi_compat.c
+++ b/sound/core/rawmidi_compat.c
@@ -53,7 +53,7 @@ static int snd_rawmidi_ioctl_params_compat(struct snd_rawmidi_file *rfile,
 	return -EINVAL;
 }
 
-struct snd_rawmidi_status32 {
+struct compat_snd_rawmidi_status32 {
 	s32 stream;
 	struct compat_timespec tstamp;
 	u32 avail;
@@ -62,10 +62,10 @@ struct snd_rawmidi_status32 {
 } __attribute__((packed));
 
 static int snd_rawmidi_ioctl_status_compat(struct snd_rawmidi_file *rfile,
-					   struct snd_rawmidi_status32 __user *src)
+					   struct compat_snd_rawmidi_status32 __user *src)
 {
 	int err;
-	struct snd_rawmidi_status status;
+	struct snd_rawmidi_status64 status;
 
 	if (rfile->output == NULL)
 		return -EINVAL;
@@ -85,7 +85,8 @@ static int snd_rawmidi_ioctl_status_compat(struct snd_rawmidi_file *rfile,
 	if (err < 0)
 		return err;
 
-	if (compat_put_timespec(&status.tstamp, &src->tstamp) ||
+	if (put_user(status.tstamp.tv_sec, &src->tstamp.tv_sec) ||
+	    put_user(status.tstamp.tv_nsec, &src->tstamp.tv_nsec) ||
 	    put_user(status.avail, &src->avail) ||
 	    put_user(status.xruns, &src->xruns))
 		return -EFAULT;
@@ -93,24 +94,63 @@ static int snd_rawmidi_ioctl_status_compat(struct snd_rawmidi_file *rfile,
 	return 0;
 }
 
-#ifdef CONFIG_X86_X32
-/* X32 ABI has 64bit timespec and 64bit alignment */
-struct snd_rawmidi_status_x32 {
+struct compat_snd_rawmidi_status64 {
 	s32 stream;
 	u32 rsvd; /* alignment */
-	struct timespec tstamp;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
 	u32 avail;
 	u32 xruns;
 	unsigned char reserved[16];
 } __attribute__((packed));
 
-#define put_timespec(src, dst) copy_to_user(dst, src, sizeof(*dst))
+static int snd_rawmidi_ioctl_status_compat64(struct snd_rawmidi_file *rfile,
+					     struct compat_snd_rawmidi_status64 __user *src)
+{
+	int err;
+	struct snd_rawmidi_status64 status;
+
+	if (rfile->output == NULL)
+		return -EINVAL;
+	if (get_user(status.stream, &src->stream))
+		return -EFAULT;
+
+	switch (status.stream) {
+	case SNDRV_RAWMIDI_STREAM_OUTPUT:
+		err = snd_rawmidi_output_status(rfile->output, &status);
+		break;
+	case SNDRV_RAWMIDI_STREAM_INPUT:
+		err = snd_rawmidi_input_status(rfile->input, &status);
+		break;
+	default:
+		return -EINVAL;
+	}
+	if (err < 0)
+		return err;
+
+	if (put_user(status.tstamp.tv_sec, &src->tstamp.tv_sec) ||
+	    put_user(status.tstamp.tv_nsec, &src->tstamp.tv_nsec) ||
+	    put_user(status.avail, &src->avail) ||
+	    put_user(status.xruns, &src->xruns))
+		return -EFAULT;
+
+	return 0;
+}
+
+#ifdef IA32_EMULATION
+struct compat_snd_rawmidi_status64_x86_32 {
+	s32 stream;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	u32 avail;
+	u32 xruns;
+	unsigned char reserved[16];
+} __attribute__((packed));
 
-static int snd_rawmidi_ioctl_status_x32(struct snd_rawmidi_file *rfile,
-					struct snd_rawmidi_status_x32 __user *src)
+static int
+snd_rawmidi_ioctl_status_compat64_x86_32(struct snd_rawmidi_file *rfile,
+					 struct compat_snd_rawmidi_status64_x86_32 __user *src)
 {
 	int err;
-	struct snd_rawmidi_status status;
+	struct snd_rawmidi_status64 status;
 
 	if (rfile->output == NULL)
 		return -EINVAL;
@@ -130,21 +170,23 @@ static int snd_rawmidi_ioctl_status_x32(struct snd_rawmidi_file *rfile,
 	if (err < 0)
 		return err;
 
-	if (put_timespec(&status.tstamp, &src->tstamp) ||
+	if (put_user(status.tstamp.tv_sec, &src->tstamp.tv_sec) ||
+	    put_user(status.tstamp.tv_nsec, &src->tstamp.tv_nsec) ||
 	    put_user(status.avail, &src->avail) ||
 	    put_user(status.xruns, &src->xruns))
 		return -EFAULT;
 
 	return 0;
 }
-#endif /* CONFIG_X86_X32 */
+#endif
 
 enum {
 	SNDRV_RAWMIDI_IOCTL_PARAMS32 = _IOWR('W', 0x10, struct snd_rawmidi_params32),
-	SNDRV_RAWMIDI_IOCTL_STATUS32 = _IOWR('W', 0x20, struct snd_rawmidi_status32),
-#ifdef CONFIG_X86_X32
-	SNDRV_RAWMIDI_IOCTL_STATUS_X32 = _IOWR('W', 0x20, struct snd_rawmidi_status_x32),
-#endif /* CONFIG_X86_X32 */
+	SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT32 = _IOWR('W', 0x20, struct compat_snd_rawmidi_status32),
+	SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64 = _IOWR('W', 0x20, struct compat_snd_rawmidi_status64),
+#ifdef IA32_EMULATION
+	SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64_X86_32 = _IOWR('W', 0x20, struct compat_snd_rawmidi_status64_x86_32),
+#endif
 };
 
 static long snd_rawmidi_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
@@ -161,12 +203,14 @@ static long snd_rawmidi_ioctl_compat(struct file *file, unsigned int cmd, unsign
 		return snd_rawmidi_ioctl(file, cmd, (unsigned long)argp);
 	case SNDRV_RAWMIDI_IOCTL_PARAMS32:
 		return snd_rawmidi_ioctl_params_compat(rfile, argp);
-	case SNDRV_RAWMIDI_IOCTL_STATUS32:
+	case SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT32:
 		return snd_rawmidi_ioctl_status_compat(rfile, argp);
-#ifdef CONFIG_X86_X32
-	case SNDRV_RAWMIDI_IOCTL_STATUS_X32:
-		return snd_rawmidi_ioctl_status_x32(rfile, argp);
-#endif /* CONFIG_X86_X32 */
+	case SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64:
+		return snd_rawmidi_ioctl_status_compat64(rfile, argp);
+#ifdef IA32_EMULATION
+	case SNDRV_RAWMIDI_IOCTL_STATUS_COMPAT64_X86_32:
+		return snd_rawmidi_ioctl_status_compat64_x86_32(rfile, argp);
+#endif
 	}
 	return -ENOIOCTLCMD;
 }
-- 
1.7.9.5

Re: [RFC PATCH 4/7] sound: core: Avoid using timespec for struct snd_rawmidi_status

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> -       case SNDRV_RAWMIDI_IOCTL_STATUS:
> +#if __BITS_PER_LONG == 32
> +       case SNDRV_RAWMIDI_IOCTL_STATUS32:
> +       {
> +               int err = 0;
> +               struct snd_rawmidi_status32 __user *status = argp;
> +               struct snd_rawmidi_status32 status32;
> +               struct snd_rawmidi_status64 status64;
> +
> +               if (copy_from_user(&status32, argp,
> +                                  sizeof(struct snd_rawmidi_status32)))
> +                       return -EFAULT;
> +               switch (status32.stream) {
> +               case SNDRV_RAWMIDI_STREAM_OUTPUT:
> +                       if (rfile->output == NULL)
> +                               return -EINVAL;
> +                       err = snd_rawmidi_output_status(rfile->output, &status64);
> +                       break;
> +               case SNDRV_RAWMIDI_STREAM_INPUT:
> +                       if (rfile->input == NULL)
> +                               return -EINVAL;
> +                       err = snd_rawmidi_input_status(rfile->input, &status64);
> +                       break;
> +               default:
> +                       return -EINVAL;
> +               }
> +               if (err < 0)
> +                       return err;
> +
> +               if (put_user(status64.stream, &status->stream) ||
> +                   put_user(status64.tstamp.tv_sec, &status->tstamp.tv_sec) ||
> +                   put_user(status64.tstamp.tv_nsec, &status->tstamp.tv_nsec) ||
> +                   put_user(status64.avail, &status->avail) ||
> +                   put_user(status64.xruns, &status->xruns))
> +                       return -EFAULT;
> +               return 0;
> +       }

This follows the existing coding style for the other functions, but I think
it would be nicer to express the last part as

       status32 = (struct snd_rawmidi_status32) {
               .stream = status->stream,
               .tstamp.tv_sec, &status->tstamp.tv_sec,
               .tstamp.tv_nsec, &status->tstamp.tv_nsec,
               .avail, &status->avail,
               .xruns, &status->xruns,
       };
       if (copy_to_user(status, &status32, sizeof(*status))
              return -EFAULT;
       return 0;

It's completely equivalent, I just find my version easier to read, and
it should produce slightly better object code.

Maybe the maintainers have a preference, or there might be
a good reason to use the series of put_user() instead.

       Arnd

Re: [RFC PATCH 4/7] sound: core: Avoid using timespec for struct snd_rawmidi_status

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> -       case SNDRV_RAWMIDI_IOCTL_STATUS:
> +#if __BITS_PER_LONG == 32
> +       case SNDRV_RAWMIDI_IOCTL_STATUS32:
> +       {
> +               int err = 0;
> +               struct snd_rawmidi_status32 __user *status = argp;
> +               struct snd_rawmidi_status32 status32;
> +               struct snd_rawmidi_status64 status64;
> +
> +               if (copy_from_user(&status32, argp,
> +                                  sizeof(struct snd_rawmidi_status32)))
> +                       return -EFAULT;
> +               switch (status32.stream) {
> +               case SNDRV_RAWMIDI_STREAM_OUTPUT:
> +                       if (rfile->output == NULL)
> +                               return -EINVAL;
> +                       err = snd_rawmidi_output_status(rfile->output, &status64);
> +                       break;
> +               case SNDRV_RAWMIDI_STREAM_INPUT:
> +                       if (rfile->input == NULL)
> +                               return -EINVAL;
> +                       err = snd_rawmidi_input_status(rfile->input, &status64);
> +                       break;
> +               default:
> +                       return -EINVAL;
> +               }
> +               if (err < 0)
> +                       return err;
> +
> +               if (put_user(status64.stream, &status->stream) ||
> +                   put_user(status64.tstamp.tv_sec, &status->tstamp.tv_sec) ||
> +                   put_user(status64.tstamp.tv_nsec, &status->tstamp.tv_nsec) ||
> +                   put_user(status64.avail, &status->avail) ||
> +                   put_user(status64.xruns, &status->xruns))
> +                       return -EFAULT;
> +               return 0;
> +       }

This follows the existing coding style for the other functions, but I think
it would be nicer to express the last part as

       status32 = (struct snd_rawmidi_status32) {
               .stream = status->stream,
               .tstamp.tv_sec, &status->tstamp.tv_sec,
               .tstamp.tv_nsec, &status->tstamp.tv_nsec,
               .avail, &status->avail,
               .xruns, &status->xruns,
       };
       if (copy_to_user(status, &status32, sizeof(*status))
              return -EFAULT;
       return 0;

It's completely equivalent, I just find my version easier to read, and
it should produce slightly better object code.

Maybe the maintainers have a preference, or there might be
a good reason to use the series of put_user() instead.

       Arnd

Re: [RFC PATCH 4/7] sound: core: Avoid using timespec for struct snd_rawmidi_status

[Baolin Wang]

Hi Arnd,

On 21 September 2017 at 20:56, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> -       case SNDRV_RAWMIDI_IOCTL_STATUS:
>> +#if __BITS_PER_LONG == 32
>> +       case SNDRV_RAWMIDI_IOCTL_STATUS32:
>> +       {
>> +               int err = 0;
>> +               struct snd_rawmidi_status32 __user *status = argp;
>> +               struct snd_rawmidi_status32 status32;
>> +               struct snd_rawmidi_status64 status64;
>> +
>> +               if (copy_from_user(&status32, argp,
>> +                                  sizeof(struct snd_rawmidi_status32)))
>> +                       return -EFAULT;
>> +               switch (status32.stream) {
>> +               case SNDRV_RAWMIDI_STREAM_OUTPUT:
>> +                       if (rfile->output == NULL)
>> +                               return -EINVAL;
>> +                       err = snd_rawmidi_output_status(rfile->output, &status64);
>> +                       break;
>> +               case SNDRV_RAWMIDI_STREAM_INPUT:
>> +                       if (rfile->input == NULL)
>> +                               return -EINVAL;
>> +                       err = snd_rawmidi_input_status(rfile->input, &status64);
>> +                       break;
>> +               default:
>> +                       return -EINVAL;
>> +               }
>> +               if (err < 0)
>> +                       return err;
>> +
>> +               if (put_user(status64.stream, &status->stream) ||
>> +                   put_user(status64.tstamp.tv_sec, &status->tstamp.tv_sec) ||
>> +                   put_user(status64.tstamp.tv_nsec, &status->tstamp.tv_nsec) ||
>> +                   put_user(status64.avail, &status->avail) ||
>> +                   put_user(status64.xruns, &status->xruns))
>> +                       return -EFAULT;
>> +               return 0;
>> +       }
>
> This follows the existing coding style for the other functions, but I think
> it would be nicer to express the last part as
>
>        status32 = (struct snd_rawmidi_status32) {
>                .stream = status->stream,
>                .tstamp.tv_sec, &status->tstamp.tv_sec,
>                .tstamp.tv_nsec, &status->tstamp.tv_nsec,
>                .avail, &status->avail,
>                .xruns, &status->xruns,
>        };
>        if (copy_to_user(status, &status32, sizeof(*status))
>               return -EFAULT;
>        return 0;
>
> It's completely equivalent, I just find my version easier to read, and
> it should produce slightly better object code.
>
> Maybe the maintainers have a preference, or there might be
> a good reason to use the series of put_user() instead.

I just saw there are not many put_user() will be used in this
function, but I agree with you and I like to change as you suggested.

-- 
Baolin.wang
Best Regards

[RFC PATCH 5/7] sound: core: Avoid using timespec for struct snd_timer_status

[Baolin Wang]

The struct snd_timer_status will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Thus thia patch introduces 'struct snd_timer_status32' and 'struct snd_timer_status64'
to handle 32bit time_t and 64bit time_t in native mode, which replace
timespec with s64 type.

In compat mode, this patch renamed the structure as 'struct compat_snd_timer_status32'
to handle 32bit time_t. Moreover we should use 'struct snd_timer_status64'
to handle 64bit time_t no matter 32bit or 64bit alignment, since they have
the same size.

When glibc changes time_t to 64-bit, any recompiled program will issue ioctl
commands that the kernel does not understand without this patch.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 sound/core/timer.c        |   66 ++++++++++++++++++++++++++++++++++++++++-----
 sound/core/timer_compat.c |   25 ++++++-----------
 2 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/sound/core/timer.c b/sound/core/timer.c
index f44d702..376b7e6 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -79,6 +79,30 @@ struct snd_timer_user {
 	struct mutex ioctl_lock;
 };
 
+#if __BITS_PER_LONG == 32
+struct snd_timer_status32 {
+	struct { s32 tv_sec; s32 tv_nsec; } 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 */
+	unsigned int queue;		/* used queue size */
+	unsigned char reserved[64];	/* reserved */
+};
+
+#define SNDRV_TIMER_IOCTL_STATUS32	_IOR('T', 0x14, struct snd_timer_status32)
+#endif
+
+struct snd_timer_status64 {
+	struct { s64 tv_sec; s64 tv_nsec; } 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 */
+	unsigned int queue;		/* used queue size */
+	unsigned char reserved[64];	/* reserved */
+};
+
+#define SNDRV_TIMER_IOCTL_STATUS64	_IOR('T', 0x14, struct snd_timer_status64)
+
 /* list of timers */
 static LIST_HEAD(snd_timer_list);
 
@@ -1798,17 +1822,43 @@ static int snd_timer_user_params(struct file *file,
 	return err;
 }
 
-static int snd_timer_user_status(struct file *file,
-				 struct snd_timer_status __user *_status)
+#if __BITS_PER_LONG == 32
+static int snd_timer_user_status32(struct file *file,
+				   struct snd_timer_status32 __user *_status)
+ {
+	struct snd_timer_user *tu;
+	struct snd_timer_status32 status;
+
+	tu = file->private_data;
+	if (!tu->timeri)
+		return -EBADFD;
+	memset(&status, 0, sizeof(status));
+	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;
+	spin_lock_irq(&tu->qlock);
+	status.queue = tu->qused;
+	spin_unlock_irq(&tu->qlock);
+	if (copy_to_user(_status, &status, sizeof(status)))
+		return -EFAULT;
+	return 0;
+}
+#endif
+
+static int snd_timer_user_status64(struct file *file,
+				   struct snd_timer_status64 __user *_status)
 {
 	struct snd_timer_user *tu;
-	struct snd_timer_status status;
+	struct snd_timer_status64 status;
 
 	tu = file->private_data;
 	if (!tu->timeri)
 		return -EBADFD;
 	memset(&status, 0, sizeof(status));
-	status.tstamp = timespec64_to_timespec(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;
@@ -1920,8 +1970,12 @@ static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return snd_timer_user_info(file, argp);
 	case SNDRV_TIMER_IOCTL_PARAMS:
 		return snd_timer_user_params(file, argp);
-	case SNDRV_TIMER_IOCTL_STATUS:
-		return snd_timer_user_status(file, argp);
+#if __BITS_PER_LONG == 32
+	case SNDRV_TIMER_IOCTL_STATUS32:
+		return snd_timer_user_status32(file, argp);
+#endif
+	case SNDRV_TIMER_IOCTL_STATUS64:
+		return snd_timer_user_status64(file, argp);
 	case SNDRV_TIMER_IOCTL_START:
 	case SNDRV_TIMER_IOCTL_START_OLD:
 		return snd_timer_user_start(file);
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
index 6a437eb..c592603 100644
--- a/sound/core/timer_compat.c
+++ b/sound/core/timer_compat.c
@@ -83,7 +83,7 @@ static int snd_timer_user_info_compat(struct file *file,
 	return 0;
 }
 
-struct snd_timer_status32 {
+struct compat_snd_timer_status32 {
 	struct compat_timespec tstamp;
 	u32 resolution;
 	u32 lost;
@@ -93,10 +93,10 @@ struct snd_timer_status32 {
 };
 
 static int snd_timer_user_status_compat(struct file *file,
-					struct snd_timer_status32 __user *_status)
+					struct compat_snd_timer_status32 __user *_status)
 {
 	struct snd_timer_user *tu;
-	struct snd_timer_status32 status;
+	struct compat_snd_timer_status32 status;
 	
 	tu = file->private_data;
 	if (snd_BUG_ON(!tu->timeri))
@@ -115,12 +115,6 @@ static int snd_timer_user_status_compat(struct file *file,
 	return 0;
 }
 
-#ifdef CONFIG_X86_X32
-/* X32 ABI has the same struct as x86-64 */
-#define snd_timer_user_status_x32(file, s) \
-	snd_timer_user_status(file, s)
-#endif /* CONFIG_X86_X32 */
-
 /*
  */
 
@@ -128,9 +122,8 @@ enum {
 	SNDRV_TIMER_IOCTL_GPARAMS32 = _IOW('T', 0x04, struct snd_timer_gparams32),
 	SNDRV_TIMER_IOCTL_INFO32 = _IOR('T', 0x11, struct snd_timer_info32),
 	SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
-#ifdef CONFIG_X86_X32
-	SNDRV_TIMER_IOCTL_STATUS_X32 = _IOW('T', 0x14, struct snd_timer_status),
-#endif /* CONFIG_X86_X32 */
+	SNDRV_TIMER_IOCTL_STATUS_COMPAT32 = _IOW('T', 0x14, struct compat_snd_timer_status32),
+	SNDRV_TIMER_IOCTL_STATUS_COMPAT64 = _IOW('T', 0x14, struct snd_timer_status64),
 };
 
 static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
@@ -158,12 +151,10 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
 		return snd_timer_user_gparams_compat(file, argp);
 	case SNDRV_TIMER_IOCTL_INFO32:
 		return snd_timer_user_info_compat(file, argp);
-	case SNDRV_TIMER_IOCTL_STATUS32:
+	case SNDRV_TIMER_IOCTL_STATUS_COMPAT32:
 		return snd_timer_user_status_compat(file, argp);
-#ifdef CONFIG_X86_X32
-	case SNDRV_TIMER_IOCTL_STATUS_X32:
-		return snd_timer_user_status_x32(file, argp);
-#endif /* CONFIG_X86_X32 */
+	case SNDRV_TIMER_IOCTL_STATUS_COMPAT64:
+		return snd_timer_user_status64(file, argp);
 	}
 	return -ENOIOCTLCMD;
 }
-- 
1.7.9.5

Re: [RFC PATCH 5/7] sound: core: Avoid using timespec for struct snd_timer_status

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

>  static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
> @@ -158,12 +151,10 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>                 return snd_timer_user_gparams_compat(file, argp);
>         case SNDRV_TIMER_IOCTL_INFO32:
>                 return snd_timer_user_info_compat(file, argp);
> -       case SNDRV_TIMER_IOCTL_STATUS32:
> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT32:
>                 return snd_timer_user_status_compat(file, argp);
> -#ifdef CONFIG_X86_X32
> -       case SNDRV_TIMER_IOCTL_STATUS_X32:
> -               return snd_timer_user_status_x32(file, argp);
> -#endif /* CONFIG_X86_X32 */
> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT64:
> +               return snd_timer_user_status64(file, argp);
>         }

I think the last line would fail to build since snd_timer_user_status64()
is defined 'static' in a different file.

Also, snd_timer_user_status_compat() seems to be the same as
snd_timer_user_status32(), so I think you can redirect it the same
way as snd_timer_user_status64 after making both functions globally
visible.

      Arnd

Re: [RFC PATCH 5/7] sound: core: Avoid using timespec for struct snd_timer_status

[Baolin Wang]

On 21 September 2017 at 21:14, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>>  static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
>> @@ -158,12 +151,10 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>>                 return snd_timer_user_gparams_compat(file, argp);
>>         case SNDRV_TIMER_IOCTL_INFO32:
>>                 return snd_timer_user_info_compat(file, argp);
>> -       case SNDRV_TIMER_IOCTL_STATUS32:
>> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT32:
>>                 return snd_timer_user_status_compat(file, argp);
>> -#ifdef CONFIG_X86_X32
>> -       case SNDRV_TIMER_IOCTL_STATUS_X32:
>> -               return snd_timer_user_status_x32(file, argp);
>> -#endif /* CONFIG_X86_X32 */
>> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT64:
>> +               return snd_timer_user_status64(file, argp);
>>         }
>
> I think the last line would fail to build since snd_timer_user_status64()
> is defined 'static' in a different file.

I saw the timer_compat.c file will be included into timer.c file, so I
think it will not. (My arm32 platform can not build compat mode, but I
will try again to make sure it can build successfully.)

>
> Also, snd_timer_user_status_compat() seems to be the same as
> snd_timer_user_status32(), so I think you can redirect it the same
> way as snd_timer_user_status64 after making both functions globally
> visible.

OK. Let me check again.

-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 5/7] sound: core: Avoid using timespec for struct snd_timer_status

[Baolin Wang]

On 21 September 2017 at 21:14, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>>  static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
>> @@ -158,12 +151,10 @@ static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
>>                 return snd_timer_user_gparams_compat(file, argp);
>>         case SNDRV_TIMER_IOCTL_INFO32:
>>                 return snd_timer_user_info_compat(file, argp);
>> -       case SNDRV_TIMER_IOCTL_STATUS32:
>> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT32:
>>                 return snd_timer_user_status_compat(file, argp);
>> -#ifdef CONFIG_X86_X32
>> -       case SNDRV_TIMER_IOCTL_STATUS_X32:
>> -               return snd_timer_user_status_x32(file, argp);
>> -#endif /* CONFIG_X86_X32 */
>> +       case SNDRV_TIMER_IOCTL_STATUS_COMPAT64:
>> +               return snd_timer_user_status64(file, argp);
>>         }
>
> I think the last line would fail to build since snd_timer_user_status64()
> is defined 'static' in a different file.

I saw the timer_compat.c file will be included into timer.c file, so I
think it will not. (My arm32 platform can not build compat mode, but I
will try again to make sure it can build successfully.)

>
> Also, snd_timer_user_status_compat() seems to be the same as
> snd_timer_user_status32(), so I think you can redirect it the same
> way as snd_timer_user_status64 after making both functions globally
> visible.

OK. Let me check again.

-- 
Baolin.wang
Best Regards

[RFC PATCH 6/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value

[Baolin Wang]

The struct snd_ctl_elem_value will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Since there are no drivers will implemented the tstamp member of the
struct snd_ctl_elem_value, and also the stucture size will not be changed
if we change timespec to s64 for tstamp member of struct snd_ctl_elem_value.

Thus we can simply change timespec to s64 for tstamp member to avoid
using the type which is not year 2038 safe on 32bits system.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/uapi/sound/asound.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 1949923..71bce52 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -943,8 +943,8 @@ struct snd_ctl_elem_value {
 		} bytes;
 		struct snd_aes_iec958 iec958;
 	} value;		/* RO */
-	struct timespec tstamp;
-	unsigned char reserved[128-sizeof(struct timespec)];
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	unsigned char reserved[128-sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
 };
 
 struct snd_ctl_tlv {
-- 
1.7.9.5

Re: [RFC PATCH 6/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> The struct snd_ctl_elem_value will use 'timespec' type variables to record
> timestamp, which is not year 2038 safe on 32bits system.
>
> Since there are no drivers will implemented the tstamp member of the
> struct snd_ctl_elem_value, and also the stucture size will not be changed
> if we change timespec to s64 for tstamp member of struct snd_ctl_elem_value.
>
> Thus we can simply change timespec to s64 for tstamp member to avoid
> using the type which is not year 2038 safe on 32bits system.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  include/uapi/sound/asound.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 1949923..71bce52 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -943,8 +943,8 @@ struct snd_ctl_elem_value {
>                 } bytes;
>                 struct snd_aes_iec958 iec958;
>         } value;                /* RO */
> -       struct timespec tstamp;
> -       unsigned char reserved[128-sizeof(struct timespec)];
> +       struct { s64 tv_sec; s64 tv_nsec; } tstamp;
> +       unsigned char reserved[128-sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
>  };

Maybe we should enforce that nobody uses the timespec field, by
enclosing it in #ifdef __KERNEL__ (with a matching length below it);

       Arnd

Re: [RFC PATCH 6/7] uapi: sound: Avoid using timespec for struct snd_ctl_elem_value

[Baolin Wang]

On 21 September 2017 at 20:58, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> The struct snd_ctl_elem_value will use 'timespec' type variables to record
>> timestamp, which is not year 2038 safe on 32bits system.
>>
>> Since there are no drivers will implemented the tstamp member of the
>> struct snd_ctl_elem_value, and also the stucture size will not be changed
>> if we change timespec to s64 for tstamp member of struct snd_ctl_elem_value.
>>
>> Thus we can simply change timespec to s64 for tstamp member to avoid
>> using the type which is not year 2038 safe on 32bits system.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  include/uapi/sound/asound.h |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
>> index 1949923..71bce52 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -943,8 +943,8 @@ struct snd_ctl_elem_value {
>>                 } bytes;
>>                 struct snd_aes_iec958 iec958;
>>         } value;                /* RO */
>> -       struct timespec tstamp;
>> -       unsigned char reserved[128-sizeof(struct timespec)];
>> +       struct { s64 tv_sec; s64 tv_nsec; } tstamp;
>> +       unsigned char reserved[128-sizeof(struct { s64 tv_sec; s64 tv_nsec; })];
>>  };
>
> Maybe we should enforce that nobody uses the timespec field, by
> enclosing it in #ifdef __KERNEL__ (with a matching length below it);

OK.

-- 
Baolin.wang
Best Regards

[RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Baolin Wang]

The struct snd_timer_tread will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Since the struct snd_timer_tread is passed through read() rather than
ioctl(), and the read syscall has no command number that lets us pick
between the 32-bit or 64-bit version of this structure.

Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new
struct snd_timer_tread64 replacing timespec with s64 type to handle
64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to
handle 64bit time_t with 32bit alignment. That means we will set
tu->tread = 2 when user space has a 64bit time_t, then we will copy
to user with struct snd_timer_tread64. For x86_32 mode, we will set
tu->tread = 3 to copy struct compat_snd_timer_tread64_x86_32 for user.
Otherwise we will use 32bit time_t variables when copying to user.

Moreover this patch replaces timespec type with timespec64 type and
related y2038 safe APIs.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 include/uapi/sound/asound.h |   11 ++-
 sound/core/timer.c          |  163 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 142 insertions(+), 32 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 71bce52..ef738bf 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -760,7 +760,7 @@ struct snd_timer_status {
 
 #define SNDRV_TIMER_IOCTL_PVERSION	_IOR('T', 0x00, int)
 #define SNDRV_TIMER_IOCTL_NEXT_DEVICE	_IOWR('T', 0x01, struct snd_timer_id)
-#define SNDRV_TIMER_IOCTL_TREAD		_IOW('T', 0x02, int)
+#define SNDRV_TIMER_IOCTL_TREAD_OLD	_IOW('T', 0x02, int)
 #define SNDRV_TIMER_IOCTL_GINFO		_IOWR('T', 0x03, struct snd_timer_ginfo)
 #define SNDRV_TIMER_IOCTL_GPARAMS	_IOW('T', 0x04, struct snd_timer_gparams)
 #define SNDRV_TIMER_IOCTL_GSTATUS	_IOWR('T', 0x05, struct snd_timer_gstatus)
@@ -773,6 +773,15 @@ struct snd_timer_status {
 #define SNDRV_TIMER_IOCTL_STOP		_IO('T', 0xa1)
 #define SNDRV_TIMER_IOCTL_CONTINUE	_IO('T', 0xa2)
 #define SNDRV_TIMER_IOCTL_PAUSE		_IO('T', 0xa3)
+#define SNDRV_TIMER_IOCTL_TREAD64	_IOW('T', 0xa4, int)
+
+#if __BITS_PER_LONG == 64
+#define SNDRV_TIMER_IOCTL_TREAD SNDRV_TIMER_IOCTL_TREAD_OLD
+#else
+#define SNDRV_TIMER_IOCTL_TREAD ((sizeof(__kernel_long_t) > sizeof(time_t)) ? \
+				 SNDRV_TIMER_IOCTL_TREAD_OLD : \
+				 SNDRV_TIMER_IOCTL_TREAD64)
+#endif
 
 struct snd_timer_read {
 	unsigned int resolution;
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 376b7e6..c33a1be 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -58,6 +58,22 @@
 MODULE_ALIAS_CHARDEV(CONFIG_SND_MAJOR, SNDRV_MINOR_TIMER);
 MODULE_ALIAS("devname:snd/timer");
 
+struct snd_timer_tread64 {
+	int event;
+	u32 pad1;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	unsigned int val;
+	u32 pad2;
+};
+
+#ifdef IA32_EMULATION
+struct compat_snd_timer_tread64_x86_32 {
+	int event;
+	struct { s64 tv_sec; s64 tv_nsec; } tstamp;
+	unsigned int val;
+} __packed;
+#endif
+
 struct snd_timer_user {
 	struct snd_timer_instance *timeri;
 	int tread;		/* enhanced read with timestamps and events */
@@ -69,7 +85,7 @@ struct snd_timer_user {
 	int queue_size;
 	bool disconnected;
 	struct snd_timer_read *queue;
-	struct snd_timer_tread *tqueue;
+	struct snd_timer_tread64 *tqueue;
 	spinlock_t qlock;
 	unsigned long last_resolution;
 	unsigned int filter;
@@ -1262,7 +1278,7 @@ static void snd_timer_user_interrupt(struct snd_timer_instance *timeri,
 }
 
 static void snd_timer_user_append_to_tqueue(struct snd_timer_user *tu,
-					    struct snd_timer_tread *tread)
+					    struct snd_timer_tread64 *tread)
 {
 	if (tu->qused >= tu->queue_size) {
 		tu->overrun++;
@@ -1279,7 +1295,7 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 				     unsigned long resolution)
 {
 	struct snd_timer_user *tu = timeri->callback_data;
-	struct snd_timer_tread r1;
+	struct snd_timer_tread64 r1;
 	unsigned long flags;
 
 	if (event >= SNDRV_TIMER_EVENT_START &&
@@ -1289,7 +1305,8 @@ static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 		return;
 	memset(&r1, 0, sizeof(r1));
 	r1.event = event;
-	r1.tstamp = timespec64_to_timespec(*tstamp);
+	r1.tstamp.tv_sec = tstamp->tv_sec;
+	r1.tstamp.tv_nsec = tstamp->tv_nsec;
 	r1.val = resolution;
 	spin_lock_irqsave(&tu->qlock, flags);
 	snd_timer_user_append_to_tqueue(tu, &r1);
@@ -1311,7 +1328,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 				      unsigned long ticks)
 {
 	struct snd_timer_user *tu = timeri->callback_data;
-	struct snd_timer_tread *r, r1;
+	struct snd_timer_tread64 *r, r1;
 	struct timespec64 tstamp;
 	int prev, append = 0;
 
@@ -1332,7 +1349,8 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 	if ((tu->filter & (1 << SNDRV_TIMER_EVENT_RESOLUTION)) &&
 	    tu->last_resolution != resolution) {
 		r1.event = SNDRV_TIMER_EVENT_RESOLUTION;
-		r1.tstamp = timespec64_to_timespec(tstamp);
+		r1.tstamp.tv_sec = tstamp.tv_sec;
+		r1.tstamp.tv_nsec = tstamp.tv_nsec;
 		r1.val = resolution;
 		snd_timer_user_append_to_tqueue(tu, &r1);
 		tu->last_resolution = resolution;
@@ -1346,14 +1364,16 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 		prev = tu->qtail == 0 ? tu->queue_size - 1 : tu->qtail - 1;
 		r = &tu->tqueue[prev];
 		if (r->event == SNDRV_TIMER_EVENT_TICK) {
-			r->tstamp = timespec64_to_timespec(tstamp);
+			r->tstamp.tv_sec = tstamp.tv_sec;
+			r->tstamp.tv_nsec = tstamp.tv_nsec;
 			r->val += ticks;
 			append++;
 			goto __wake;
 		}
 	}
 	r1.event = SNDRV_TIMER_EVENT_TICK;
-	r1.tstamp = timespec64_to_timespec(tstamp);
+	r1.tstamp.tv_sec = tstamp.tv_sec;
+	r1.tstamp.tv_nsec = tstamp.tv_nsec;
 	r1.val = ticks;
 	snd_timer_user_append_to_tqueue(tu, &r1);
 	append++;
@@ -1368,7 +1388,7 @@ static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 static int realloc_user_queue(struct snd_timer_user *tu, int size)
 {
 	struct snd_timer_read *queue = NULL;
-	struct snd_timer_tread *tqueue = NULL;
+	struct snd_timer_tread64 *tqueue = NULL;
 
 	if (tu->tread) {
 		tqueue = kcalloc(size, sizeof(*tqueue), GFP_KERNEL);
@@ -1797,7 +1817,7 @@ static int snd_timer_user_params(struct file *file,
 	tu->qhead = tu->qtail = tu->qused = 0;
 	if (tu->timeri->flags & SNDRV_TIMER_IFLG_EARLY_EVENT) {
 		if (tu->tread) {
-			struct snd_timer_tread tread;
+			struct snd_timer_tread64 tread;
 			memset(&tread, 0, sizeof(tread));
 			tread.event = SNDRV_TIMER_EVENT_EARLY;
 			tread.tstamp.tv_sec = 0;
@@ -1921,6 +1941,39 @@ static int snd_timer_user_pause(struct file *file)
 	return (err = snd_timer_pause(tu->timeri)) < 0 ? err : 0;
 }
 
+static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
+				unsigned int cmd)
+{
+	int __user *p = argp;
+	int xarg, old_tread;
+
+	if (tu->timeri)	/* too late */
+		return -EBUSY;
+	if (get_user(xarg, p))
+		return -EFAULT;
+
+	old_tread = tu->tread;
+#if __BITS_PER_LONG == 64
+	tu->tread = xarg ? 2 : 0;
+#ifdef IA32_EMULATION
+	tu->tread = xarg ? 3 : 0;
+#endif
+#else
+	if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
+		tu->tread = xarg ? 2 : 0;
+	else
+		tu->tread = xarg ? 1 : 0;
+#endif
+
+	if (tu->tread != old_tread &&
+	    realloc_user_queue(tu, tu->queue_size) < 0) {
+		tu->tread = old_tread;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 enum {
 	SNDRV_TIMER_IOCTL_START_OLD = _IO('T', 0x20),
 	SNDRV_TIMER_IOCTL_STOP_OLD = _IO('T', 0x21),
@@ -1941,23 +1994,9 @@ static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return put_user(SNDRV_TIMER_VERSION, p) ? -EFAULT : 0;
 	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
 		return snd_timer_user_next_device(argp);
-	case SNDRV_TIMER_IOCTL_TREAD:
-	{
-		int xarg, old_tread;
-
-		if (tu->timeri)	/* too late */
-			return -EBUSY;
-		if (get_user(xarg, p))
-			return -EFAULT;
-		old_tread = tu->tread;
-		tu->tread = xarg ? 1 : 0;
-		if (tu->tread != old_tread &&
-		    realloc_user_queue(tu, tu->queue_size) < 0) {
-			tu->tread = old_tread;
-			return -ENOMEM;
-		}
-		return 0;
-	}
+	case SNDRV_TIMER_IOCTL_TREAD_OLD:
+	case SNDRV_TIMER_IOCTL_TREAD64:
+		return snd_timer_user_tread(argp, tu, cmd);
 	case SNDRV_TIMER_IOCTL_GINFO:
 		return snd_timer_user_ginfo(file, argp);
 	case SNDRV_TIMER_IOCTL_GPARAMS:
@@ -2021,7 +2060,25 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 	int err = 0;
 
 	tu = file->private_data;
-	unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);
+	switch (tu->tread) {
+#ifdef IA32_EMULATION
+	case 3:
+		unit = sizeof(struct compat_snd_timer_tread64_x86_32);
+		break;
+#endif
+	case 2:
+		unit = sizeof(struct snd_timer_tread64);
+		break;
+	case 1:
+		unit = sizeof(struct snd_timer_tread);
+		break;
+	case 0:
+		unit = sizeof(struct snd_timer_read);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
 	mutex_lock(&tu->ioctl_lock);
 	spin_lock_irq(&tu->qlock);
 	while ((long)count - result >= unit) {
@@ -2061,9 +2118,53 @@ static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 		spin_unlock_irq(&tu->qlock);
 
 		if (tu->tread) {
-			if (copy_to_user(buffer, &tu->tqueue[qhead],
-					 sizeof(struct snd_timer_tread)))
-				err = -EFAULT;
+			struct snd_timer_tread64 *tread = &tu->tqueue[qhead];
+			struct snd_timer_tread tread32;
+#ifdef IA32_EMULATION
+			struct compat_snd_timer_tread64_x86_32 compat_tread64;
+#endif
+
+			switch (tu->tread) {
+#ifdef IA32_EMULATION
+			case 3:
+				memset(&compat_tread64, 0, sizeof(compat_tread64));
+				compat_tread64 = (struct compat_snd_timer_tread64_x86_32) {
+					.event = tread->event,
+					.tstamp = {
+						.tv_sec = tread->tstamp.tv_sec,
+						.tv_nsec = tread->tstamp.tv_nsec,
+					},
+					.val = tread->val,
+				};
+
+				if (copy_to_user(buffer, &compat_tread64,
+						 sizeof(compat_tread64)))
+					err = -EFAULT;
+				break;
+#endif
+			case 2:
+				if (copy_to_user(buffer, tread,
+						 sizeof(struct snd_timer_tread64)))
+					err = -EFAULT;
+				break;
+			case 1:
+				memset(&tread32, 0, sizeof(tread32));
+				tread32 = (struct snd_timer_tread) {
+					.event = tread->event,
+					.tstamp = {
+						.tv_sec = tread->tstamp.tv_sec,
+						.tv_nsec = tread->tstamp.tv_nsec,
+					},
+					.val = tread->val,
+				};
+
+				if (copy_to_user(buffer, &tread32, sizeof(tread32)))
+					err = -EFAULT;
+				break;
+			default:
+				err = -ENOTSUPP;
+				break;
+			}
 		} else {
 			if (copy_to_user(buffer, &tu->queue[qhead],
 					 sizeof(struct snd_timer_read)))
-- 
1.7.9.5

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Arnd Bergmann]

On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:

> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
> +                               unsigned int cmd)
> +{
> +       int __user *p = argp;
> +       int xarg, old_tread;
> +
> +       if (tu->timeri) /* too late */
> +               return -EBUSY;
> +       if (get_user(xarg, p))
> +               return -EFAULT;
> +
> +       old_tread = tu->tread;
> +#if __BITS_PER_LONG == 64
> +       tu->tread = xarg ? 2 : 0;
> +#ifdef IA32_EMULATION
> +       tu->tread = xarg ? 3 : 0;
> +#endif
> +#else
> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
> +               tu->tread = xarg ? 2 : 0;
> +       else
> +               tu->tread = xarg ? 1 : 0;
> +#endif

The 64-bit case looks broken here:

- The tread flag is different for compat and native mode, so you
   must pass a flag to identify whether you are called from
   __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().

- On x86, you have to check whether calling user space process uses
   the i386 or the x32 ABI by checking in_x32_syscall()

       Arnd

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Baolin Wang]

On 21 September 2017 at 21:09, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
>> +                               unsigned int cmd)
>> +{
>> +       int __user *p = argp;
>> +       int xarg, old_tread;
>> +
>> +       if (tu->timeri) /* too late */
>> +               return -EBUSY;
>> +       if (get_user(xarg, p))
>> +               return -EFAULT;
>> +
>> +       old_tread = tu->tread;
>> +#if __BITS_PER_LONG == 64
>> +       tu->tread = xarg ? 2 : 0;
>> +#ifdef IA32_EMULATION
>> +       tu->tread = xarg ? 3 : 0;
>> +#endif
>> +#else
>> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
>> +               tu->tread = xarg ? 2 : 0;
>> +       else
>> +               tu->tread = xarg ? 1 : 0;
>> +#endif
>
> The 64-bit case looks broken here:
>
> - The tread flag is different for compat and native mode, so you
>    must pass a flag to identify whether you are called from
>    __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().

I have some confusion here. For 64-bit, we will set tu->tread = 2 no
matter it is native mode or compat mode, only we will set tu->tread =
3 for x86_32 in compat mode, right?
So I think we do not need to identify whether called from native mode
or compat mode.

>
> - On x86, you have to check whether calling user space process uses
>    the i386 or the x32 ABI by checking in_x32_syscall()

Make sense.

-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 5:00 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 21 September 2017 at 21:09, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>
>>> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
>>> +                               unsigned int cmd)
>>> +{
>>> +       int __user *p = argp;
>>> +       int xarg, old_tread;
>>> +
>>> +       if (tu->timeri) /* too late */
>>> +               return -EBUSY;
>>> +       if (get_user(xarg, p))
>>> +               return -EFAULT;
>>> +
>>> +       old_tread = tu->tread;
>>> +#if __BITS_PER_LONG == 64
>>> +       tu->tread = xarg ? 2 : 0;
>>> +#ifdef IA32_EMULATION
>>> +       tu->tread = xarg ? 3 : 0;
>>> +#endif
>>> +#else
>>> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
>>> +               tu->tread = xarg ? 2 : 0;
>>> +       else
>>> +               tu->tread = xarg ? 1 : 0;
>>> +#endif
>>
>> The 64-bit case looks broken here:
>>
>> - The tread flag is different for compat and native mode, so you
>>    must pass a flag to identify whether you are called from
>>    __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().
>
> I have some confusion here. For 64-bit, we will set tu->tread = 2 no
> matter it is native mode or compat mode, only we will set tu->tread =
> 3 for x86_32 in compat mode, right?
> So I think we do not need to identify whether called from native mode
> or compat mode.

When we have a user space program with 32-bit time_t in compat mode
(i.e. cmd==SNDRV_TIMER_IOCTL_TREAD) on a 64-bit kernel, we want
to set tread=1, and that is different from the native mode that wants to
set tread=2.

For determining whether to use tread=2 or tread=3, we have to check
both compat mode and x32 mode. This could be done by checking for
"if (IS_ENABLED(CONFIG_IA32_EMULATION) && in_compat_syscall() &&
is_x32_task())", but the in_compat_syscall() check can be skipped when
you know that you were called from .compat_ioct().

       Arnd

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Arnd Bergmann]

On Fri, Sep 22, 2017 at 5:00 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
> On 21 September 2017 at 21:09, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>
>>> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
>>> +                               unsigned int cmd)
>>> +{
>>> +       int __user *p = argp;
>>> +       int xarg, old_tread;
>>> +
>>> +       if (tu->timeri) /* too late */
>>> +               return -EBUSY;
>>> +       if (get_user(xarg, p))
>>> +               return -EFAULT;
>>> +
>>> +       old_tread = tu->tread;
>>> +#if __BITS_PER_LONG == 64
>>> +       tu->tread = xarg ? 2 : 0;
>>> +#ifdef IA32_EMULATION
>>> +       tu->tread = xarg ? 3 : 0;
>>> +#endif
>>> +#else
>>> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
>>> +               tu->tread = xarg ? 2 : 0;
>>> +       else
>>> +               tu->tread = xarg ? 1 : 0;
>>> +#endif
>>
>> The 64-bit case looks broken here:
>>
>> - The tread flag is different for compat and native mode, so you
>>    must pass a flag to identify whether you are called from
>>    __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().
>
> I have some confusion here. For 64-bit, we will set tu->tread = 2 no
> matter it is native mode or compat mode, only we will set tu->tread =
> 3 for x86_32 in compat mode, right?
> So I think we do not need to identify whether called from native mode
> or compat mode.

When we have a user space program with 32-bit time_t in compat mode
(i.e. cmd==SNDRV_TIMER_IOCTL_TREAD) on a 64-bit kernel, we want
to set tread=1, and that is different from the native mode that wants to
set tread=2.

For determining whether to use tread=2 or tread=3, we have to check
both compat mode and x32 mode. This could be done by checking for
"if (IS_ENABLED(CONFIG_IA32_EMULATION) && in_compat_syscall() &&
is_x32_task())", but the in_compat_syscall() check can be skipped when
you know that you were called from .compat_ioct().

       Arnd

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Baolin Wang]

On 22 September 2017 at 15:57, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Sep 22, 2017 at 5:00 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 21 September 2017 at 21:09, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>
>>>> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
>>>> +                               unsigned int cmd)
>>>> +{
>>>> +       int __user *p = argp;
>>>> +       int xarg, old_tread;
>>>> +
>>>> +       if (tu->timeri) /* too late */
>>>> +               return -EBUSY;
>>>> +       if (get_user(xarg, p))
>>>> +               return -EFAULT;
>>>> +
>>>> +       old_tread = tu->tread;
>>>> +#if __BITS_PER_LONG == 64
>>>> +       tu->tread = xarg ? 2 : 0;
>>>> +#ifdef IA32_EMULATION
>>>> +       tu->tread = xarg ? 3 : 0;
>>>> +#endif
>>>> +#else
>>>> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
>>>> +               tu->tread = xarg ? 2 : 0;
>>>> +       else
>>>> +               tu->tread = xarg ? 1 : 0;
>>>> +#endif
>>>
>>> The 64-bit case looks broken here:
>>>
>>> - The tread flag is different for compat and native mode, so you
>>>    must pass a flag to identify whether you are called from
>>>    __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().
>>
>> I have some confusion here. For 64-bit, we will set tu->tread = 2 no
>> matter it is native mode or compat mode, only we will set tu->tread =
>> 3 for x86_32 in compat mode, right?
>> So I think we do not need to identify whether called from native mode
>> or compat mode.
>
> When we have a user space program with 32-bit time_t in compat mode
> (i.e. cmd==SNDRV_TIMER_IOCTL_TREAD) on a 64-bit kernel, we want
> to set tread=1, and that is different from the native mode that wants to
> set tread=2.

I understand your meaning now, thanks for explanation.

>
> For determining whether to use tread=2 or tread=3, we have to check
> both compat mode and x32 mode. This could be done by checking for
> "if (IS_ENABLED(CONFIG_IA32_EMULATION) && in_compat_syscall() &&
> is_x32_task())", but the in_compat_syscall() check can be skipped when
> you know that you were called from .compat_ioct().

OK.

-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Baolin Wang]

On 22 September 2017 at 15:57, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Sep 22, 2017 at 5:00 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>> On 21 September 2017 at 21:09, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>
>>>> +static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
>>>> +                               unsigned int cmd)
>>>> +{
>>>> +       int __user *p = argp;
>>>> +       int xarg, old_tread;
>>>> +
>>>> +       if (tu->timeri) /* too late */
>>>> +               return -EBUSY;
>>>> +       if (get_user(xarg, p))
>>>> +               return -EFAULT;
>>>> +
>>>> +       old_tread = tu->tread;
>>>> +#if __BITS_PER_LONG == 64
>>>> +       tu->tread = xarg ? 2 : 0;
>>>> +#ifdef IA32_EMULATION
>>>> +       tu->tread = xarg ? 3 : 0;
>>>> +#endif
>>>> +#else
>>>> +       if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
>>>> +               tu->tread = xarg ? 2 : 0;
>>>> +       else
>>>> +               tu->tread = xarg ? 1 : 0;
>>>> +#endif
>>>
>>> The 64-bit case looks broken here:
>>>
>>> - The tread flag is different for compat and native mode, so you
>>>    must pass a flag to identify whether you are called from
>>>    __snd_timer_user_ioctl or from snd_timer_user_ioctl_compat().
>>
>> I have some confusion here. For 64-bit, we will set tu->tread = 2 no
>> matter it is native mode or compat mode, only we will set tu->tread =
>> 3 for x86_32 in compat mode, right?
>> So I think we do not need to identify whether called from native mode
>> or compat mode.
>
> When we have a user space program with 32-bit time_t in compat mode
> (i.e. cmd==SNDRV_TIMER_IOCTL_TREAD) on a 64-bit kernel, we want
> to set tread=1, and that is different from the native mode that wants to
> set tread=2.

I understand your meaning now, thanks for explanation.

>
> For determining whether to use tread=2 or tread=3, we have to check
> both compat mode and x32 mode. This could be done by checking for
> "if (IS_ENABLED(CONFIG_IA32_EMULATION) && in_compat_syscall() &&
> is_x32_task())", but the in_compat_syscall() check can be skipped when
> you know that you were called from .compat_ioct().

OK.

-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Takashi Sakamoto]

Hi,

On Sep 21 2017 15:18, Baolin Wang wrote:
> Since many structures will use timespec type variables to record time stamp
> in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset
> tries to introduce new structures removing timespec type to compatible native
> mode and compat mode.
> 
> Moreover this patchset also converts the internal structrures to use timespec64
> type and related APIs.
> 
> Baolin Wang (7):
>    sound: Replace timespec with timespec64
>    sound: core: Avoid using timespec for struct snd_pcm_status
>    sound: core: Avoid using timespec for struct snd_pcm_sync_ptr
>    sound: core: Avoid using timespec for struct snd_rawmidi_status
>    sound: core: Avoid using timespec for struct snd_timer_status
>    uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
>    sound: core: Avoid using timespec for struct snd_timer_tread
> 
>   include/sound/pcm.h               |  113 ++++++++-
>   include/sound/timer.h             |    4 +-
>   include/uapi/sound/asound.h       |   15 +-
>   sound/core/pcm.c                  |   14 +-
>   sound/core/pcm_compat.c           |  466 +++++++++++++++++++++++++++++--------
>   sound/core/pcm_lib.c              |   33 +--
>   sound/core/pcm_native.c           |  227 ++++++++++++++----
>   sound/core/rawmidi.c              |   74 +++++-
>   sound/core/rawmidi_compat.c       |   90 +++++--
>   sound/core/timer.c                |  247 ++++++++++++++++----
>   sound/core/timer_compat.c         |   25 +-
>   sound/pci/hda/hda_controller.c    |   10 +-
>   sound/soc/intel/skylake/skl-pcm.c |    4 +-
>   13 files changed, 1046 insertions(+), 276 deletions(-)

I'm a minor Takashi in this subsystem and not those who you'd like to
talk about this issue. But I have interests in it and would like to
assist you, as long as I can do for it.

As a nitpicking, your patchset brings compilation error at
configurations for x86, and x86-64 with x32 ABI support.


## x86-64 architecture and amd64 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y

Success.


## x86-64 architecture and amd64/x32 ABI support
CONFIG_64BIT=y
CONFIG_X86_64=y
CONFIG_X86_X32=y

```
sound/core/timer_compat.c:124:54: error: array type has incomplete 
element type 'struct snd_timer_status32'
   SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
```

This error comes from a commit 1229cccbefe7 ('sound: core: Avoid using 
timespec for struct snd_timer_status').

```
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_compat':
sound/core/pcm_compat.c:623:9: error: assignment from incompatible 
pointer type [-Werror=incompatible-pointer-types]
   status = runtime->status;
          ^
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_x32':
sound/core/pcm_compat.c:711:9: error: assignment from incompatible 
pointer type [-Werror=incompatible-pointer-types]
   status = runtime->status;
```

This error comes from a commit 947c463adc00('sound: core: Avoid using 
timespec
for struct snd_pcm_status').


## x86 architecture and i386 ABI support
CONFIG_X86_32=y

```
sound/core/pcm_native.c: In function 'snd_pcm_common_ioctl':
sound/core/pcm_native.c:3065:2: error: duplicate case value
   case SNDRV_PCM_IOCTL_SYNC_PTR64:
   ^~~~
sound/core/pcm_native.c:3062:2: error: previously used here
   case SNDRV_PCM_IOCTL_SYNC_PTR32:

```

This error comes from a commit c0513348a7b39 ('sound: core: Avoid using
timespec for struct snd_pcm_sync_ptr').


Your patchset brought conflicts to 'for-next' branch in a repository
which Iwai-san maintains[1]. I rebased your patchset on a commit 
729fbfc92a45 ('ALSA: line6: add support for POD HD DESKTOP') which is a 
HEAD of 'for-next' branch and pushed into my repository on github[2].


I respect your work for this issue, however it's better to check whether
your patchset is buildable or not on major configurations before
posting.


I note that at a development period for v4.5 kernel, ALSA developers
(mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
a rough set of test for ioctl command[3] to check his work. The set will
partly help your work, I think (but it's really rough).

I need more time for reviewing. At least, this week is for recovery from
my tough work to rewrite aplay[4].


[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
[2] https://github.com/takaswie/sound/tree/topic/year2038-rfc1
[3] https://github.com/takaswie/alsa-ioctl-test/
[4] [alsa-devel] [RFCv2][PATCH 00/38] alsa-utils: axfer: rewrite aplay
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125574.html

Thanks

Takashi Sakamoto

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Baolin Wang]

Hi Takashi,

On 22 September 2017 at 12:07, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
> Hi,
>
>
> On Sep 21 2017 15:18, Baolin Wang wrote:
>>
>> Since many structures will use timespec type variables to record time
>> stamp
>> in uapi/asound.h, which are not year 2038 safe on 32bit system. This
>> patchset
>> tries to introduce new structures removing timespec type to compatible
>> native
>> mode and compat mode.
>>
>> Moreover this patchset also converts the internal structrures to use
>> timespec64
>> type and related APIs.
>>
>> Baolin Wang (7):
>>    sound: Replace timespec with timespec64
>>    sound: core: Avoid using timespec for struct snd_pcm_status
>>    sound: core: Avoid using timespec for struct snd_pcm_sync_ptr
>>    sound: core: Avoid using timespec for struct snd_rawmidi_status
>>    sound: core: Avoid using timespec for struct snd_timer_status
>>    uapi: sound: Avoid using timespec for struct snd_ctl_elem_value
>>    sound: core: Avoid using timespec for struct snd_timer_tread
>>
>>   include/sound/pcm.h               |  113 ++++++++-
>>   include/sound/timer.h             |    4 +-
>>   include/uapi/sound/asound.h       |   15 +-
>>   sound/core/pcm.c                  |   14 +-
>>   sound/core/pcm_compat.c           |  466
>> +++++++++++++++++++++++++++++--------
>>   sound/core/pcm_lib.c              |   33 +--
>>   sound/core/pcm_native.c           |  227 ++++++++++++++----
>>   sound/core/rawmidi.c              |   74 +++++-
>>   sound/core/rawmidi_compat.c       |   90 +++++--
>>   sound/core/timer.c                |  247 ++++++++++++++++----
>>   sound/core/timer_compat.c         |   25 +-
>>   sound/pci/hda/hda_controller.c    |   10 +-
>>   sound/soc/intel/skylake/skl-pcm.c |    4 +-
>>   13 files changed, 1046 insertions(+), 276 deletions(-)
>
>
> I'm a minor Takashi in this subsystem and not those who you'd like to
> talk about this issue. But I have interests in it and would like to
> assist you, as long as I can do for it.

Thanks a lot.

>
> As a nitpicking, your patchset brings compilation error at
> configurations for x86, and x86-64 with x32 ABI support.
>
>
> ## x86-64 architecture and amd64 ABI support
> CONFIG_64BIT=y
> CONFIG_X86_64=y
>
> Success.
>
>
> ## x86-64 architecture and amd64/x32 ABI support
> CONFIG_64BIT=y
> CONFIG_X86_64=y
> CONFIG_X86_X32=y
>
> ```
> sound/core/timer_compat.c:124:54: error: array type has incomplete element
> type 'struct snd_timer_status32'
>   SNDRV_TIMER_IOCTL_STATUS32 = _IOW('T', 0x14, struct snd_timer_status32),
> ```
>
> This error comes from a commit 1229cccbefe7 ('sound: core: Avoid using
> timespec for struct snd_timer_status').
>
> ```
> sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_compat':
> sound/core/pcm_compat.c:623:9: error: assignment from incompatible pointer
> type [-Werror=incompatible-pointer-types]
>   status = runtime->status;
>          ^
> sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_sync_ptr_x32':
> sound/core/pcm_compat.c:711:9: error: assignment from incompatible pointer
> type [-Werror=incompatible-pointer-types]
>   status = runtime->status;
> ```
>
> This error comes from a commit 947c463adc00('sound: core: Avoid using
> timespec
> for struct snd_pcm_status').
>
>
> ## x86 architecture and i386 ABI support
> CONFIG_X86_32=y
>
> ```
> sound/core/pcm_native.c: In function 'snd_pcm_common_ioctl':
> sound/core/pcm_native.c:3065:2: error: duplicate case value
>   case SNDRV_PCM_IOCTL_SYNC_PTR64:
>   ^~~~
> sound/core/pcm_native.c:3062:2: error: previously used here
>   case SNDRV_PCM_IOCTL_SYNC_PTR32:
>
> ```
>
> This error comes from a commit c0513348a7b39 ('sound: core: Avoid using
> timespec for struct snd_pcm_sync_ptr').
>
>
> Your patchset brought conflicts to 'for-next' branch in a repository
> which Iwai-san maintains[1]. I rebased your patchset on a commit
> 729fbfc92a45 ('ALSA: line6: add support for POD HD DESKTOP') which is a HEAD
> of 'for-next' branch and pushed into my repository on github[2].
>
>
> I respect your work for this issue, however it's better to check whether
> your patchset is buildable or not on major configurations before
> posting.

Sorry for the building errors, since I can not build CONFIG_COMPAT
mode on my arm32 platform. But I will try to fix these build errors in
next version. This RFC patchset, I just want to show how to fix the
y2038 issue and to see if it is on the correct way. Sorry for the
building errors again.

>
> I note that at a development period for v4.5 kernel, ALSA developers
> (mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
> a rough set of test for ioctl command[3] to check his work. The set will
> partly help your work, I think (but it's really rough).

Ah, thanks.

>
> I need more time for reviewing. At least, this week is for recovery from
> my tough work to rewrite aplay[4].

Understood. Very appreciated for your comments.

>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> [2] https://github.com/takaswie/sound/tree/topic/year2038-rfc1
> [3] https://github.com/takaswie/alsa-ioctl-test/
> [4] [alsa-devel] [RFCv2][PATCH 00/38] alsa-utils: axfer: rewrite aplay
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-September/125574.html
>
> Thanks
>
> Takashi Sakamoto



-- 
Baolin.wang
Best Regards

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

On Fri, Sep 22, 2017 at 01:07:37PM +0900, Takashi Sakamoto wrote:

> I note that at a development period for v4.5 kernel, ALSA developers
> (mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
> a rough set of test for ioctl command[3] to check his work. The set will
> partly help your work, I think (but it's really rough).

Might it be worth trying to get these added to kselftest?  Seems like
the sort of thing that fits well there and it'd make it more
discoverable.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Fri, Sep 22, 2017 at 01:07:37PM +0900, Takashi Sakamoto wrote:

> I note that at a development period for v4.5 kernel, ALSA developers
> (mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
> a rough set of test for ioctl command[3] to check his work. The set will
> partly help your work, I think (but it's really rough).

Might it be worth trying to get these added to kselftest?  Seems like
the sort of thing that fits well there and it'd make it more
discoverable.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Takashi Iwai]

On Fri, 22 Sep 2017 11:15:05 +0200,
Mark Brown wrote:
> 
> On Fri, Sep 22, 2017 at 01:07:37PM +0900, Takashi Sakamoto wrote:
> 
> > I note that at a development period for v4.5 kernel, ALSA developers
> > (mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
> > a rough set of test for ioctl command[3] to check his work. The set will
> > partly help your work, I think (but it's really rough).
> 
> Might it be worth trying to get these added to kselftest?  Seems like
> the sort of thing that fits well there and it'd make it more
> discoverable.

Yes, that sounds like a sensible option.
Some ioctl sanity checks can be done well without hardware, too.


thanks,

Takashi

Re: [RFC PATCH 0/7] Fix year 2038 issue for sound subsystem

[Takashi Iwai]

On Fri, 22 Sep 2017 11:15:05 +0200,
Mark Brown wrote:
> 
> On Fri, Sep 22, 2017 at 01:07:37PM +0900, Takashi Sakamoto wrote:
> 
> > I note that at a development period for v4.5 kernel, ALSA developers
> > (mainly Iwai-san) fixed x32 ABI compatibility bugs. Then I prepared for
> > a rough set of test for ioctl command[3] to check his work. The set will
> > partly help your work, I think (but it's really rough).
> 
> Might it be worth trying to get these added to kselftest?  Seems like
> the sort of thing that fits well there and it'd make it more
> discoverable.

Yes, that sounds like a sensible option.
Some ioctl sanity checks can be done well without hardware, too.


thanks,

Takashi