[alsa-devel] [PATCH v2 alsa-lib 1/6] pcm: hw: add helper functions to map/unmap status/control data for runtime of PCM substream

[Takashi Iwai]

On Tue, 27 Jun 2017 10:09:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 27 2017 00:09, Takashi Iwai wrote:
> > On Sun, 25 Jun 2017 06:41:19 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> Handling mapping operation for status/control data includes some
> >> supplemental operations for fallback mode. It's better to have helper
> >> functions for this purpose.
> >>
> >> This commit adds the helper functions.
> >> ---
> >>   src/pcm/pcm_hw.c | 48 ++++++++++++++++++++++++++++++++++--------------
> >>   1 file changed, 34 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
> >> index a648d12c..abf4afe0 100644
> >> --- a/src/pcm/pcm_hw.c
> >> +++ b/src/pcm/pcm_hw.c
> >> @@ -31,6 +31,7 @@
> >>   #include <stdlib.h>
> >>   #include <stddef.h>
> >>   #include <unistd.h>
> >> +#include <stdbool.h>
> >>   #include <signal.h>
> >>   #include <string.h>
> >>   #include <fcntl.h>
> >> @@ -866,7 +867,7 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
> >>   	return xfern.result;
> >>   }
> >>   -static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
> >> +static int map_status_data(snd_pcm_t *pcm)
> >>   {
> >>   	snd_pcm_hw_t *hw = pcm->private_data;
> >>   	struct snd_pcm_sync_ptr sync_ptr;
> >> @@ -900,7 +901,7 @@ static int snd_pcm_hw_mmap_status(snd_pcm_t *pcm)
> >>   	return 0;
> >>   }
> >>   -static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
> >> +static int map_control_data(snd_pcm_t *pcm)
> >>   {
> >>   	snd_pcm_hw_t *hw = pcm->private_data;
> >>   	void *ptr;
> >> @@ -922,10 +923,28 @@ static int snd_pcm_hw_mmap_control(snd_pcm_t *pcm)
> >>   	return 0;
> >>   }
> >>   -static int snd_pcm_hw_munmap_status(snd_pcm_t *pcm)
> >> +static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
> >>   {
> >>   	snd_pcm_hw_t *hw = pcm->private_data;
> >>   	int err;
> >> +
> >> +	hw->sync_ptr_ioctl = (int)force_fallback;
> >> +
> >> +	err = map_status_data(pcm);
> >> +	if (err < 0)
> >> +		return err;
> >> +
> >> +	err = map_control_data(pcm);
> >> +	if (err < 0)
> >> +		return err;
> >
> > This would leave the status mmap in the error path?
> 
> In the error path, snd_pcm_hw_open_fd() calls snd_pcm_close(), then
> The status data is going to be unmapped.
> 
> snd_pcm_hw_open_fd()
> ->snd_pcm_close()
>   ->snd_pcm_hw_close()
>     ->unmap_status_and_control_data()
>       ->unmap_status_data()
>         ->munmap(2) or free(3)

Ah then there was already a bug there.  It free/munmap unconditionally
for both status and control even for an error path of the mmap of
control (while status was mmapped).

But OK, it's no big issue and doesn't need to block the whole patchset
at this point.


thanks,

Takashi