| Message ID | 20230721100146.67293-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] ALSA: korg1212: Re-use sockptr_t and respective APIs | expand |
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; > 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 = { > -- > 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?
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)
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
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 = {
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(-)