[PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Takashi Iwai]

Hi,

this is a patch set to clean up the PCM copy ops using sockptr_t as a
"universal" pointer, inspired by the recent patch from Andy
Shevchenko:
  https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com

Even though it sounds a bit weird, sockptr_t is a generic type that is
used already in wide ranges, and it can fit our purpose, too.  With
sockptr_t, the former split of copy_user and copy_kernel PCM ops can
be unified again gracefully.

The patch set introduces the new PCM ops, converting users, and drops
the old PCM ops.  Most of conversions are straightforward, simply
replacing copy_*_user() with copy_*_sockptr() variants.

Note that the conversion in ASoC will fix a potential problem of ASoC
PCM that has been for long time.  Since ASoC component takes care of
only copy_user, the conversion form/to kernel space might have been
missing.  With this patch set, both cases are handled with sockptr_t
by a single callback.

The patches are lightly tested (with a faked PCM copy implementation
on HD-audio), while most of patches are only compile-tested.


Takashi

===

Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Andrey Utkin <andrey_utkin@fastmail.com>
Cc: Anton Sviridenko <anton@corp.bluecherry.net>
Cc: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Cc: Banajit Goswami <bgoswami@quicinc.com>
Cc: Bluecherry Maintainers <maintainers@bluecherrydvr.com>
Cc: Claudiu Beznea <claudiu.beznea@microchip.com>
Cc: Ismael Luceno <ismael@iodev.co.uk>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Cc: Olivier Moysan <olivier.moysan@foss.st.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: linux-media@vger.kernel.org
Cc: xen-devel@lists.xenproject.org

===

Takashi Iwai (24):
  ALSA: pcm: Add copy ops with universal sockptr_t
  ALSA: core: Add memory copy helpers between sockptr and iomem
  ALSA: dummy: Convert to generic PCM copy ops
  ALSA: gus: Convert to generic PCM copy ops
  ALSA: emu8000: Convert to generic PCM copy ops
  ALSA: es1938: Convert to generic PCM copy ops
  ALSA: korg1212: Convert to generic PCM copy ops
  ALSA: nm256: Convert to generic PCM copy ops
  ALSA: rme32: Convert to generic PCM copy ops
  ALSA: rme96: Convert to generic PCM copy ops
  ALSA: hdsp: Convert to generic PCM copy ops
  ALSA: rme9652: Convert to generic PCM copy ops
  ALSA: sh: Convert to generic PCM copy ops
  ALSA: xen: Convert to generic PCM copy ops
  ALSA: pcmtest: Update comment about PCM copy ops
  media: solo6x10: Convert to generic PCM copy ops
  ASoC: component: Add generic PCM copy ops
  ASoC: mediatek: Convert to generic PCM copy ops
  ASoC: qcom: Convert to generic PCM copy ops
  ASoC: dmaengine: Convert to generic PCM copy ops
  ASoC: dmaengine: Use sockptr_t for process callback, too
  ALSA: doc: Update description for the new PCM copy ops
  ASoC: pcm: Drop obsoleted PCM copy_user ops
  ALSA: pcm: Drop obsoleted PCM copy_user and copy_kernel ops

 .../kernel-api/writing-an-alsa-driver.rst     | 59 +++++---------
 drivers/media/pci/solo6x10/solo6x10-g723.c    | 41 ++--------
 include/sound/dmaengine_pcm.h                 |  2 +-
 include/sound/pcm.h                           | 12 +--
 include/sound/soc-component.h                 | 14 ++--
 sound/core/memory.c                           | 39 +++++++++
 sound/core/pcm_lib.c                          | 81 +++++++++----------
 sound/core/pcm_native.c                       |  2 +-
 sound/drivers/dummy.c                         | 12 +--
 sound/drivers/pcmtest.c                       |  2 +-
 sound/isa/gus/gus_pcm.c                       | 23 +-----
 sound/isa/sb/emu8000_pcm.c                    | 79 +++++-------------
 sound/pci/es1938.c                            | 31 ++-----
 sound/pci/korg1212/korg1212.c                 | 46 +++--------
 sound/pci/nm256/nm256.c                       | 42 ++--------
 sound/pci/rme32.c                             | 50 +++---------
 sound/pci/rme96.c                             | 48 +++--------
 sound/pci/rme9652/hdsp.c                      | 42 ++--------
 sound/pci/rme9652/rme9652.c                   | 46 ++---------
 sound/sh/sh_dac_audio.c                       | 25 +-----
 sound/soc/atmel/mchp-pdmc.c                   |  2 +-
 sound/soc/mediatek/common/mtk-btcvsd.c        | 22 ++---
 sound/soc/qcom/lpass-platform.c               | 12 +--
 sound/soc/soc-component.c                     | 10 +--
 sound/soc/soc-generic-dmaengine-pcm.c         | 18 ++---
 sound/soc/soc-pcm.c                           |  4 +-
 sound/soc/stm/stm32_sai_sub.c                 |  2 +-
 sound/xen/xen_snd_front_alsa.c                | 55 +++----------
 28 files changed, 251 insertions(+), 570 deletions(-)

-- 
2.35.3

[PATCH 16/24] media: solo6x10: Convert to generic PCM copy ops

[Takashi Iwai]

This patch converts the solo6x10 driver code to use the new unified
PCM copy callback.  It's a straightforward conversion from *_user() to
*_sockptr() variants.

Cc: Bluecherry Maintainers <maintainers@bluecherrydvr.com>
Cc: Anton Sviridenko <anton@corp.bluecherry.net>
Cc: Andrey Utkin <andrey_utkin@fastmail.com>
Cc: Ismael Luceno <ismael@iodev.co.uk>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/media/pci/solo6x10/solo6x10-g723.c | 41 +++++-----------------
 1 file changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-g723.c b/drivers/media/pci/solo6x10/solo6x10-g723.c
index 6cebad665565..cf134810b8ec 100644
--- a/drivers/media/pci/solo6x10/solo6x10-g723.c
+++ b/drivers/media/pci/solo6x10/solo6x10-g723.c
@@ -204,12 +204,13 @@ static snd_pcm_uframes_t snd_solo_pcm_pointer(struct snd_pcm_substream *ss)
 	return idx * G723_FRAMES_PER_PAGE;
 }
 
-static int snd_solo_pcm_copy_user(struct snd_pcm_substream *ss, int channel,
-				  unsigned long pos, void __user *dst,
-				  unsigned long count)
+static int snd_solo_pcm_copy(struct snd_pcm_substream *ss, int channel,
+			     unsigned long pos, sockptr_t dst,
+			     unsigned long count)
 {
 	struct solo_snd_pcm *solo_pcm = snd_pcm_substream_chip(ss);
 	struct solo_dev *solo_dev = solo_pcm->solo_dev;
+	unsigned int off = 0;
 	int err, i;
 
 	for (i = 0; i < (count / G723_FRAMES_PER_PAGE); i++) {
@@ -223,35 +224,10 @@ static int snd_solo_pcm_copy_user(struct snd_pcm_substream *ss, int channel,
 		if (err)
 			return err;
 
-		if (copy_to_user(dst, solo_pcm->g723_buf, G723_PERIOD_BYTES))
+		if (copy_to_sockptr_offset(dst, off,
+					   solo_pcm->g723_buf, G723_PERIOD_BYTES))
 			return -EFAULT;
-		dst += G723_PERIOD_BYTES;
-	}
-
-	return 0;
-}
-
-static int snd_solo_pcm_copy_kernel(struct snd_pcm_substream *ss, int channel,
-				    unsigned long pos, void *dst,
-				    unsigned long count)
-{
-	struct solo_snd_pcm *solo_pcm = snd_pcm_substream_chip(ss);
-	struct solo_dev *solo_dev = solo_pcm->solo_dev;
-	int err, i;
-
-	for (i = 0; i < (count / G723_FRAMES_PER_PAGE); i++) {
-		int page = (pos / G723_FRAMES_PER_PAGE) + i;
-
-		err = solo_p2m_dma_t(solo_dev, 0, solo_pcm->g723_dma,
-				     SOLO_G723_EXT_ADDR(solo_dev) +
-				     (page * G723_PERIOD_BLOCK) +
-				     (ss->number * G723_PERIOD_BYTES),
-				     G723_PERIOD_BYTES, 0, 0);
-		if (err)
-			return err;
-
-		memcpy(dst, solo_pcm->g723_buf, G723_PERIOD_BYTES);
-		dst += G723_PERIOD_BYTES;
+		off += G723_PERIOD_BYTES;
 	}
 
 	return 0;
@@ -263,8 +239,7 @@ static const struct snd_pcm_ops snd_solo_pcm_ops = {
 	.prepare = snd_solo_pcm_prepare,
 	.trigger = snd_solo_pcm_trigger,
 	.pointer = snd_solo_pcm_pointer,
-	.copy_user = snd_solo_pcm_copy_user,
-	.copy_kernel = snd_solo_pcm_copy_kernel,
+	.copy = snd_solo_pcm_copy,
 };
 
 static int snd_solo_capture_volume_info(struct snd_kcontrol *kcontrol,
-- 
2.35.3

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Mark Brown]

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

On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote:

> this is a patch set to clean up the PCM copy ops using sockptr_t as a
> "universal" pointer, inspired by the recent patch from Andy
> Shevchenko:
>   https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com

> Even though it sounds a bit weird, sockptr_t is a generic type that is
> used already in wide ranges, and it can fit our purpose, too.  With
> sockptr_t, the former split of copy_user and copy_kernel PCM ops can
> be unified again gracefully.

It really feels like we ought to rename, or add an alias for, the type
if we're going to start using it more widely - it's not helping to make
the code clearer.

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

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Takashi Iwai]

On Mon, 31 Jul 2023 19:20:54 +0200,
Mark Brown wrote:
> 
> On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote:
> 
> > this is a patch set to clean up the PCM copy ops using sockptr_t as a
> > "universal" pointer, inspired by the recent patch from Andy
> > Shevchenko:
> >   https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com
> 
> > Even though it sounds a bit weird, sockptr_t is a generic type that is
> > used already in wide ranges, and it can fit our purpose, too.  With
> > sockptr_t, the former split of copy_user and copy_kernel PCM ops can
> > be unified again gracefully.
> 
> It really feels like we ought to rename, or add an alias for, the type
> if we're going to start using it more widely - it's not helping to make
> the code clearer.

That was my very first impression, too, but I changed my mind after
seeing the already used code.  An alias might work, either typedef or
define genptr_t or such as sockptr_t.  But we'll need to copy the
bunch of helper functions, too...


Takashi

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Mark Brown]

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

On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > It really feels like we ought to rename, or add an alias for, the type
> > if we're going to start using it more widely - it's not helping to make
> > the code clearer.

> That was my very first impression, too, but I changed my mind after
> seeing the already used code.  An alias might work, either typedef or
> define genptr_t or such as sockptr_t.  But we'll need to copy the
> bunch of helper functions, too...

I would predict that if the type becomes more widely used that'll happen
eventually and the longer it's left the more work it'll be.

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

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Takashi Iwai]

On Mon, 31 Jul 2023 21:40:20 +0200,
Mark Brown wrote:
> 
> On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > It really feels like we ought to rename, or add an alias for, the type
> > > if we're going to start using it more widely - it's not helping to make
> > > the code clearer.
> 
> > That was my very first impression, too, but I changed my mind after
> > seeing the already used code.  An alias might work, either typedef or
> > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > bunch of helper functions, too...
> 
> I would predict that if the type becomes more widely used that'll happen
> eventually and the longer it's left the more work it'll be.

That's true.  The question is how more widely it'll be used, then.

Is something like below what you had in mind, too?


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] Introduce uniptr_t as replacement of sockptr_t

Although sockptr_t is used already in several places as a "universal"
pointer, it's still too confusing as if were related with network
stuff.

Make a more generic type, uniptr_t, that does exactly as same as
sockptr_t for a wider use.  sockptr_t becomes now alias to uniptr_t.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/linux/sockptr.h | 124 +++++-----------------------------------
 include/linux/uniptr.h  | 117 +++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 109 deletions(-)
 create mode 100644 include/linux/uniptr.h

diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
index bae5e2369b4f..dc803989a4d6 100644
--- a/include/linux/sockptr.h
+++ b/include/linux/sockptr.h
@@ -1,118 +1,24 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * Copyright (c) 2020 Christoph Hellwig.
- *
- * Support for "universal" pointers that can point to either kernel or userspace
- * memory.
+ * Aliases for the old sockptr_t and its helpers for the new uniptr_t
  */
 #ifndef _LINUX_SOCKPTR_H
 #define _LINUX_SOCKPTR_H
 
-#include <linux/slab.h>
-#include <linux/uaccess.h>
+#include <linux/uniptr.h>
 
-typedef struct {
-	union {
-		void		*kernel;
-		void __user	*user;
-	};
-	bool		is_kernel : 1;
-} sockptr_t;
-
-static inline bool sockptr_is_kernel(sockptr_t sockptr)
-{
-	return sockptr.is_kernel;
-}
-
-static inline sockptr_t KERNEL_SOCKPTR(void *p)
-{
-	return (sockptr_t) { .kernel = p, .is_kernel = true };
-}
-
-static inline sockptr_t USER_SOCKPTR(void __user *p)
-{
-	return (sockptr_t) { .user = p };
-}
-
-static inline bool sockptr_is_null(sockptr_t sockptr)
-{
-	if (sockptr_is_kernel(sockptr))
-		return !sockptr.kernel;
-	return !sockptr.user;
-}
-
-static inline int copy_from_sockptr_offset(void *dst, sockptr_t src,
-		size_t offset, size_t size)
-{
-	if (!sockptr_is_kernel(src))
-		return copy_from_user(dst, src.user + offset, size);
-	memcpy(dst, src.kernel + offset, size);
-	return 0;
-}
-
-static inline int copy_from_sockptr(void *dst, sockptr_t src, size_t size)
-{
-	return copy_from_sockptr_offset(dst, src, 0, size);
-}
-
-static inline int copy_to_sockptr_offset(sockptr_t dst, size_t offset,
-		const void *src, size_t size)
-{
-	if (!sockptr_is_kernel(dst))
-		return copy_to_user(dst.user + offset, src, size);
-	memcpy(dst.kernel + offset, src, size);
-	return 0;
-}
-
-static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size)
-{
-	return copy_to_sockptr_offset(dst, 0, src, size);
-}
-
-static inline void *memdup_sockptr(sockptr_t src, size_t len)
-{
-	void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
-
-	if (!p)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_sockptr(p, src, len)) {
-		kfree(p);
-		return ERR_PTR(-EFAULT);
-	}
-	return p;
-}
-
-static inline void *memdup_sockptr_nul(sockptr_t src, size_t len)
-{
-	char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);
-
-	if (!p)
-		return ERR_PTR(-ENOMEM);
-	if (copy_from_sockptr(p, src, len)) {
-		kfree(p);
-		return ERR_PTR(-EFAULT);
-	}
-	p[len] = '\0';
-	return p;
-}
-
-static inline long strncpy_from_sockptr(char *dst, sockptr_t src, size_t count)
-{
-	if (sockptr_is_kernel(src)) {
-		size_t len = min(strnlen(src.kernel, count - 1) + 1, count);
-
-		memcpy(dst, src.kernel, len);
-		return len;
-	}
-	return strncpy_from_user(dst, src.user, count);
-}
-
-static inline int check_zeroed_sockptr(sockptr_t src, size_t offset,
-				       size_t size)
-{
-	if (!sockptr_is_kernel(src))
-		return check_zeroed_user(src.user + offset, size);
-	return memchr_inv(src.kernel + offset, 0, size) == NULL;
-}
+#define sockptr_t		uniptr_t
+#define sockptr_is_kernel	uniptr_is_kernel
+#define KERNEL_SOCKPTR		KERNEL_UNIPTR
+#define USER_SOCKPTR		USER_UNIPTR
+#define sockptr_is_null		uniptr_is_null
+#define copy_from_sockptr_offset copy_from_uniptr_offset
+#define copy_from_sockptr	copy_from_uniptr
+#define copy_to_sockptr_offset	copy_to_uniptr_offset
+#define copy_to_sockptr		copy_to_uniptr
+#define memdup_sockptr		memdup_uniptr
+#define memdup_sockptr_nul	memdup_uniptr_nul
+#define strncpy_from_sockptr	strncpy_from_uniptr
+#define check_zeroed_sockptr	check_zeroed_uniptr
 
 #endif /* _LINUX_SOCKPTR_H */
diff --git a/include/linux/uniptr.h b/include/linux/uniptr.h
new file mode 100644
index 000000000000..3ca9fc8eab4e
--- /dev/null
+++ b/include/linux/uniptr.h
@@ -0,0 +1,117 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2020 Christoph Hellwig.
+ *
+ * Support for "universal" pointers that can point to either kernel or userspace
+ * memory.
+ */
+#ifndef _LINUX_UNIPTR_H
+#define _LINUX_UNIPTR_H
+
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+typedef struct {
+	union {
+		void		*kernel;
+		void __user	*user;
+	};
+	bool		is_kernel : 1;
+} uniptr_t;
+
+static inline bool uniptr_is_kernel(uniptr_t uniptr)
+{
+	return uniptr.is_kernel;
+}
+
+static inline uniptr_t KERNEL_UNIPTR(void *p)
+{
+	return (uniptr_t) { .kernel = p, .is_kernel = true };
+}
+
+static inline uniptr_t USER_UNIPTR(void __user *p)
+{
+	return (uniptr_t) { .user = p };
+}
+
+static inline bool uniptr_is_null(uniptr_t uniptr)
+{
+	if (uniptr_is_kernel(uniptr))
+		return !uniptr.kernel;
+	return !uniptr.user;
+}
+
+static inline int copy_from_uniptr_offset(void *dst, uniptr_t src,
+					  size_t offset, size_t size)
+{
+	if (!uniptr_is_kernel(src))
+		return copy_from_user(dst, src.user + offset, size);
+	memcpy(dst, src.kernel + offset, size);
+	return 0;
+}
+
+static inline int copy_from_uniptr(void *dst, uniptr_t src, size_t size)
+{
+	return copy_from_uniptr_offset(dst, src, 0, size);
+}
+
+static inline int copy_to_uniptr_offset(uniptr_t dst, size_t offset,
+					const void *src, size_t size)
+{
+	if (!uniptr_is_kernel(dst))
+		return copy_to_user(dst.user + offset, src, size);
+	memcpy(dst.kernel + offset, src, size);
+	return 0;
+}
+
+static inline int copy_to_uniptr(uniptr_t dst, const void *src, size_t size)
+{
+	return copy_to_uniptr_offset(dst, 0, src, size);
+}
+
+static inline void *memdup_uniptr(uniptr_t src, size_t len)
+{
+	void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
+
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+	if (copy_from_uniptr(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+	return p;
+}
+
+static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
+{
+	char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);
+
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+	if (copy_from_uniptr(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+	p[len] = '\0';
+	return p;
+}
+
+static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
+{
+	if (uniptr_is_kernel(src)) {
+		size_t len = min(strnlen(src.kernel, count - 1) + 1, count);
+
+		memcpy(dst, src.kernel, len);
+		return len;
+	}
+	return strncpy_from_user(dst, src.user, count);
+}
+
+static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t size)
+{
+	if (!uniptr_is_kernel(src))
+		return check_zeroed_user(src.user + offset, size);
+	return memchr_inv(src.kernel + offset, 0, size) == NULL;
+}
+
+#endif /* _LINUX_UNIPTR_H */
-- 
2.35.3

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 19:20:54 +0200,
> Mark Brown wrote:
> > 
> > On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote:
> > 
> > > this is a patch set to clean up the PCM copy ops using sockptr_t as a
> > > "universal" pointer, inspired by the recent patch from Andy
> > > Shevchenko:
> > >   https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevchenko@linux.intel.com
> > 
> > > Even though it sounds a bit weird, sockptr_t is a generic type that is
> > > used already in wide ranges, and it can fit our purpose, too.  With
> > > sockptr_t, the former split of copy_user and copy_kernel PCM ops can
> > > be unified again gracefully.
> > 
> > It really feels like we ought to rename, or add an alias for, the type
> > if we're going to start using it more widely - it's not helping to make
> > the code clearer.
> 
> That was my very first impression, too, but I changed my mind after
> seeing the already used code.  An alias might work, either typedef or
> define genptr_t or such as sockptr_t.  But we'll need to copy the
> bunch of helper functions, too...

Maybe we should define a genptr_t (in genptr.h) and convert sockptr infra to
use it (in sockptr.h)? This will leave network and other existing users to
convert to it step-by-step.

Another approach is to simply copy sockptr.h to genptr.h with changed naming
scheme and add a deprecation note to the former.

Thank you, Takashi, for doing this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Mark Brown]

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

On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:

> That's true.  The question is how more widely it'll be used, then.

Indeed.

> Is something like below what you had in mind, too?

Yes, or Andy's suggestion of just copying without trying to put a
redirect in works for me too.  I imagine there might be some
bikeshedding of the name, your proposal seems fine to me.

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

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 21:40:20 +0200,
> Mark Brown wrote:
> > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > It really feels like we ought to rename, or add an alias for, the type
> > > > if we're going to start using it more widely - it's not helping to make
> > > > the code clearer.
> > 
> > > That was my very first impression, too, but I changed my mind after
> > > seeing the already used code.  An alias might work, either typedef or
> > > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > > bunch of helper functions, too...
> > 
> > I would predict that if the type becomes more widely used that'll happen
> > eventually and the longer it's left the more work it'll be.
> 
> That's true.  The question is how more widely it'll be used, then.
> 
> Is something like below what you had in mind, too?

I agree with your proposal (uniptr_t also works for me), but see below.

...

> +#include <linux/slab.h>
> +#include <linux/uaccess.h>

But let make the list of the headers right this time.

It seems to me that

err.h
minmax.h // maybe not, see a remark at the bottom
string.h
types.h

are missing.

More below.

...

> +	void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> +
> +	if (!p)
> +		return ERR_PTR(-ENOMEM);

This can use cleanup.h.

> +	if (copy_from_uniptr(p, src, len)) {
> +		kfree(p);
> +		return ERR_PTR(-EFAULT);
> +	}
> +	return p;
> +}
> +
> +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
> +{
> +	char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);

Ditto.

> +	if (!p)
> +		return ERR_PTR(-ENOMEM);
> +	if (copy_from_uniptr(p, src, len)) {
> +		kfree(p);
> +		return ERR_PTR(-EFAULT);
> +	}
> +	p[len] = '\0';
> +	return p;
> +}

...

> +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
> +{
> +	if (uniptr_is_kernel(src)) {
> +		size_t len = min(strnlen(src.kernel, count - 1) + 1, count);

I didn't get why do we need min()? To check the count == 0 case?

Wouldn't

		size_t len;

		len = strnlen(src.kernel, count);
		if (len == 0)
			return 0;

		/* Copy a trailing NUL if found */
		if (len < count)
			len++;

be a good equivalent?

> +		memcpy(dst, src.kernel, len);
> +		return len;
> +	}
> +	return strncpy_from_user(dst, src.user, count);
> +}

...

> +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t size)
> +{
> +	if (!uniptr_is_kernel(src))

Why not to align all the functions to use same conditional (either always
positive or negative)?

> +		return check_zeroed_user(src.user + offset, size);
> +	return memchr_inv(src.kernel + offset, 0, size) == NULL;
> +}

...

Taking all remarks into account I would rather go with sockptr.h being
untouched for now, just a big

/* DO NOT USE, it's obsolete, use uniptr.h instead! */

to be added.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Takashi Iwai]

On Tue, 01 Aug 2023 19:51:39 +0200,
Andy Shevchenko wrote:
> 
> On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:
> > On Mon, 31 Jul 2023 21:40:20 +0200,
> > Mark Brown wrote:
> > > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > > > Mark Brown wrote:
> > > 
> > > > > It really feels like we ought to rename, or add an alias for, the type
> > > > > if we're going to start using it more widely - it's not helping to make
> > > > > the code clearer.
> > > 
> > > > That was my very first impression, too, but I changed my mind after
> > > > seeing the already used code.  An alias might work, either typedef or
> > > > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > > > bunch of helper functions, too...
> > > 
> > > I would predict that if the type becomes more widely used that'll happen
> > > eventually and the longer it's left the more work it'll be.
> > 
> > That's true.  The question is how more widely it'll be used, then.
> > 
> > Is something like below what you had in mind, too?
> 
> I agree with your proposal (uniptr_t also works for me), but see below.
> 
> ...
> 
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> 
> But let make the list of the headers right this time.
> 
> It seems to me that
> 
> err.h
> minmax.h // maybe not, see a remark at the bottom
> string.h
> types.h
> 
> are missing.

OK, makes sense to add them.

> 
> More below.
> 
> ...
> 
> > +	void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> > +
> > +	if (!p)
> > +		return ERR_PTR(-ENOMEM);
> 
> This can use cleanup.h.

Hm, I don't think it can be replaced with that.
There may be more use of cleanup.h, but it's no direct alternative for
kmalloc_track_caller()...

> > +	if (copy_from_uniptr(p, src, len)) {
> > +		kfree(p);
> > +		return ERR_PTR(-EFAULT);
> > +	}
> > +	return p;
> > +}
> > +
> > +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
> > +{
> > +	char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);
> 
> Ditto.
> 
> > +	if (!p)
> > +		return ERR_PTR(-ENOMEM);
> > +	if (copy_from_uniptr(p, src, len)) {
> > +		kfree(p);
> > +		return ERR_PTR(-EFAULT);
> > +	}
> > +	p[len] = '\0';
> > +	return p;
> > +}
> 
> ...
> 
> > +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
> > +{
> > +	if (uniptr_is_kernel(src)) {
> > +		size_t len = min(strnlen(src.kernel, count - 1) + 1, count);
> 
> I didn't get why do we need min()? To check the count == 0 case?
> 
> Wouldn't
> 
> 		size_t len;
> 
> 		len = strnlen(src.kernel, count);
> 		if (len == 0)
> 			return 0;
> 
> 		/* Copy a trailing NUL if found */
> 		if (len < count)
> 			len++;
> 
> be a good equivalent?

A good question.  I rather wonder why it can't be simple strncpy().

> > +		memcpy(dst, src.kernel, len);
> > +		return len;
> > +	}
> > +	return strncpy_from_user(dst, src.user, count);
> > +}
> 
> ...
> 
> > +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t size)
> > +{
> > +	if (!uniptr_is_kernel(src))
> 
> Why not to align all the functions to use same conditional (either always
> positive or negative)?

A different person, a different taste :)  But it's trivial to fix.

> > +		return check_zeroed_user(src.user + offset, size);
> > +	return memchr_inv(src.kernel + offset, 0, size) == NULL;
> > +}
> 
> ...
> 
> Taking all remarks into account I would rather go with sockptr.h being
> untouched for now, just a big
> 
> /* DO NOT USE, it's obsolete, use uniptr.h instead! */
> 
> to be added.

Possibly that's a safer choice.  But the biggest question is whether
we want a generic type or not.  Let's try to ask it first.

Interestingly, this file doesn't belong to any subsystem in
MAINTAINERS, so I'm not sure who to be Cc'ed.  Chirstoph as the
original author and net dev, maybe.


thanks,

Takashi

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 05:22:18PM +0200, Takashi Iwai wrote:
> On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote:
> > On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:

...

> I rather wonder why it can't be simple strncpy().

This is obvious. To avoid compiler warning about 0 (possible) truncation.

...

> > Taking all remarks into account I would rather go with sockptr.h being
> > untouched for now, just a big
> > 
> > /* DO NOT USE, it's obsolete, use uniptr.h instead! */
> > 
> > to be added.
> 
> Possibly that's a safer choice.  But the biggest question is whether
> we want a generic type or not.  Let's try to ask it first.
> 
> Interestingly, this file doesn't belong to any subsystem in
> MAINTAINERS, so I'm not sure who to be Cc'ed.  Chirstoph as the
> original author and net dev, maybe.

Yes, but actually it's fine to just copy and change sockptr.h to say
"that's blablabla for net subsystem" (maybe this is implied by the name?).
In that case we just introduce our copy and can do whatever modifications
we want (see previous reply).

-- 
With Best Regards,
Andy Shevchenko