[PATCH v2 alsa-lib 0/6] pcm: hw: maintain fallback mode for status/control data separately

[PATCH v2 alsa-lib 0/6] pcm: hw: maintain fallback mode for status/control data separately

[Takashi Sakamoto]

Hi,

This patchset is a revised version for a part of my previous RFC:
[alsa-devel] [alsa-lib][RFC][PATCH 0/9] pcm: handle status/control mapping independently
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122130.html

As I described, we found an advantage to handle results of page mapping
for status/control data separately in a discussion about ALSA PCM protocol
v2.0.14.

In this version, when kernel-land drivers support
SNDRV_PCM_INFO_SYNC_APPLPTR for a certain design of hardware, userspace
applications are expected to issue current position of applptr for several
merits. This should be performed even if applications are programmed to run
with mmap operation for PCM buffer. In the above discussion, the way to
take applications to perform it is one of important issues.

On platforms without enough cache coherency support such as ARM, this
doesn't matter. On the platforms, ALSA PCM core already disallows any
of mappings for the status/control data of runtime of PCM substream.
For this case, userland stuffs were already programmed to go to fallback
mode, in which ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR is used to
issue/query the status/control data. Even if handling PCM frames on PCM
buffer by mmap operation, the position of applptr is issues into ALSA
PCM core by a call of ioctl(2) with the command.

As a solution for old stuffs in userland, the above mechanism is used
for cache coherent architecture such as x86. A patch was already merged
into kernel land[1]. This patch changes behaviours of ALSA PCM core to
disallow userland to map the status/control data according to drivers'
support for SNDRV_PCM_INFO_SYNC_APPLPTR.

However, disallowing mappings for both of status/control data has a slight
overhead to increase calls of ioctl(2), than usual operation on x86. As
I reported[2]. Furthermore, it's clear that current proposals make no
sense for drivers because 'struct snd_pcm_ops.ack' in driver implementation
is not called expectedly due to a commit[4]. Even if the applptr is
issued by ioctl(2), the callback is skipped as long as mapped status
includes the updated value for the applptr.

In the above discussion, some solutions were suggested[5]. One of them
is to enable map of the status data and disable map of the control data.
This idea can archive both of the aim of v2.0.14 protocol and reduction
of calls for status data, for newer userland stuffs. 

This patchset for alsa-lib is a preparation for the idea. In current
implementation of this library, results of mapping operation for
status/control data are not handled separately. This patchset is a
similar to code refactoring, thus this includes no essential behaviour
changes against released kernel lands.

[1] ALSA: pcm: Add the explicit appl_ptr sync support
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=42f945970af9df6216e3d771b4df371d02d8742c
[2] ALSA: pcm: add 'applptr' event of tracepoint
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next&id=fccf53881e9b564321326f62ed5f85130fa6e569
[3] [alsa-devel] [PATCH 0/3] ALSA: Add the explicit appl_ptr sync support
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122092.html
[4] ALSA: pcm: Skip ack callback without actual appl_ptr update
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?h=for-next&id=f8ff2f28ba49fa41a06215ac3187dede347bc9a7
[5] [alsa-devel] [PATCH 0/3] ALSA: Add the explicit appl_ptr sync support
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/122093.html

Takashi Sakamoto (6):
  pcm: hw: add helper functions to map/unmap status/control data for
    runtime of PCM substream
  pcm: hw: add an arrangement for initialization of appl_ptr/avail_min
  pcm: hw: deallocate fallback buffer when trials of unmapping finished
  pcm: hw: allocate fallback buffer in advance of trials of mapping
  pcm: hw: maintain fallback mode for status data mapping
  pcm: hw: maintain fallback mode for control data mapping independently

 src/pcm/pcm_hw.c | 192 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 115 insertions(+), 77 deletions(-)

-- 
2.11.0

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

[Takashi Sakamoto]

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;
+
+	return 0;
+}
+
+static int unmap_status_data(snd_pcm_hw_t *hw)
+{
+	int err;
+
 	if (hw->sync_ptr_ioctl) {
 		free(hw->sync_ptr);
 		hw->sync_ptr = NULL;
@@ -939,10 +958,10 @@ static int snd_pcm_hw_munmap_status(snd_pcm_t *pcm)
 	return 0;
 }
 
-static int snd_pcm_hw_munmap_control(snd_pcm_t *pcm)
+static int unmap_control_data(snd_pcm_hw_t *hw)
 {
-	snd_pcm_hw_t *hw = pcm->private_data;
 	int err;
+
 	if (hw->sync_ptr_ioctl) {
 		free(hw->sync_ptr);
 		hw->sync_ptr = NULL;
@@ -956,6 +975,12 @@ static int snd_pcm_hw_munmap_control(snd_pcm_t *pcm)
 	return 0;
 }
 
+static void unmap_status_and_control_data(snd_pcm_hw_t *hw)
+{
+	unmap_status_data(hw);
+	unmap_control_data(hw);
+}
+
 static int snd_pcm_hw_mmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED)
 {
 	return 0;
@@ -974,8 +999,9 @@ static int snd_pcm_hw_close(snd_pcm_t *pcm)
 		err = -errno;
 		SYSMSG("close failed (%i)\n", err);
 	}
-	snd_pcm_hw_munmap_status(pcm);
-	snd_pcm_hw_munmap_control(pcm);
+
+	unmap_status_and_control_data(hw);
+
 	free(hw);
 	return err;
 }
@@ -1484,7 +1510,6 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
 	hw->device = info.device;
 	hw->subdevice = info.subdevice;
 	hw->fd = fd;
-	hw->sync_ptr_ioctl = sync_ptr_ioctl;
 	/* no restriction */
 	hw->format = SND_PCM_FORMAT_UNKNOWN;
 	hw->rate = 0;
@@ -1508,12 +1533,7 @@ int snd_pcm_hw_open_fd(snd_pcm_t **pcmp, const char *name, int fd,
 #endif
 	pcm->own_state_check = 1; /* skip the common state check */
 
-	ret = snd_pcm_hw_mmap_status(pcm);
-	if (ret < 0) {
-		snd_pcm_close(pcm);
-		return ret;
-	}
-	ret = snd_pcm_hw_mmap_control(pcm);
+	ret = map_status_and_control_data(pcm, !!sync_ptr_ioctl);
 	if (ret < 0) {
 		snd_pcm_close(pcm);
 		return ret;
-- 
2.11.0

[PATCH v2 alsa-lib 2/6] pcm: hw: add an arrangement for initialization of appl_ptr/avail_min

[Takashi Sakamoto]

Regardless of success/failure mapping of control/status data for runtime of
PCM substream, appl_ptr/avail_min parameters are initialized. In current
implementation, they are initialized in case-dependent, different places.
It's possible to collect them to one place.

This commit unifies relevant code in a place after all of trials for the
mappings are finished.
---
 src/pcm/pcm_hw.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index abf4afe0..c60a521b 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -867,10 +867,8 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
 	return xfern.result;
 }
 
-static int map_status_data(snd_pcm_t *pcm)
+static int map_status_data(snd_pcm_hw_t *hw)
 {
-	snd_pcm_hw_t *hw = pcm->private_data;
-	struct snd_pcm_sync_ptr sync_ptr;
 	void *ptr;
 	int err;
 	ptr = MAP_FAILED;
@@ -879,15 +877,6 @@ static int map_status_data(snd_pcm_t *pcm)
 			   PROT_READ, MAP_FILE|MAP_SHARED, 
 			   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
 	if (ptr == MAP_FAILED || ptr == NULL) {
-		memset(&sync_ptr, 0, sizeof(sync_ptr));
-		sync_ptr.c.control.appl_ptr = 0;
-		sync_ptr.c.control.avail_min = 1;
-		err = ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, &sync_ptr);
-		if (err < 0) {
-			err = -errno;
-			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
-			return err;
-		}
 		hw->sync_ptr = calloc(1, sizeof(struct snd_pcm_sync_ptr));
 		if (hw->sync_ptr == NULL)
 			return -ENOMEM;
@@ -897,13 +886,12 @@ static int map_status_data(snd_pcm_t *pcm)
 	} else {
 		hw->mmap_status = ptr;
 	}
-	snd_pcm_set_hw_ptr(pcm, &hw->mmap_status->hw_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS + offsetof(struct snd_pcm_mmap_status, hw_ptr));
+
 	return 0;
 }
 
-static int map_control_data(snd_pcm_t *pcm)
+static int map_control_data(snd_pcm_hw_t *hw)
 {
-	snd_pcm_hw_t *hw = pcm->private_data;
 	void *ptr;
 	int err;
 	if (hw->sync_ptr == NULL) {
@@ -916,10 +904,8 @@ static int map_control_data(snd_pcm_t *pcm)
 			return err;
 		}
 		hw->mmap_control = ptr;
-	} else {
-		hw->mmap_control->avail_min = 1;
 	}
-	snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
+
 	return 0;
 }
 
@@ -930,14 +916,30 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 
 	hw->sync_ptr_ioctl = (int)force_fallback;
 
-	err = map_status_data(pcm);
+	err = map_status_data(hw);
 	if (err < 0)
 		return err;
 
-	err = map_control_data(pcm);
+	err = map_control_data(hw);
 	if (err < 0)
 		return err;
 
+	/* Initialize the data. */
+	hw->mmap_control->appl_ptr = 0;
+	hw->mmap_control->avail_min = 1;
+	snd_pcm_set_hw_ptr(pcm, &hw->mmap_status->hw_ptr, hw->fd,
+			   SNDRV_PCM_MMAP_OFFSET_STATUS +
+				offsetof(struct snd_pcm_mmap_status, hw_ptr));
+	snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd,
+			     SNDRV_PCM_MMAP_OFFSET_CONTROL);
+	if (hw->sync_ptr_ioctl) {
+		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, hw->sync_ptr) < 0) {
+			err = -errno;
+			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
+			return err;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.11.0

[PATCH v2 alsa-lib 3/6] pcm: hw: deallocate fallback buffer when trials of unmapping finished

[Takashi Sakamoto]

In current implementation, deallocation of fallback buffer is done at
several places.

This commit unifies these deallocations in one place.
---
 src/pcm/pcm_hw.c | 43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index c60a521b..1d34956c 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -943,44 +943,35 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 	return 0;
 }
 
-static int unmap_status_data(snd_pcm_hw_t *hw)
+static void unmap_status_data(snd_pcm_hw_t *hw)
 {
-	int err;
-
-	if (hw->sync_ptr_ioctl) {
-		free(hw->sync_ptr);
-		hw->sync_ptr = NULL;
-	} else {
-		if (munmap((void*)hw->mmap_status, page_align(sizeof(*hw->mmap_status))) < 0) {
-			err = -errno;
-			SYSMSG("status munmap failed (%i)", err);
-			return err;
-		}
+	if (!hw->sync_ptr) {
+		if (munmap((void *)hw->mmap_status,
+			   page_align(sizeof(*hw->mmap_status))) < 0)
+			SYSMSG("status munmap failed (%u)", errno);
 	}
-	return 0;
 }
 
-static int unmap_control_data(snd_pcm_hw_t *hw)
+static void unmap_control_data(snd_pcm_hw_t *hw)
 {
-	int err;
-
-	if (hw->sync_ptr_ioctl) {
-		free(hw->sync_ptr);
-		hw->sync_ptr = NULL;
-	} else {
-		if (munmap(hw->mmap_control, page_align(sizeof(*hw->mmap_control))) < 0) {
-			err = -errno;
-			SYSMSG("control munmap failed (%i)", err);
-			return err;
-		}
+	if (!hw->sync_ptr) {
+		if (munmap((void *)hw->mmap_control,
+			   page_align(sizeof(*hw->mmap_control))) < 0)
+			SYSMSG("control munmap failed (%u)", errno);
 	}
-	return 0;
 }
 
 static void unmap_status_and_control_data(snd_pcm_hw_t *hw)
 {
 	unmap_status_data(hw);
 	unmap_control_data(hw);
+
+	if (hw->sync_ptr)
+		free(hw->sync_ptr);
+
+	hw->mmap_status = NULL;
+	hw->mmap_control = NULL;
+	hw->sync_ptr = NULL;
 }
 
 static int snd_pcm_hw_mmap(snd_pcm_t *pcm ATTRIBUTE_UNUSED)
-- 
2.11.0

[PATCH v2 alsa-lib 4/6] pcm: hw: allocate fallback buffer in advance of trials of mapping

[Takashi Sakamoto]

When allowing failure of map operation for both of status/control data
for runtime of PCM substream, applications need to use fallback buffer
for an alternative ioctl. However, in current implementation, status
mapping is dominant to the allocation.

This commit moves code for the allocation outside of the mapping
operation for status data.
---
 src/pcm/pcm_hw.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 1d34956c..a3d1f137 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -867,21 +867,18 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
 	return xfern.result;
 }
 
-static int map_status_data(snd_pcm_hw_t *hw)
+static int map_status_data(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *sync_ptr)
 {
 	void *ptr;
-	int err;
+
 	ptr = MAP_FAILED;
 	if (hw->sync_ptr_ioctl == 0)
 		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_status)),
 			   PROT_READ, MAP_FILE|MAP_SHARED, 
 			   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
 	if (ptr == MAP_FAILED || ptr == NULL) {
-		hw->sync_ptr = calloc(1, sizeof(struct snd_pcm_sync_ptr));
-		if (hw->sync_ptr == NULL)
-			return -ENOMEM;
-		hw->mmap_status = &hw->sync_ptr->s.status;
-		hw->mmap_control = &hw->sync_ptr->c.control;
+		hw->mmap_status = &sync_ptr->s.status;
+		hw->mmap_control = &sync_ptr->c.control;
 		hw->sync_ptr_ioctl = 1;
 	} else {
 		hw->mmap_status = ptr;
@@ -894,7 +891,7 @@ static int map_control_data(snd_pcm_hw_t *hw)
 {
 	void *ptr;
 	int err;
-	if (hw->sync_ptr == NULL) {
+	if (hw->sync_ptr_ioctl == 0) {
 		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_control)),
 			   PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED, 
 			   hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
@@ -912,11 +909,18 @@ static int map_control_data(snd_pcm_hw_t *hw)
 static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 {
 	snd_pcm_hw_t *hw = pcm->private_data;
+	struct snd_pcm_sync_ptr *sync_ptr;
 	int err;
 
+	/* Preparation for fallback to failure of mmap(2). */
+	sync_ptr = malloc(sizeof(*sync_ptr));
+	if (sync_ptr == NULL)
+		return -ENOMEM;
+	memset(sync_ptr, 0, sizeof(*sync_ptr));
+
 	hw->sync_ptr_ioctl = (int)force_fallback;
 
-	err = map_status_data(hw);
+	err = map_status_data(hw, sync_ptr);
 	if (err < 0)
 		return err;
 
@@ -924,6 +928,14 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 	if (err < 0)
 		return err;
 
+	/* Any fallback mode needs to keep the buffer. */
+	if (hw->sync_ptr_ioctl == 0) {
+		hw->sync_ptr = sync_ptr;
+	} else {
+		free(sync_ptr);
+		hw->sync_ptr = NULL;
+	}
+
 	/* Initialize the data. */
 	hw->mmap_control->appl_ptr = 0;
 	hw->mmap_control->avail_min = 1;
-- 
2.11.0

[PATCH v2 alsa-lib 5/6] pcm: hw: maintain fallback mode for status data mapping

[Takashi Sakamoto]

In current implementation, results to map status/control data are not
maintained separately. It's handled as a fatal error that mapping of status
data is successful and mapping of control data is failed. However, it's
possible to handle this case by utilizing fallback buffer.

This commit adds a member into a local structure to maintain fallback mode
just for the mapping of status data as a preparation of later commit, in
which mapping results are maintained separately for each of status/control
data.
---
 src/pcm/pcm_hw.c | 78 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index a3d1f137..78857941 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -91,10 +91,12 @@ typedef struct {
 	int version;
 	int fd;
 	int card, device, subdevice;
-	int sync_ptr_ioctl;
+
 	volatile struct snd_pcm_mmap_status * mmap_status;
 	struct snd_pcm_mmap_control *mmap_control;
+	bool mmap_status_fallbacked;
 	struct snd_pcm_sync_ptr *sync_ptr;
+
 	int period_event;
 	snd_timer_t *period_timer;
 	struct pollfd period_timer_pfd;
@@ -867,42 +869,53 @@ static snd_pcm_sframes_t snd_pcm_hw_readn(snd_pcm_t *pcm, void **bufs, snd_pcm_u
 	return xfern.result;
 }
 
-static int map_status_data(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *sync_ptr)
+static bool map_status_data(snd_pcm_hw_t *hw, struct snd_pcm_sync_ptr *sync_ptr,
+			    bool force_fallback)
 {
-	void *ptr;
+	struct snd_pcm_mmap_status *mmap_status;
+	bool fallbacked;
+
+	mmap_status = MAP_FAILED;
+	if (!force_fallback) {
+		mmap_status = mmap(NULL, page_align(sizeof(*mmap_status)),
+				   PROT_READ, MAP_FILE|MAP_SHARED,
+				   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
+	}
 
-	ptr = MAP_FAILED;
-	if (hw->sync_ptr_ioctl == 0)
-		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_status)),
-			   PROT_READ, MAP_FILE|MAP_SHARED, 
-			   hw->fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
-	if (ptr == MAP_FAILED || ptr == NULL) {
-		hw->mmap_status = &sync_ptr->s.status;
-		hw->mmap_control = &sync_ptr->c.control;
-		hw->sync_ptr_ioctl = 1;
+	if (mmap_status == MAP_FAILED || mmap_status == NULL) {
+		mmap_status = &sync_ptr->s.status;
+		fallbacked = true;
 	} else {
-		hw->mmap_status = ptr;
+		fallbacked = false;
 	}
 
-	return 0;
+	hw->mmap_status = mmap_status;
+
+	return fallbacked;
 }
 
-static int map_control_data(snd_pcm_hw_t *hw)
+static bool map_control_data(snd_pcm_hw_t *hw,
+			     struct snd_pcm_sync_ptr *sync_ptr,
+			     bool force_fallback)
 {
-	void *ptr;
+	struct snd_pcm_mmap_control *mmap_control;
 	int err;
-	if (hw->sync_ptr_ioctl == 0) {
-		ptr = mmap(NULL, page_align(sizeof(struct snd_pcm_mmap_control)),
-			   PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED, 
-			   hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
-		if (ptr == MAP_FAILED || ptr == NULL) {
+
+	if (!force_fallback) {
+		mmap_control = mmap(NULL, page_align(sizeof(*mmap_control)),
+				    PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED,
+				    hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
+		if (mmap_control == MAP_FAILED || mmap_control == NULL) {
 			err = -errno;
 			SYSMSG("control mmap failed (%i)", err);
 			return err;
 		}
-		hw->mmap_control = ptr;
+	} else {
+		mmap_control = &sync_ptr->c.control;
 	}
 
+	hw->mmap_control = mmap_control;
+
 	return 0;
 }
 
@@ -918,18 +931,14 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 		return -ENOMEM;
 	memset(sync_ptr, 0, sizeof(*sync_ptr));
 
-	hw->sync_ptr_ioctl = (int)force_fallback;
-
-	err = map_status_data(hw, sync_ptr);
-	if (err < 0)
-		return err;
-
-	err = map_control_data(hw);
+	hw->mmap_status_fallbacked =
+			map_status_data(hw, sync_ptr, force_fallback);
+	err = map_control_data(hw, sync_ptr, hw->mmap_status_fallbacked);
 	if (err < 0)
 		return err;
 
 	/* Any fallback mode needs to keep the buffer. */
-	if (hw->sync_ptr_ioctl == 0) {
+	if (hw->mmap_status_fallbacked == 0) {
 		hw->sync_ptr = sync_ptr;
 	} else {
 		free(sync_ptr);
@@ -944,7 +953,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 				offsetof(struct snd_pcm_mmap_status, hw_ptr));
 	snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd,
 			     SNDRV_PCM_MMAP_OFFSET_CONTROL);
-	if (hw->sync_ptr_ioctl) {
+	if (hw->mmap_status_fallbacked) {
 		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, hw->sync_ptr) < 0) {
 			err = -errno;
 			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
@@ -957,7 +966,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 
 static void unmap_status_data(snd_pcm_hw_t *hw)
 {
-	if (!hw->sync_ptr) {
+	if (!hw->mmap_status_fallbacked) {
 		if (munmap((void *)hw->mmap_status,
 			   page_align(sizeof(*hw->mmap_status))) < 0)
 			SYSMSG("status munmap failed (%u)", errno);
@@ -966,7 +975,7 @@ static void unmap_status_data(snd_pcm_hw_t *hw)
 
 static void unmap_control_data(snd_pcm_hw_t *hw)
 {
-	if (!hw->sync_ptr) {
+	if (!hw->mmap_status_fallbacked) {
 		if (munmap((void *)hw->mmap_control,
 			   page_align(sizeof(*hw->mmap_control))) < 0)
 			SYSMSG("control munmap failed (%u)", errno);
@@ -978,11 +987,12 @@ static void unmap_status_and_control_data(snd_pcm_hw_t *hw)
 	unmap_status_data(hw);
 	unmap_control_data(hw);
 
-	if (hw->sync_ptr)
+	if (hw->mmap_status_fallbacked)
 		free(hw->sync_ptr);
 
 	hw->mmap_status = NULL;
 	hw->mmap_control = NULL;
+	hw->mmap_status_fallbacked = false;
 	hw->sync_ptr = NULL;
 }
 
-- 
2.11.0

[PATCH v2 alsa-lib 6/6] pcm: hw: maintain fallback mode for control data mapping independently

[Takashi Sakamoto]

Currently, failures of status/control data mapping are handled dependently.
However, it's not sure that one of the operations is failed when another
is failed.

This commit adds a member into private data structure to maintain fallback
mode for control data mapping, independently of status data mapping. As a
result, we have four cases to handle status/control data:

1. both of status/control data are mapped.
Nothing changed. A structure with alias of 'snd_pcm_hw_t' already has two
members to point the mapped area and in application runtime they're used
to refer/set status/control data. No need to call ioctl(2) with
SNDRV_PCM_IOCTL_SYNC_PTR to issue/query the data.

2. both of status/control data are unmapped.
The two members point to allocated memory for fallback buffer. In
application runtime, the buffer is given as an argument for ioctl(2) with
SNDRV_PCM_IOCTL_SYNC_PTR to issue/query the data.

3. status data is mapped only.
One of the two members is used to point the mapped area. Another points to
allocated memory for fallback buffer. In application runtime, the buffer
is used as an argument to execute ioctl(2) with SNDRV_PCM_IOCTL_SYNC_PTR
for the latter data, but the former data is already synchronized.

4. control data is mapped only.
The same as the above.

In design of ALSA PCM interface, userspace applications are not expected
to map the status data as writable. On the other hand, expected to map
the control data as writable. In a focus on the differences, we could
achieve to reduce calls of the ioctl(2) in a case that one of the
status/control data is successfully mapped and another is failed (case 3
and 4). Especially, in current alsa-lib implementation, application
runtime queries state of runtime of PCM substream so often.
---
 src/pcm/pcm_hw.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/src/pcm/pcm_hw.c b/src/pcm/pcm_hw.c
index 78857941..5573fce2 100644
--- a/src/pcm/pcm_hw.c
+++ b/src/pcm/pcm_hw.c
@@ -95,6 +95,7 @@ typedef struct {
 	volatile struct snd_pcm_mmap_status * mmap_status;
 	struct snd_pcm_mmap_control *mmap_control;
 	bool mmap_status_fallbacked;
+	bool mmap_control_fallbacked;
 	struct snd_pcm_sync_ptr *sync_ptr;
 
 	int period_event;
@@ -143,9 +144,11 @@ static int sync_ptr1(snd_pcm_hw_t *hw, unsigned int flags)
 	return 0;
 }
 
-static inline int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags)
+static int sync_ptr(snd_pcm_hw_t *hw, unsigned int flags)
 {
-	return hw->sync_ptr ? sync_ptr1(hw, flags) : 0;
+	if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked)
+		return sync_ptr1(hw, flags);
+	return 0;
 }
 
 static int snd_pcm_hw_clear_timer_queue(snd_pcm_hw_t *hw)
@@ -899,24 +902,24 @@ static bool map_control_data(snd_pcm_hw_t *hw,
 			     bool force_fallback)
 {
 	struct snd_pcm_mmap_control *mmap_control;
-	int err;
+	bool fallbacked;
 
 	if (!force_fallback) {
 		mmap_control = mmap(NULL, page_align(sizeof(*mmap_control)),
 				    PROT_READ|PROT_WRITE, MAP_FILE|MAP_SHARED,
 				    hw->fd, SNDRV_PCM_MMAP_OFFSET_CONTROL);
-		if (mmap_control == MAP_FAILED || mmap_control == NULL) {
-			err = -errno;
-			SYSMSG("control mmap failed (%i)", err);
-			return err;
-		}
-	} else {
+	}
+
+	if (mmap_control == MAP_FAILED || mmap_control == NULL) {
 		mmap_control = &sync_ptr->c.control;
+		fallbacked = true;
+	} else {
+		fallbacked = false;
 	}
 
 	hw->mmap_control = mmap_control;
 
-	return 0;
+	return fallbacked;
 }
 
 static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
@@ -933,12 +936,11 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 
 	hw->mmap_status_fallbacked =
 			map_status_data(hw, sync_ptr, force_fallback);
-	err = map_control_data(hw, sync_ptr, hw->mmap_status_fallbacked);
-	if (err < 0)
-		return err;
+	hw->mmap_control_fallbacked =
+			map_control_data(hw, sync_ptr, force_fallback);
 
 	/* Any fallback mode needs to keep the buffer. */
-	if (hw->mmap_status_fallbacked == 0) {
+	if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked) {
 		hw->sync_ptr = sync_ptr;
 	} else {
 		free(sync_ptr);
@@ -953,7 +955,7 @@ static int map_status_and_control_data(snd_pcm_t *pcm, bool force_fallback)
 				offsetof(struct snd_pcm_mmap_status, hw_ptr));
 	snd_pcm_set_appl_ptr(pcm, &hw->mmap_control->appl_ptr, hw->fd,
 			     SNDRV_PCM_MMAP_OFFSET_CONTROL);
-	if (hw->mmap_status_fallbacked) {
+	if (hw->mmap_control_fallbacked) {
 		if (ioctl(hw->fd, SNDRV_PCM_IOCTL_SYNC_PTR, hw->sync_ptr) < 0) {
 			err = -errno;
 			SYSMSG("SNDRV_PCM_IOCTL_SYNC_PTR failed (%i)", err);
@@ -975,7 +977,7 @@ static void unmap_status_data(snd_pcm_hw_t *hw)
 
 static void unmap_control_data(snd_pcm_hw_t *hw)
 {
-	if (!hw->mmap_status_fallbacked) {
+	if (!hw->mmap_control_fallbacked) {
 		if (munmap((void *)hw->mmap_control,
 			   page_align(sizeof(*hw->mmap_control))) < 0)
 			SYSMSG("control munmap failed (%u)", errno);
@@ -987,12 +989,13 @@ static void unmap_status_and_control_data(snd_pcm_hw_t *hw)
 	unmap_status_data(hw);
 	unmap_control_data(hw);
 
-	if (hw->mmap_status_fallbacked)
+	if (hw->mmap_status_fallbacked || hw->mmap_control_fallbacked)
 		free(hw->sync_ptr);
 
 	hw->mmap_status = NULL;
 	hw->mmap_control = NULL;
 	hw->mmap_status_fallbacked = false;
+	hw->mmap_control_fallbacked = false;
 	hw->sync_ptr = NULL;
 }
 
-- 
2.11.0

Re: [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 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?


thanks,

Takashi

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

[Takashi Sakamoto]

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)


Thanks

Takashi Sakamoto

Re: [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

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

[Takashi Sakamoto]

Hi,

On Jun 27 2017 17:15, Takashi Iwai wrote:
> 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).

Yep. These codes were originally written with a loose manner. But it 
would be allowed because both 'mmap(NULL, non-zero)' and 'free(NULL)' 
generates no error and 'sync_ptr_ioctl' works fine, and 'sync_ptr' is 
assigned to NULL after deallocated.

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

For your information.

======== 8< --------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#include <string.h>
#include <limits.h>
#include <errno.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#include <sys/ioctl.h>
#include <sys/mman.h>

#include <sound/asound.h>

int main(void)
{
     int fd;
     size_t page_size = sysconf(_SC_PAGESIZE);
     int pages;
     void *status;
     int status_length;
     void *control;
     int control_length;

     fd = open("/dev/snd/pcmC0D0p", O_RDWR);
     if (fd < 0) {
         printf("open(2): %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Map status data. */
     pages = (sizeof(struct snd_pcm_mmap_status) + page_size - 1) / 
page_size;
     status_length = pages * page_size;
     status = MAP_FAILED;
     status = mmap(NULL, status_length, PROT_READ, MAP_FILE | MAP_SHARED,
                   fd, SNDRV_PCM_MMAP_OFFSET_STATUS);
     if (status == MAP_FAILED || status == NULL) {
         close(fd);
         printf("mmap(2): status: %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Map control data. */
     pages = (sizeof(struct snd_pcm_mmap_control) + page_size - 1) / 
page_size;
     control_length = pages * page_size;
     control = MAP_FAILED;
     control = mmap(NULL, control_length, PROT_READ | PROT_WRITE,
                    MAP_FILE | MAP_SHARED, fd, 
SNDRV_PCM_MMAP_OFFSET_CONTROL);
     if (status == MAP_FAILED || status == NULL) {
         munmap(status, status_length);
         close(fd);
         printf("mmap(2): control: %s\n", strerror(errno));
         return EXIT_FAILURE;
     }

     /* Not error. */
     if (munmap(NULL, 10000) < 0)
         printf("unexpected: munmap(2): NULL: %s\n", strerror(errno));

     /* Generate error. */
     if (munmap(NULL, 0) < 0)
         printf("expected: munmap(2): NULL: %s\n", strerror(errno));
     /* Ditto. */
     if (munmap(control, 0) < 0)
         printf("expected: munmap(2): control: 0: %s\n", strerror(errno));
     /* Ditto. */
     if (munmap(status, 0) < 0)
         printf("expected: munmap(2): status: 0: %s\n", strerror(errno));

     if (munmap(control, control_length) < 0)
         printf("unexpected: munmap(2): control: 0: %s\n", strerror(errno));
     if (munmap(status, status_length) < 0)
         printf("unexpected: munmap(2): status: 0: %s\n", strerror(errno));
     close(fd);

     /* Not error. */
     free(NULL);

     return EXIT_SUCCESS;
}
======== 8< --------


Regards

Takashi Sakamoto

Re: [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:57:55 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 27 2017 17:15, Takashi Iwai wrote:
> > 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).
> 
> Yep. These codes were originally written with a loose manner. But it
> would be allowed because both 'mmap(NULL, non-zero)' and 'free(NULL)'
> generates no error and 'sync_ptr_ioctl' works fine, and 'sync_ptr' is
> assigned to NULL after deallocated.

Yes, that's a bit fragile way, but it works.

BTW, the whole patchset seems missing your sign-off.
I can add it manually, though.


Takashi

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

[Takashi Sakamoto]

Hi,

On Jun 27 2017 18:10, Takashi Iwai wrote:
> On Tue, 27 Jun 2017 10:57:55 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Jun 27 2017 17:15, Takashi Iwai wrote:
>>> 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).
>>
>> Yep. These codes were originally written with a loose manner. But it
>> would be allowed because both 'mmap(NULL, non-zero)' and 'free(NULL)'
>> generates no error and 'sync_ptr_ioctl' works fine, and 'sync_ptr' is
>> assigned to NULL after deallocated.
> 
> Yes, that's a bit fragile way, but it works.
> 
> BTW, the whole patchset seems missing your sign-off.
> I can add it manually, though.

Aieee... I would ask you to do it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>


Thanks

Takashi Sakamoto