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

[Takashi Iwai]

On Tue, 16 May 2017 07:38:40 +0200,
Takashi Sakamoto wrote:
> 
> 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.

It's only a part of the condition.

In this case, the problem is that the mmap control allows the appl_ptr
being changed silently without interaction with the driver.  If the
driver requires some explicit action for changing the appl_ptr, it
won't work.

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

Worthless is a too strong word, but I agree that more clarification
would be more helpful.


thanks,

Takashi

> 
> > 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
>