[3/7] ALSA: pcm: avoid mmap of control data if .update_appl_ptr is implemented
diff mbox

Message ID 1475239410-16548-4-git-send-email-subhransu.s.prusty@intel.com
State New
Headers show

Commit Message

Subhransu S. Prusty Sept. 30, 2016, 12:43 p.m. UTC
From: Ramesh Babu <ramesh.babu@intel.com>

In case of mmap, by default alsa-lib mmaps both control & status data.

If driver subscribes for .appl_ptr_update, driver needs to get
notification whenever appl ptr changes. So with control & status mmaped,
driver won't get appl ptr notifications.

In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
conf But this makes driver behavior dependent on a flag in the conf.

This patch conditionally checks for .appl_ptr_update and returns error
when user land asks for mmaping control & status data, thus forcing user
to issue IOCTL_SYNC_PTR.

One drawback with this approach is, if .appl_ptr_update is subscribed by
driver, the user space looses flexibility to mmap the control & status
data.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/core/pcm_native.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Takashi Iwai Sept. 30, 2016, 1:40 p.m. UTC | #1
On Fri, 30 Sep 2016 14:43:26 +0200,
Subhransu S. Prusty wrote:
> 
> From: Ramesh Babu <ramesh.babu@intel.com>
> 
> In case of mmap, by default alsa-lib mmaps both control & status data.
> 
> If driver subscribes for .appl_ptr_update, driver needs to get
> notification whenever appl ptr changes. So with control & status mmaped,
> driver won't get appl ptr notifications.
> 
> In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
> conf But this makes driver behavior dependent on a flag in the conf.
> 
> This patch conditionally checks for .appl_ptr_update and returns error
> when user land asks for mmaping control & status data, thus forcing user
> to issue IOCTL_SYNC_PTR.
> 
> One drawback with this approach is, if .appl_ptr_update is subscribed by
> driver, the user space looses flexibility to mmap the control & status
> data.

Yes, and it can be seen as an obvious regression, so I'm not sure
whether this condition is the best choice.

OTOH, I now understand why you need it in this way -- alsa-lib does
mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as
a fallback.  So, one solution would be to fix alsa-lib side.  Or, if
we fix the kernel in this way, it would work with old alsa-lib, but it
has another penalty.  Hmm...


Takashi

> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/core/pcm_native.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c56d4ed..1965d83 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3535,10 +3535,22 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
>  	case SNDRV_PCM_MMAP_OFFSET_STATUS:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * we want to force sync pointer,
> +		 * if driver implements appl_ptr_update
> +		 */
> +		if (substream->ops->appl_ptr_update)
> +			return -ENXIO;
>  		return snd_pcm_mmap_status(substream, file, area);
>  	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * we want to force sync pointer,
> +		 * if driver implements appl_ptr_update
> +		 */
> +		if (substream->ops->appl_ptr_update)
> +			return -ENXIO;
>  		return snd_pcm_mmap_control(substream, file, area);
>  	default:
>  		return snd_pcm_mmap_data(substream, file, area);
> -- 
> 1.9.1
>
Subhransu S. Prusty Oct. 3, 2016, 4:43 a.m. UTC | #2
On Fri, Sep 30, 2016 at 03:40:45PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:26 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Ramesh Babu <ramesh.babu@intel.com>
> > 
> > In case of mmap, by default alsa-lib mmaps both control & status data.
> > 
> > If driver subscribes for .appl_ptr_update, driver needs to get
> > notification whenever appl ptr changes. So with control & status mmaped,
> > driver won't get appl ptr notifications.
> > 
> > In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
> > conf But this makes driver behavior dependent on a flag in the conf.
> > 
> > This patch conditionally checks for .appl_ptr_update and returns error
> > when user land asks for mmaping control & status data, thus forcing user
> > to issue IOCTL_SYNC_PTR.
> > 
> > One drawback with this approach is, if .appl_ptr_update is subscribed by
> > driver, the user space looses flexibility to mmap the control & status
> > data.
> 
> Yes, and it can be seen as an obvious regression, so I'm not sure
> whether this condition is the best choice.
> 
> OTOH, I now understand why you need it in this way -- alsa-lib does
> mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as
> a fallback.  So, one solution would be to fix alsa-lib side.  Or, if
> we fix the kernel in this way, it would work with old alsa-lib, but it
> has another penalty.  Hmm...

I will check if it can be fixed in alsa-lib side.

Regards,
Subhransu
> 
>

Patch
diff mbox

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c56d4ed..1965d83 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3535,10 +3535,22 @@  static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
 	case SNDRV_PCM_MMAP_OFFSET_STATUS:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+		/*
+		 * we want to force sync pointer,
+		 * if driver implements appl_ptr_update
+		 */
+		if (substream->ops->appl_ptr_update)
+			return -ENXIO;
 		return snd_pcm_mmap_status(substream, file, area);
 	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+		/*
+		 * we want to force sync pointer,
+		 * if driver implements appl_ptr_update
+		 */
+		if (substream->ops->appl_ptr_update)
+			return -ENXIO;
 		return snd_pcm_mmap_control(substream, file, area);
 	default:
 		return snd_pcm_mmap_data(substream, file, area);