[RFC PATCH 1/3] ALSA: pcm: refactor copy from/to user in SNDRV_PCM_IOCTL_SYNC_PTR

[RFC PATCH 1/3] ALSA: pcm: refactor copy from/to user in SNDRV_PCM_IOCTL_SYNC_PTR

[Christophe Leroy]

In an effort of optimising SNDRV_PCM_IOCTL_SYNC_PTR ioctl which
is a hot path, lets first refactor the copy from and to user
with macros.

This is done with macros and not static inline fonctions because
types differs between the different versions of snd_pcm_sync_ptr()
like functions.

First step is to refactor only snd_pcm_ioctl_sync_ptr_compat() and
snd_pcm_ioctl_sync_ptr_x32() as it would be a performance
regression for snd_pcm_sync_ptr() and snd_pcm_ioctl_sync_ptr_buggy()
for now. They may be refactored after next patch.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 sound/core/pcm_compat.c | 14 ++------------
 sound/core/pcm_native.c | 42 +++++++++++++++++++++++++++++------------
 2 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index a42ec7f5a1da..17540020ac2f 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -418,9 +418,7 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
 	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))
+	if (snd_pcm_sync_ptr_get_user(sflags, scontrol, src))
 		return -EFAULT;
 	if (sflags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
 		err = snd_pcm_hwsync(substream);
@@ -450,15 +448,7 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
 	}
 	if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL))
 		snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE);
-	if (put_user(sstatus.state, &src->s.status.state) ||
-	    put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) ||
-	    put_user(sstatus.tstamp.tv_sec, &src->s.status.tstamp_sec) ||
-	    put_user(sstatus.tstamp.tv_nsec, &src->s.status.tstamp_nsec) ||
-	    put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
-	    put_user(sstatus.audio_tstamp.tv_sec, &src->s.status.audio_tstamp_sec) ||
-	    put_user(sstatus.audio_tstamp.tv_nsec, &src->s.status.audio_tstamp_nsec) ||
-	    put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
-	    put_user(scontrol.avail_min, &src->c.control.avail_min))
+	if (snd_pcm_sync_ptr_put_user(sstatus, scontrol, src))
 		return -EFAULT;
 
 	return 0;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ecb71bf1859d..2ea31df0c46d 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3052,6 +3052,34 @@ static inline int snd_pcm_hwsync(struct snd_pcm_substream *substream)
 	return snd_pcm_delay(substream, NULL);
 }
 
+#define snd_pcm_sync_ptr_get_user(__f, __c, __ptr) ({				\
+	int err = 0;								\
+	typeof(*__ptr) __user *__src = (__ptr);					\
+										\
+	if (get_user(__f, &src->flags) ||					\
+	    get_user(__c.appl_ptr, &__src->c.control.appl_ptr) ||		\
+	    get_user(__c.avail_min, &__src->c.control.avail_min))		\
+		err = -EFAULT;							\
+	err;									\
+})
+
+#define snd_pcm_sync_ptr_put_user(__s, __c, __ptr) ({				\
+	int err = 0;								\
+	typeof(*__ptr) __user *__src = (__ptr);					\
+										\
+	if (put_user(__s.state, &__src->s.status.state) ||			\
+	    put_user(__s.hw_ptr, &__src->s.status.hw_ptr) ||			\
+	    put_user(__s.tstamp.tv_sec, &__src->s.status.tstamp_sec) ||	\
+	    put_user(__s.tstamp.tv_nsec, &__src->s.status.tstamp_nsec) ||	\
+	    put_user(__s.suspended_state, &__src->s.status.suspended_state) ||\
+	    put_user(__s.audio_tstamp.tv_sec, &__src->s.status.audio_tstamp_sec) ||\
+	    put_user(__s.audio_tstamp.tv_nsec, &__src->s.status.audio_tstamp_nsec) ||\
+	    put_user(__c.appl_ptr, &__src->c.control.appl_ptr) ||		\
+	    put_user(__c.avail_min, &__src->c.control.avail_min))		\
+		err = -EFAULT;							\
+	err;									\
+})
+
 static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			    struct snd_pcm_sync_ptr __user *_sync_ptr)
 {
@@ -3165,9 +3193,7 @@ static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream,
 	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))
+	if (snd_pcm_sync_ptr_get_user(sflags, scontrol, src))
 		return -EFAULT;
 	if (sflags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
 		err = snd_pcm_hwsync(substream);
@@ -3200,15 +3226,7 @@ static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream,
 	}
 	if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL))
 		snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE);
-	if (put_user(sstatus.state, &src->s.status.state) ||
-	    put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) ||
-	    put_user(sstatus.tstamp.tv_sec, &src->s.status.tstamp_sec) ||
-	    put_user(sstatus.tstamp.tv_nsec, &src->s.status.tstamp_nsec) ||
-	    put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
-	    put_user(sstatus.audio_tstamp.tv_sec, &src->s.status.audio_tstamp_sec) ||
-	    put_user(sstatus.audio_tstamp.tv_nsec, &src->s.status.audio_tstamp_nsec) ||
-	    put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
-	    put_user(scontrol.avail_min, &src->c.control.avail_min))
+	if (snd_pcm_sync_ptr_put_user(sstatus, scontrol, src))
 		return -EFAULT;
 
 	return 0;
-- 
2.47.0

[RFC PATCH 2/3] ALSA: pcm: Convert SNDRV_PCM_IOCTL_SYNC_PTR to user_access_begin/user_access_end()

[Christophe Leroy]

With user access protection (Called SMAP on x86 or KUAP on powerpc)
each and every call to get_user() or put_user() performs heavy
operations to unlock and lock kernel access to userspace.

SNDRV_PCM_IOCTL_SYNC_PTR is a hot path which is called really often
and needs to run as fast as possible.

To improve performance, perform user accesses by blocks using
user_access_begin/user_access_end() and unsafe_get_user()/
unsafe_put_user().

Before the patch the 9 calls to put_user() at the end of
snd_pcm_ioctl_sync_ptr_compat() imply the following set of
instructions about 9 times (access_ok - enable user - write - disable
user):
    0.00 :   c057f858:       3d 20 7f ff     lis     r9,32767
    0.29 :   c057f85c:       39 5e 00 14     addi    r10,r30,20
    0.77 :   c057f860:       61 29 ff fc     ori     r9,r9,65532
    0.32 :   c057f864:       7c 0a 48 40     cmplw   r10,r9
    0.36 :   c057f868:       41 a1 fb 58     bgt     c057f3c0 <snd_pcm_ioctl+0xbb0>
    0.30 :   c057f86c:       3d 20 dc 00     lis     r9,-9216
    1.95 :   c057f870:       7d 3a c3 a6     mtspr   794,r9
    0.33 :   c057f874:       92 8a 00 00     stw     r20,0(r10)
    0.27 :   c057f878:       3d 20 de 00     lis     r9,-8704
    0.28 :   c057f87c:       7d 3a c3 a6     mtspr   794,r9
...

A perf profile shows that in total the 9 put_user() represent 36% of
the time spent in snd_pcm_ioctl() and about 80 instructions.

With this patch everything is done in 13 instructions and represent
only 15% of the time spent in snd_pcm_ioctl():

    0.57 :   c057f5dc:       3d 20 dc 00     lis     r9,-9216
    0.98 :   c057f5e0:       7d 3a c3 a6     mtspr   794,r9
    0.16 :   c057f5e4:       92 7f 00 04     stw     r19,4(r31)
    0.63 :   c057f5e8:       93 df 00 0c     stw     r30,12(r31)
    0.16 :   c057f5ec:       93 9f 00 10     stw     r28,16(r31)
    4.95 :   c057f5f0:       92 9f 00 14     stw     r20,20(r31)
    0.19 :   c057f5f4:       92 5f 00 18     stw     r18,24(r31)
    0.49 :   c057f5f8:       92 bf 00 1c     stw     r21,28(r31)
    0.27 :   c057f5fc:       93 7f 00 20     stw     r27,32(r31)
    5.88 :   c057f600:       93 36 00 00     stw     r25,0(r22)
    0.11 :   c057f604:       93 17 00 00     stw     r24,0(r23)
    0.00 :   c057f608:       3d 20 de 00     lis     r9,-8704
    0.79 :   c057f60c:       7d 3a c3 a6     mtspr   794,r9

Note that here the access_ok() in user_write_access_begin() is skipped
because the exact same verification has already been performed at the
beginning of the fonction with the call to user_read_access_begin().

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 sound/core/pcm_native.c | 42 +++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 2ea31df0c46d..554352f546c9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3053,30 +3053,40 @@ static inline int snd_pcm_hwsync(struct snd_pcm_substream *substream)
 }
 
 #define snd_pcm_sync_ptr_get_user(__f, __c, __ptr) ({				\
-	int err = 0;								\
+	__label__ failed;							\
+	int err = -EFAULT;							\
 	typeof(*__ptr) __user *__src = (__ptr);					\
 										\
-	if (get_user(__f, &src->flags) ||					\
-	    get_user(__c.appl_ptr, &__src->c.control.appl_ptr) ||		\
-	    get_user(__c.avail_min, &__src->c.control.avail_min))		\
-		err = -EFAULT;							\
+	if (!user_read_access_begin(__src, sizeof(*__src)))			\
+		goto failed;							\
+	unsafe_get_user(__f, &__src->flags, failed);				\
+	unsafe_get_user(__c.appl_ptr, &__src->c.control.appl_ptr, failed);	\
+	unsafe_get_user(__c.avail_min, &__src->c.control.avail_min, failed);	\
+	err = 0;								\
+failed:										\
+	user_read_access_end();							\
 	err;									\
 })
 
 #define snd_pcm_sync_ptr_put_user(__s, __c, __ptr) ({				\
-	int err = 0;								\
+	__label__ failed;							\
+	int err = -EFAULT;							\
 	typeof(*__ptr) __user *__src = (__ptr);					\
 										\
-	if (put_user(__s.state, &__src->s.status.state) ||			\
-	    put_user(__s.hw_ptr, &__src->s.status.hw_ptr) ||			\
-	    put_user(__s.tstamp.tv_sec, &__src->s.status.tstamp_sec) ||	\
-	    put_user(__s.tstamp.tv_nsec, &__src->s.status.tstamp_nsec) ||	\
-	    put_user(__s.suspended_state, &__src->s.status.suspended_state) ||\
-	    put_user(__s.audio_tstamp.tv_sec, &__src->s.status.audio_tstamp_sec) ||\
-	    put_user(__s.audio_tstamp.tv_nsec, &__src->s.status.audio_tstamp_nsec) ||\
-	    put_user(__c.appl_ptr, &__src->c.control.appl_ptr) ||		\
-	    put_user(__c.avail_min, &__src->c.control.avail_min))		\
-		err = -EFAULT;							\
+	if (!user_write_access_begin(__src, sizeof(*__src)))			\
+		goto failed;							\
+	unsafe_put_user(__s.state, &__src->s.status.state, failed);		\
+	unsafe_put_user(__s.hw_ptr, &__src->s.status.hw_ptr, failed);		\
+	unsafe_put_user(__s.tstamp.tv_sec, &__src->s.status.tstamp_sec, failed);\
+	unsafe_put_user(__s.tstamp.tv_nsec, &__src->s.status.tstamp_nsec, failed);		\
+	unsafe_put_user(__s.suspended_state, &__src->s.status.suspended_state, failed);		\
+	unsafe_put_user(__s.audio_tstamp.tv_sec, &__src->s.status.audio_tstamp_sec, failed);	\
+	unsafe_put_user(__s.audio_tstamp.tv_nsec, &__src->s.status.audio_tstamp_nsec, failed);	\
+	unsafe_put_user(__c.appl_ptr, &__src->c.control.appl_ptr, failed);	\
+	unsafe_put_user(__c.avail_min, &__src->c.control.avail_min, failed);	\
+	err = 0;								\
+failed:										\
+	user_write_access_end();						\
 	err;									\
 })
 
-- 
2.47.0

[RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Christophe Leroy]

Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
are converted to user_access_begin/user_access_end(),
snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
followed by a copy_from_user(). And because copy_{to/from}_user() are
generic functions focussed on transfer of big data blocks to/from user,
snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
data.

So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
snd_pcm_sync_ptr() too.

In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
struct __snd_timespec.

snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
wouldn't be straigh-forward do to the workaround it provides.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 sound/core/pcm_native.c | 52 +++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 554352f546c9..31ddfdb0edfa 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3077,11 +3077,11 @@ failed:										\
 		goto failed;							\
 	unsafe_put_user(__s.state, &__src->s.status.state, failed);		\
 	unsafe_put_user(__s.hw_ptr, &__src->s.status.hw_ptr, failed);		\
-	unsafe_put_user(__s.tstamp.tv_sec, &__src->s.status.tstamp_sec, failed);\
-	unsafe_put_user(__s.tstamp.tv_nsec, &__src->s.status.tstamp_nsec, failed);		\
+	unsafe_put_user(__s.tstamp.tv_sec, &__src->s.status.tstamp.tv_sec, failed);		\
+	unsafe_put_user(__s.tstamp.tv_nsec, &__src->s.status.tstamp.tv_nsec, failed);		\
 	unsafe_put_user(__s.suspended_state, &__src->s.status.suspended_state, failed);		\
-	unsafe_put_user(__s.audio_tstamp.tv_sec, &__src->s.status.audio_tstamp_sec, failed);	\
-	unsafe_put_user(__s.audio_tstamp.tv_nsec, &__src->s.status.audio_tstamp_nsec, failed);	\
+	unsafe_put_user(__s.audio_tstamp.tv_sec, &__src->s.status.audio_tstamp.tv_sec, failed);	\
+	unsafe_put_user(__s.audio_tstamp.tv_nsec, &__src->s.status.audio_tstamp.tv_nsec, failed);\
 	unsafe_put_user(__c.appl_ptr, &__src->c.control.appl_ptr, failed);	\
 	unsafe_put_user(__c.avail_min, &__src->c.control.avail_min, failed);	\
 	err = 0;								\
@@ -3094,45 +3094,43 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			    struct snd_pcm_sync_ptr __user *_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_control *control;
+	u32 sflags;
+	struct snd_pcm_mmap_control scontrol;
+	struct snd_pcm_mmap_status sstatus;
 	int err;
 
-	memset(&sync_ptr, 0, sizeof(sync_ptr));
-	if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags)))
+	if (snd_pcm_sync_ptr_get_user(sflags, scontrol, _sync_ptr))
 		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 (sflags & SNDRV_PCM_SYNC_PTR_HWSYNC) {
 		err = snd_pcm_hwsync(substream);
 		if (err < 0)
 			return err;
 	}
 	scoped_guard(pcm_stream_lock_irq, substream) {
-		if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
-			err = pcm_lib_apply_appl_ptr(substream,
-						     sync_ptr.c.control.appl_ptr);
+		if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) {
+			err = pcm_lib_apply_appl_ptr(substream, scontrol.appl_ptr);
 			if (err < 0)
 				return err;
 		} else {
-			sync_ptr.c.control.appl_ptr = control->appl_ptr;
+			scontrol.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 (!(sflags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
+			control->avail_min = scontrol.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.s.status.audio_tstamp = status->audio_tstamp;
+			scontrol.avail_min = control->avail_min;
+		sstatus.state = status->state;
+		sstatus.hw_ptr = status->hw_ptr;
+		sstatus.tstamp = status->tstamp;
+		sstatus.suspended_state = status->suspended_state;
+		sstatus.audio_tstamp = status->audio_tstamp;
 	}
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
+	if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL))
 		snd_pcm_dma_buffer_sync(substream, SNDRV_DMA_SYNC_DEVICE);
-	if (copy_to_user(_sync_ptr, &sync_ptr, sizeof(sync_ptr)))
+	if (snd_pcm_sync_ptr_put_user(sstatus, scontrol, _sync_ptr))
 		return -EFAULT;
 	return 0;
 }
@@ -3141,11 +3139,9 @@ struct snd_pcm_mmap_status32 {
 	snd_pcm_state_t state;
 	s32 pad1;
 	u32 hw_ptr;
-	s32 tstamp_sec;
-	s32 tstamp_nsec;
+	struct __snd_timespec tstamp;
 	snd_pcm_state_t suspended_state;
-	s32 audio_tstamp_sec;
-	s32 audio_tstamp_nsec;
+	struct __snd_timespec audio_tstamp;
 } __packed;
 
 struct snd_pcm_mmap_control32 {
-- 
2.47.0

Re: [RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Takashi Iwai]

On Thu, 12 Jun 2025 12:51:05 +0200,
Christophe Leroy wrote:
> 
> Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
> are converted to user_access_begin/user_access_end(),
> snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
> followed by a copy_from_user(). And because copy_{to/from}_user() are
> generic functions focussed on transfer of big data blocks to/from user,
> snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
> data.
> 
> So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
> snd_pcm_sync_ptr() too.
> 
> In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
> replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
> struct __snd_timespec.
> 
> snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
> wouldn't be straigh-forward do to the workaround it provides.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Through a quick glance, all patches look almost fine, but one favor to
ask: this patch contains the convert from s32/s32 pair to struct
__snd_timespec.  It should be factored out to a prerequisite patch
instead of burying in a big change.

I'm asking it because this timepsec definition is very confusing (and
complex) due to historical reasons, and it should be handled with a
special care.
IIUC, struct __snd_timespec is always s32/s32 for the kernel code, so
the conversion must be fine.  This needs to be commented in the
commit.


Thanks!

Takashi

Re: [RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Christophe Leroy]

Le 13/06/2025 à 11:29, Takashi Iwai a écrit :
> On Thu, 12 Jun 2025 12:51:05 +0200,
> Christophe Leroy wrote:
>>
>> Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
>> are converted to user_access_begin/user_access_end(),
>> snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
>> followed by a copy_from_user(). And because copy_{to/from}_user() are
>> generic functions focussed on transfer of big data blocks to/from user,
>> snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
>> data.
>>
>> So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
>> snd_pcm_sync_ptr() too.
>>
>> In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
>> replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
>> struct __snd_timespec.
>>
>> snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
>> wouldn't be straigh-forward do to the workaround it provides.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> Through a quick glance, all patches look almost fine, but one favor to
> ask: this patch contains the convert from s32/s32 pair to struct
> __snd_timespec.  It should be factored out to a prerequisite patch
> instead of burying in a big change.

Shall I understand you prefer this series over the more simple "ALSA: 
pcm: Convert snd_pcm_ioctl_sync_ptr_{compat/x32} to 
user_access_begin/user_access_end()" patch ?

I'm asking because I was myself not sure about the benefit of such two 
big macros over the other proposal in terms of readability.


> 
> I'm asking it because this timepsec definition is very confusing (and
> complex) due to historical reasons, and it should be handled with a
> special care.
> IIUC, struct __snd_timespec is always s32/s32 for the kernel code, so
> the conversion must be fine.  This needs to be commented in the
> commit.


Sure I will do that.

Christophe

Re: [RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Takashi Iwai]

On Fri, 13 Jun 2025 13:03:04 +0200,
Christophe Leroy wrote:
> 
> 
> 
> Le 13/06/2025 à 11:29, Takashi Iwai a écrit :
> > On Thu, 12 Jun 2025 12:51:05 +0200,
> > Christophe Leroy wrote:
> >> 
> >> Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
> >> are converted to user_access_begin/user_access_end(),
> >> snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
> >> followed by a copy_from_user(). And because copy_{to/from}_user() are
> >> generic functions focussed on transfer of big data blocks to/from user,
> >> snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
> >> data.
> >> 
> >> So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
> >> snd_pcm_sync_ptr() too.
> >> 
> >> In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
> >> replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
> >> struct __snd_timespec.
> >> 
> >> snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
> >> wouldn't be straigh-forward do to the workaround it provides.
> >> 
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > 
> > Through a quick glance, all patches look almost fine, but one favor to
> > ask: this patch contains the convert from s32/s32 pair to struct
> > __snd_timespec.  It should be factored out to a prerequisite patch
> > instead of burying in a big change.
> 
> Shall I understand you prefer this series over the more simple "ALSA:
> pcm: Convert snd_pcm_ioctl_sync_ptr_{compat/x32} to
> user_access_begin/user_access_end()" patch ?

Err, no, sorry for ambiguity.
I wanted to move the replacement of tstamp_sec/nsec with struct
__snd_timespec as a small preliminary patch from patch#3.
That is,
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3103,11 +3103,9 @@ struct snd_pcm_mmap_status32 {
 	snd_pcm_state_t state;
 	s32 pad1;
 	u32 hw_ptr;
-	s32 tstamp_sec;
-	s32 tstamp_nsec;
+	struct __snd_timespec tstamp;
 	snd_pcm_state_t suspended_state;
-	s32 audio_tstamp_sec;
-	s32 audio_tstamp_nsec;
+	struct __snd_timespec audio_tstamp;
 } __packed;
etc.  By factoring this out, it becomes clear that the timespec
compatibility is fully cared.

__snd_timespec may be defined in different ways on user-space, but in
the kernel code, it's a single definition of s32/s32 pair.  This needs
to be emphasized.


thanks,

Takashi

Re: [RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Christophe Leroy]

Le 13/06/2025 à 14:37, Takashi Iwai a écrit :
> On Fri, 13 Jun 2025 13:03:04 +0200,
> Christophe Leroy wrote:
>>
>>
>>
>> Le 13/06/2025 à 11:29, Takashi Iwai a écrit :
>>> On Thu, 12 Jun 2025 12:51:05 +0200,
>>> Christophe Leroy wrote:
>>>>
>>>> Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
>>>> are converted to user_access_begin/user_access_end(),
>>>> snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
>>>> followed by a copy_from_user(). And because copy_{to/from}_user() are
>>>> generic functions focussed on transfer of big data blocks to/from user,
>>>> snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
>>>> data.
>>>>
>>>> So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
>>>> snd_pcm_sync_ptr() too.
>>>>
>>>> In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
>>>> replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
>>>> struct __snd_timespec.
>>>>
>>>> snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
>>>> wouldn't be straigh-forward do to the workaround it provides.
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>
>>> Through a quick glance, all patches look almost fine, but one favor to
>>> ask: this patch contains the convert from s32/s32 pair to struct
>>> __snd_timespec.  It should be factored out to a prerequisite patch
>>> instead of burying in a big change.
>>
>> Shall I understand you prefer this series over the more simple "ALSA:
>> pcm: Convert snd_pcm_ioctl_sync_ptr_{compat/x32} to
>> user_access_begin/user_access_end()" patch ?
> 
> Err, no, sorry for ambiguity.

Then I'm lost.

I sent two alternative proposals:
A/ Single patch, simple, handling only two fonctions 
snd_pcm_ioctl_sync_ptr_{compat/x32} , without refactoring. [1]
B/ This RFC series, more elaborate, refactoring and putting user copy 
into helper macros. [2]

So the question was to be sure you prefer alternative B over alternative 
A. I guess the answer is YES as you asking me improve it.

[1] 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/8df11af98033e4cb4d9b0f16d6e9d5b69110b036.1749724057.git.christophe.leroy@csgroup.eu/
[2] 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?state=*&series=460665


> I wanted to move the replacement of tstamp_sec/nsec with struct
> __snd_timespec as a small preliminary patch from patch#3.
> That is,

Yes that's what I understood.

Thanks
Christophe


> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3103,11 +3103,9 @@ struct snd_pcm_mmap_status32 {
>   	snd_pcm_state_t state;
>   	s32 pad1;
>   	u32 hw_ptr;
> -	s32 tstamp_sec;
> -	s32 tstamp_nsec;
> +	struct __snd_timespec tstamp;
>   	snd_pcm_state_t suspended_state;
> -	s32 audio_tstamp_sec;
> -	s32 audio_tstamp_nsec;
> +	struct __snd_timespec audio_tstamp;
>   } __packed;
> etc.  By factoring this out, it becomes clear that the timespec
> compatibility is fully cared.
> 
> __snd_timespec may be defined in different ways on user-space, but in
> the kernel code, it's a single definition of s32/s32 pair.  This needs
> to be emphasized.
> 
> 
> thanks,
> 
> Takashi

Re: [RFC PATCH 3/3] ALSA: pcm: Convert snd_pcm_sync_ptr() to user_access_begin/user_access_end()

[Takashi Iwai]

On Fri, 13 Jun 2025 14:46:46 +0200,
Christophe Leroy wrote:
> 
> 
> 
> Le 13/06/2025 à 14:37, Takashi Iwai a écrit :
> > On Fri, 13 Jun 2025 13:03:04 +0200,
> > Christophe Leroy wrote:
> >> 
> >> 
> >> 
> >> Le 13/06/2025 à 11:29, Takashi Iwai a écrit :
> >>> On Thu, 12 Jun 2025 12:51:05 +0200,
> >>> Christophe Leroy wrote:
> >>>> 
> >>>> Now that snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user()
> >>>> are converted to user_access_begin/user_access_end(),
> >>>> snd_pcm_sync_ptr_get_user() is more efficient than a raw get_user()
> >>>> followed by a copy_from_user(). And because copy_{to/from}_user() are
> >>>> generic functions focussed on transfer of big data blocks to/from user,
> >>>> snd_pcm_sync_ptr_put_user() is also more efficient for small amont of
> >>>> data.
> >>>> 
> >>>> So use snd_pcm_sync_ptr_get_user() and snd_pcm_sync_ptr_put_user() in
> >>>> snd_pcm_sync_ptr() too.
> >>>> 
> >>>> In order to have snd_pcm_mmap_status32 similar to snd_pcm_mmap_status,
> >>>> replace to tsamp_{sec/nsec} and audio_tstamp_{sec/nsec} by equivalent
> >>>> struct __snd_timespec.
> >>>> 
> >>>> snd_pcm_ioctl_sync_ptr_buggy() is left as it is because the conversion
> >>>> wouldn't be straigh-forward do to the workaround it provides.
> >>>> 
> >>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >>> 
> >>> Through a quick glance, all patches look almost fine, but one favor to
> >>> ask: this patch contains the convert from s32/s32 pair to struct
> >>> __snd_timespec.  It should be factored out to a prerequisite patch
> >>> instead of burying in a big change.
> >> 
> >> Shall I understand you prefer this series over the more simple "ALSA:
> >> pcm: Convert snd_pcm_ioctl_sync_ptr_{compat/x32} to
> >> user_access_begin/user_access_end()" patch ?
> > 
> > Err, no, sorry for ambiguity.
> 
> Then I'm lost.
> 
> I sent two alternative proposals:
> A/ Single patch, simple, handling only two fonctions
> snd_pcm_ioctl_sync_ptr_{compat/x32} , without refactoring. [1]
> B/ This RFC series, more elaborate, refactoring and putting user copy
> into helper macros. [2]
> 
> So the question was to be sure you prefer alternative B over
> alternative A. I guess the answer is YES as you asking me improve it.

Right, let's go with the RFC series with refactoring.


thanks,

Takashi

> 
> [1]
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/8df11af98033e4cb4d9b0f16d6e9d5b69110b036.1749724057.git.christophe.leroy@csgroup.eu/
> [2]
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?state=*&series=460665
> 
> 
> > I wanted to move the replacement of tstamp_sec/nsec with struct
> > __snd_timespec as a small preliminary patch from patch#3.
> > That is,
> 
> Yes that's what I understood.
> 
> Thanks
> Christophe
> 
> 
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -3103,11 +3103,9 @@ struct snd_pcm_mmap_status32 {
> >   	snd_pcm_state_t state;
> >   	s32 pad1;
> >   	u32 hw_ptr;
> > -	s32 tstamp_sec;
> > -	s32 tstamp_nsec;
> > +	struct __snd_timespec tstamp;
> >   	snd_pcm_state_t suspended_state;
> > -	s32 audio_tstamp_sec;
> > -	s32 audio_tstamp_nsec;
> > +	struct __snd_timespec audio_tstamp;
> >   } __packed;
> > etc.  By factoring this out, it becomes clear that the timespec
> > compatibility is fully cared.
> > 
> > __snd_timespec may be defined in different ways on user-space, but in
> > the kernel code, it's a single definition of s32/s32 pair.  This needs
> > to be emphasized.
> > 
> > 
> > thanks,
> > 
> > Takashi
>