[PATCH v1 1/1] ALSA: korg1212: Re-use sockptr_t and respective APIs
The sockptr_t (despite the naming) is a generic type to hold kernel or user pointer and there are respective APIs to copy data to or from it. Replace open coded variants in the driver by them.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- sound/pci/korg1212/korg1212.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c index 33b4f95d65b3..92c3eab4d12c 100644 --- a/sound/pci/korg1212/korg1212.c +++ b/sound/pci/korg1212/korg1212.c @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/pci.h> #include <linux/slab.h> +#include <linux/sockptr.h> #include <linux/wait.h> #include <linux/module.h> #include <linux/mutex.h> @@ -1285,8 +1286,7 @@ static int snd_korg1212_silence(struct snd_korg1212 *korg1212, int pos, int coun }
static int snd_korg1212_copy_to(struct snd_pcm_substream *substream, - void __user *dst, int pos, int count, - bool in_kernel) + sockptr_t dst, int pos, int count) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_korg1212 *korg1212 = snd_pcm_substream_chip(substream); @@ -1306,24 +1306,21 @@ static int snd_korg1212_copy_to(struct snd_pcm_substream *substream, #if K1212_DEBUG_LEVEL > 0 if ( (void *) src < (void *) korg1212->recordDataBufsPtr || (void *) src > (void *) korg1212->recordDataBufsPtr[8].bufferData ) { - printk(KERN_DEBUG "K1212_DEBUG: snd_korg1212_copy_to KERNEL EFAULT, src=%p dst=%p iter=%d\n", src, dst, i); + printk(KERN_DEBUG "K1212_DEBUG: %s KERNEL EFAULT, src=%p dst=%p iter=%d\n", + __func__, src, sockptr_is_kernel(dst) ? dst.kernel : dst.user, i); return -EFAULT; } #endif - if (in_kernel) - memcpy((__force void *)dst, src, size); - else if (copy_to_user(dst, src, size)) + if (copy_to_sockptr_offset(dst, i * size, src, size)) return -EFAULT; src++; - dst += size; }
return 0; }
static int snd_korg1212_copy_from(struct snd_pcm_substream *substream, - void __user *src, int pos, int count, - bool in_kernel) + sockptr_t src, int pos, int count) { struct snd_pcm_runtime *runtime = substream->runtime; struct snd_korg1212 *korg1212 = snd_pcm_substream_chip(substream); @@ -1345,16 +1342,14 @@ static int snd_korg1212_copy_from(struct snd_pcm_substream *substream, #if K1212_DEBUG_LEVEL > 0 if ( (void *) dst < (void *) korg1212->playDataBufsPtr || (void *) dst > (void *) korg1212->playDataBufsPtr[8].bufferData ) { - printk(KERN_DEBUG "K1212_DEBUG: snd_korg1212_copy_from KERNEL EFAULT, src=%p dst=%p iter=%d\n", src, dst, i); + printk(KERN_DEBUG "K1212_DEBUG: %s KERNEL EFAULT, src=%p dst=%p iter=%d\n", + __func__, sockptr_is_kernel(src) ? src.kernel : src.user, dst, i); return -EFAULT; } #endif - if (in_kernel) - memcpy(dst, (__force void *)src, size); - else if (copy_from_user(dst, src, size)) + if (copy_from_sockptr_offset(dst, src, i * size, size)) return -EFAULT; dst++; - src += size; }
return 0; @@ -1644,15 +1639,14 @@ static int snd_korg1212_playback_copy(struct snd_pcm_substream *substream, int channel, unsigned long pos, void __user *src, unsigned long count) { - return snd_korg1212_copy_from(substream, src, pos, count, false); + return snd_korg1212_copy_from(substream, USER_SOCKPTR(src), pos, count); }
static int snd_korg1212_playback_copy_kernel(struct snd_pcm_substream *substream, int channel, unsigned long pos, void *src, unsigned long count) { - return snd_korg1212_copy_from(substream, (void __user *)src, - pos, count, true); + return snd_korg1212_copy_from(substream, KERNEL_SOCKPTR(src), pos, count); }
static int snd_korg1212_playback_silence(struct snd_pcm_substream *substream, @@ -1672,15 +1666,14 @@ static int snd_korg1212_capture_copy(struct snd_pcm_substream *substream, int channel, unsigned long pos, void __user *dst, unsigned long count) { - return snd_korg1212_copy_to(substream, dst, pos, count, false); + return snd_korg1212_copy_to(substream, USER_SOCKPTR(dst), pos, count); }
static int snd_korg1212_capture_copy_kernel(struct snd_pcm_substream *substream, int channel, unsigned long pos, void *dst, unsigned long count) { - return snd_korg1212_copy_to(substream, (void __user *)dst, - pos, count, true); + return snd_korg1212_copy_to(substream, KERNEL_SOCKPTR(dst), pos, count); }
static const struct snd_pcm_ops snd_korg1212_playback_ops = {
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
The sockptr_t (despite the naming) is a generic type to hold kernel or user pointer and there are respective APIs to copy data to or from it. Replace open coded variants in the driver by them.
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Also, the current function actually follows the call pattern, and we know in the caller side whether it's called for a kernel pointer or a user pointer. So, if any, the PCM core callbacks should be revised to use a generic pointer instead of fiddling in each driver side.
thanks,
Takashi
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
sound/pci/korg1212/korg1212.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/sound/pci/korg1212/korg1212.c b/sound/pci/korg1212/korg1212.c index 33b4f95d65b3..92c3eab4d12c 100644 --- a/sound/pci/korg1212/korg1212.c +++ b/sound/pci/korg1212/korg1212.c @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/pci.h> #include <linux/slab.h> +#include <linux/sockptr.h> #include <linux/wait.h> #include <linux/module.h> #include <linux/mutex.h> @@ -1285,8 +1286,7 @@ static int snd_korg1212_silence(struct snd_korg1212 *korg1212, int pos, int coun }
static int snd_korg1212_copy_to(struct snd_pcm_substream *substream,
void __user *dst, int pos, int count,bool in_kernel)
sockptr_t dst, int pos, int count){ struct snd_pcm_runtime *runtime = substream->runtime; struct snd_korg1212 *korg1212 = snd_pcm_substream_chip(substream); @@ -1306,24 +1306,21 @@ static int snd_korg1212_copy_to(struct snd_pcm_substream *substream, #if K1212_DEBUG_LEVEL > 0 if ( (void *) src < (void *) korg1212->recordDataBufsPtr || (void *) src > (void *) korg1212->recordDataBufsPtr[8].bufferData ) {
printk(KERN_DEBUG "K1212_DEBUG: snd_korg1212_copy_to KERNEL EFAULT, src=%p dst=%p iter=%d\n", src, dst, i);
printk(KERN_DEBUG "K1212_DEBUG: %s KERNEL EFAULT, src=%p dst=%p iter=%d\n",__func__, src, sockptr_is_kernel(dst) ? dst.kernel : dst.user, i); return -EFAULT;#endif
if (in_kernel)memcpy((__force void *)dst, src, size);else if (copy_to_user(dst, src, size))
if (copy_to_sockptr_offset(dst, i * size, src, size)) return -EFAULT;
dst += size;}
return 0;
}
static int snd_korg1212_copy_from(struct snd_pcm_substream *substream,
void __user *src, int pos, int count,bool in_kernel)
sockptr_t src, int pos, int count){ struct snd_pcm_runtime *runtime = substream->runtime; struct snd_korg1212 *korg1212 = snd_pcm_substream_chip(substream); @@ -1345,16 +1342,14 @@ static int snd_korg1212_copy_from(struct snd_pcm_substream *substream, #if K1212_DEBUG_LEVEL > 0 if ( (void *) dst < (void *) korg1212->playDataBufsPtr || (void *) dst > (void *) korg1212->playDataBufsPtr[8].bufferData ) {
printk(KERN_DEBUG "K1212_DEBUG: snd_korg1212_copy_from KERNEL EFAULT, src=%p dst=%p iter=%d\n", src, dst, i);
printk(KERN_DEBUG "K1212_DEBUG: %s KERNEL EFAULT, src=%p dst=%p iter=%d\n",__func__, sockptr_is_kernel(src) ? src.kernel : src.user, dst, i); return -EFAULT;#endif
if (in_kernel)memcpy(dst, (__force void *)src, size);else if (copy_from_user(dst, src, size))
if (copy_from_sockptr_offset(dst, src, i * size, size)) return -EFAULT;
src += size;}
return 0;
@@ -1644,15 +1639,14 @@ static int snd_korg1212_playback_copy(struct snd_pcm_substream *substream, int channel, unsigned long pos, void __user *src, unsigned long count) {
- return snd_korg1212_copy_from(substream, src, pos, count, false);
- return snd_korg1212_copy_from(substream, USER_SOCKPTR(src), pos, count);
}
static int snd_korg1212_playback_copy_kernel(struct snd_pcm_substream *substream, int channel, unsigned long pos, void *src, unsigned long count) {
- return snd_korg1212_copy_from(substream, (void __user *)src,
pos, count, true);
- return snd_korg1212_copy_from(substream, KERNEL_SOCKPTR(src), pos, count);
}
static int snd_korg1212_playback_silence(struct snd_pcm_substream *substream, @@ -1672,15 +1666,14 @@ static int snd_korg1212_capture_copy(struct snd_pcm_substream *substream, int channel, unsigned long pos, void __user *dst, unsigned long count) {
- return snd_korg1212_copy_to(substream, dst, pos, count, false);
- return snd_korg1212_copy_to(substream, USER_SOCKPTR(dst), pos, count);
}
static int snd_korg1212_capture_copy_kernel(struct snd_pcm_substream *substream, int channel, unsigned long pos, void *dst, unsigned long count) {
- return snd_korg1212_copy_to(substream, (void __user *)dst,
pos, count, true);
- return snd_korg1212_copy_to(substream, KERNEL_SOCKPTR(dst), pos, count);
}
static const struct snd_pcm_ops snd_korg1212_playback_ops = {
2.40.0.1.gaa8946217a0b
On Fri, Jul 21, 2023 at 12:08:46PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
The sockptr_t (despite the naming) is a generic type to hold kernel or user pointer and there are respective APIs to copy data to or from it. Replace open coded variants in the driver by them.
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Also, the current function actually follows the call pattern, and we know in the caller side whether it's called for a kernel pointer or a user pointer. So, if any, the PCM core callbacks should be revised to use a generic pointer instead of fiddling in each driver side.
Any suggestion for the name? And I believe for the bigger series the new callback should be added first.
On Fri, 21 Jul 2023 12:42:08 +0200, Andy Shevchenko wrote:
On Fri, Jul 21, 2023 at 12:08:46PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
The sockptr_t (despite the naming) is a generic type to hold kernel or user pointer and there are respective APIs to copy data to or from it. Replace open coded variants in the driver by them.
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Also, the current function actually follows the call pattern, and we know in the caller side whether it's called for a kernel pointer or a user pointer. So, if any, the PCM core callbacks should be revised to use a generic pointer instead of fiddling in each driver side.
Any suggestion for the name?
It's a universal pointer... uniptr_t? Or a generic pointer, genptr_t?
I'm not good at naming, and I'm open for it.
And I believe for the bigger series the new callback should be added first.
Yes.
Takashi
On Fri, Jul 21, 2023 at 12:58:14PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:42:08 +0200, Andy Shevchenko wrote:
On Fri, Jul 21, 2023 at 12:08:46PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
...
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Any suggestion for the name?
It's a universal pointer... uniptr_t? Or a generic pointer, genptr_t?
I'm not good at naming, and I'm open for it.
It seems it's already spread enough with this name, I would rather stick with it for now (besides net it's used in crypto, nvme, and security).
The (new) callback though makes a lot of sense on its own.
What do you think?
On Fri, 21 Jul 2023 14:58:05 +0200, Andy Shevchenko wrote:
On Fri, Jul 21, 2023 at 12:58:14PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:42:08 +0200, Andy Shevchenko wrote:
On Fri, Jul 21, 2023 at 12:08:46PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
...
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Any suggestion for the name?
It's a universal pointer... uniptr_t? Or a generic pointer, genptr_t?
I'm not good at naming, and I'm open for it.
It seems it's already spread enough with this name, I would rather stick with it for now (besides net it's used in crypto, nvme, and security).
The (new) callback though makes a lot of sense on its own.
OK, fair enough.
What do you think?
Yes, we can go with it.
Basically we need to add a new "copy" callback to take sockptr_t and use it instead of the old "copy_kernel" and "copy_user" callbacks. It's used only in sound/core/pcm_lib.c, so it shouldn't be too difficult, I suppose. Then replace the defined callbacks in each driver, and finally deprecate old callbacks.
thanks,
Takashi
From: Andy Shevchenko
Sent: 21 July 2023 11:42
On Fri, Jul 21, 2023 at 12:08:46PM +0200, Takashi Iwai wrote:
On Fri, 21 Jul 2023 12:01:46 +0200, Andy Shevchenko wrote:
The sockptr_t (despite the naming) is a generic type to hold kernel or user pointer and there are respective APIs to copy data to or from it. Replace open coded variants in the driver by them.
While I see the benefit, I feel this is very confusing. If we use the API for a generic use, it should be renamed at first.
Also, the current function actually follows the call pattern, and we know in the caller side whether it's called for a kernel pointer or a user pointer. So, if any, the PCM core callbacks should be revised to use a generic pointer instead of fiddling in each driver side.
Any suggestion for the name? And I believe for the bigger series the new callback should be added first.
It would also be better to replace the current sockptr_t with a structure that contains separate user and kernel addresses (instead of a union of the pointer types and a flag).
The size of the structure wouldn't change and the code might even come out better.
There is also scope for adding a length and passing the structure by reference instead of by value.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
participants (3)
-
Andy Shevchenko -
David Laight -
Takashi Iwai