[alsa-devel] [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

Hi,

On May 16 2017 10:01, Subhransu S. Prusty wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>
> In case of mmap, by default alsa-lib mmaps both control and status data.
>
> If driver subscribes for application pointer update, driver needs to get
> notification whenever appl ptr changes. With the above case driver won't
> get appl ptr notifications.
>
> This patch check on a hw info flag and returns error when user land asks
> for mmaping control & status data, thus forcing user to issue
> IOCTL_SYNC_PTR.
>
> Suggested-by: Takashi Iwai <tiwai at suse.de>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
> Signed-off-by: Ramesh Babu <ramesh.babu at intel.com>
> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi at intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty at intel.com>
> ---
>  include/uapi/sound/asound.h |  1 +
>  sound/core/pcm_native.c     | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)

Would you please explain about the case that drivers can't support page 
frame mapping, please?

As long as I know, it depends on kernel and platform architecture 
whether mapping is available or not, because the data of 'struct 
snd_pcm_mmap_status' and 'struct snd_pcm_mmap_control' is not directly 
related to data transmission and independent of target device design.

Of course, I can understand your intension for previous patches. But the 
code comment is quite misleading and worthless.

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index c697ff90450d..dea7d89b41ca 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -284,6 +284,7 @@ enum {
>  #define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME     0x02000000  /* report absolute hardware link audio time, not reset on startup */
>  #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
>  #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
> +#define SNDRV_PCM_INFO_NO_STATUS_MMAP	0x10000000	/* status and control mmap not supported */
>
>  #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
>  #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c14cdbd9ff86..2635a7d4d1fa 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3524,21 +3524,38 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
>  	struct snd_pcm_file * pcm_file;
>  	struct snd_pcm_substream *substream;	
>  	unsigned long offset;
> +	unsigned int info;
>  	
>  	pcm_file = file->private_data;
>  	substream = pcm_file->substream;
>  	if (PCM_RUNTIME_CHECK(substream))
>  		return -ENXIO;
> +	info = substream->runtime->hw.info;
>
>  	offset = area->vm_pgoff << PAGE_SHIFT;
>  	switch (offset) {
>  	case SNDRV_PCM_MMAP_OFFSET_STATUS:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * force fallback to ioctl if driver doesn't support status
> +		 * and control mmap.
> +		 */
> +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> +			return -ENXIO;
> +
>  		return snd_pcm_mmap_status(substream, file, area);
>  	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +
> +		/*
> +		 * force fallback to ioctl if driver doesn't support status
> +		 * and control mmap.
> +		 */
> +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> +			return -ENXIO;
> +
>  		return snd_pcm_mmap_control(substream, file, area);
>  	default:
>  		return snd_pcm_mmap_data(substream, file, area);

Regards

Takashi Sakamoto