[PATCH 0/8] ALSA: pcm: code refactoring for snd_pcm_hw_refine()

[PATCH 0/8] ALSA: pcm: code refactoring for snd_pcm_hw_refine()

[Takashi Sakamoto]

Hi,

This patchset is an update of part of my RFC, and goes for upstream.

[alsa-devel] [PATCH RFC 00/21] ALSA: pcm: add tracepoints for PCM params operation
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120548.html

In ALSA PCM interface, applications can get hardware capability as
'struct snd_pcm_hw_params' type of data returned by a call of ioctl(2)
with SNDRV_PCM_IOCTL_HW_REFINE/SNDRV_PCM_IOCTL_HW_PARAMS' commands. In
kernel land, the data is prepared in 'snd_pcm_hw_refine()' in
'sound/core/pcm_native.c'. This patchset is a code refactoring to this
function.

When applications execute open(2) to any PCM character device, runtime
of corresponding PCM substream is created in kernel land. Then, the
runtime gets information to configure the parameter data. When
applications execute ioctl(2) with the above commands, the given
parameter data is actually changed according to the information.

The parameter data structure includes several types of information. 
Major part of the parameter data is represented by two kinds of shape;
mask ('struct snd_mask') and interval ('struct snd_interval'). 15
types of data is pre-defined as 'SNDRV_PCM_HW_PARAM_XXX' macros with
the two types.

The runtime has several constraints, which is a kind of information to
configure the parameters. There're two types of constraints; independent
and dependent. The independent constrain is for each of single parameters.
On the other hand, the dependent constrain is for each of combination
between parameters, thus has specific structure, 'snd_pcm_hw_rule'. This
structure describes relationships between several parameters.

In 'sound/core/pcm_native.c', the above constraints and rules are
applied to parameter data for userspace. This processing is done
in 'snd_pcm_hw_refine()'. In this patchset, I attempt to clean up the
processing so that developers easily understand it.

This is a kind of refactoring, thus behaviour of the processing is not
changed. Tracepoints added in my previous patchset is helpful for your
test to check it. I wrote an application as low level program and
it shall satisfies your interests about the above mechanism[1].

[1] refine-pcm-params.c
https://github.com/takaswie/alsa-ioctl-test/

Takashi Sakamoto (8):
  ALSA: pcm: add a helper function to constrain mask-type parameters
  ALSA: pcm: add a helper function to constrain interval-type parameters
  ALSA: pcm: add a helper function to apply parameter rules
  ALSA: pcm: use goto statement instead of while statement to reduce
    indentation
  ALSA: pcm: remove function local variable with alternative evaluation
  ALSA: pcm: adaption of code formatting
  ALSA: pcm: use helper functions to check whether parameters are
    determined
  ALSA: pcm: add comment about application of rule to PCM parameters

 sound/core/pcm_native.c | 253 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 179 insertions(+), 74 deletions(-)

-- 
2.11.0

[PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Sakamoto]

Application of constraints to mask-type parameters for PCM substream is
done in a call of snd_pcm_hw_refine(), while the function includes much
codes and is not enough friendly to readers.

This commit splits the codes to a separated function so that readers can
get it easily. I leave desicion into compilers to merge the function into
its callee.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 2bde07a4a87f..b3e8aab3915e 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -253,6 +253,40 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
 	return true;
 }
 
+static int constrain_mask_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params)
+{
+	struct snd_pcm_hw_constraints *constrs =
+					&substream->runtime->hw_constraints;
+	struct snd_mask *m;
+	unsigned int k;
+	int changed;
+
+	struct snd_mask __maybe_unused old_mask;
+
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
+		m = hw_param_mask(params, k);
+		if (snd_mask_empty(m))
+			return -EINVAL;
+		if (!(params->rmask & (1 << k)))
+			continue;
+
+		if (trace_hw_mask_param_enabled())
+			old_mask = *m;
+
+		changed = snd_mask_refine(m, constrs_mask(constrs, k));
+
+		trace_hw_mask_param(substream, k, 0, &old_mask, m);
+
+		if (changed)
+			params->cmask |= 1 << k;
+		if (changed < 0)
+			return changed;
+	}
+
+	return 0;
+}
+
 int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
 		      struct snd_pcm_hw_params *params)
 {
@@ -265,6 +299,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
 	unsigned int stamp = 2;
 	int changed, again;
+	int err;
 
 	struct snd_mask __maybe_unused old_mask;
 	struct snd_interval __maybe_unused old_interval;
@@ -278,25 +313,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		params->rate_den = 0;
 	}
 
-	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
-		m = hw_param_mask(params, k);
-		if (snd_mask_empty(m))
-			return -EINVAL;
-		if (!(params->rmask & (1 << k)))
-			continue;
-
-		if (trace_hw_mask_param_enabled())
-			old_mask = *m;
-
-		changed = snd_mask_refine(m, constrs_mask(constrs, k));
-
-		trace_hw_mask_param(substream, k, 0, &old_mask, m);
-
-		if (changed)
-			params->cmask |= 1 << k;
-		if (changed < 0)
-			return changed;
-	}
+	err = constrain_mask_params(substream, params);
+	if (err < 0)
+		return err;
 
 	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
 		i = hw_param_interval(params, k);
-- 
2.11.0

[PATCH 2/8] ALSA: pcm: add a helper function to constrain interval-type parameters

[Takashi Sakamoto]

Application of constraints to interval-type parameters for PCM substream
is done in a call of snd_pcm_hw_refine(), while the function includes
much codes and is not enough friendly to readers.

This commit splits the codes to a separated function so that readers can
get it easily. I leave desicion into compilers to merge the function into
its callee.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 56 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index b3e8aab3915e..049b943c9e7b 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -287,6 +287,40 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int constrain_interval_params(struct snd_pcm_substream *substream,
+				     struct snd_pcm_hw_params *params)
+{
+	struct snd_pcm_hw_constraints *constrs =
+					&substream->runtime->hw_constraints;
+	struct snd_interval *i;
+	unsigned int k;
+	int changed;
+
+	struct snd_interval __maybe_unused old_interval;
+
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
+		i = hw_param_interval(params, k);
+		if (snd_interval_empty(i))
+			return -EINVAL;
+		if (!(params->rmask & (1 << k)))
+			continue;
+
+		if (trace_hw_interval_param_enabled())
+			old_interval = *i;
+
+		changed = snd_interval_refine(i, constrs_interval(constrs, k));
+
+		trace_hw_interval_param(substream, k, 0, &old_interval, i);
+
+		if (changed)
+			params->cmask |= 1 << k;
+		if (changed < 0)
+			return changed;
+	}
+
+	return 0;
+}
+
 int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
 		      struct snd_pcm_hw_params *params)
 {
@@ -317,25 +351,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	if (err < 0)
 		return err;
 
-	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
-		i = hw_param_interval(params, k);
-		if (snd_interval_empty(i))
-			return -EINVAL;
-		if (!(params->rmask & (1 << k)))
-			continue;
-
-		if (trace_hw_interval_param_enabled())
-			old_interval = *i;
-
-		changed = snd_interval_refine(i, constrs_interval(constrs, k));
-
-		trace_hw_interval_param(substream, k, 0, &old_interval, i);
-
-		if (changed)
-			params->cmask |= 1 << k;
-		if (changed < 0)
-			return changed;
-	}
+	err = constrain_interval_params(substream, params);
+	if (err < 0)
+		return err;
 
 	for (k = 0; k < constrs->rules_num; k++)
 		rstamps[k] = 0;
-- 
2.11.0

[PATCH 3/8] ALSA: pcm: add a helper function to apply parameter rules

[Takashi Sakamoto]

Application of rules to parameters of PCM substream is done in a call of
snd_pcm_hw_refine(), while the function includes much codes and is not
enough friendly to readers.

This commit splits the codes to a separated function so that readers can
get it easily. I leave desicion into compilers to merge the function into
its callee.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 68 +++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 049b943c9e7b..76041faf95df 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -321,40 +321,21 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
-int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
-		      struct snd_pcm_hw_params *params)
+static int constrain_params_by_rules(struct snd_pcm_substream *substream,
+				     struct snd_pcm_hw_params *params)
 {
+	struct snd_pcm_hw_constraints *constrs =
+					&substream->runtime->hw_constraints;
 	unsigned int k;
-	struct snd_pcm_hardware *hw;
-	struct snd_interval *i = NULL;
-	struct snd_mask *m = NULL;
-	struct snd_pcm_hw_constraints *constrs = &substream->runtime->hw_constraints;
 	unsigned int rstamps[constrs->rules_num];
 	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
 	unsigned int stamp = 2;
-	int changed, again;
-	int err;
+	int again;
+	int changed;
 
 	struct snd_mask __maybe_unused old_mask;
 	struct snd_interval __maybe_unused old_interval;
 
-	params->info = 0;
-	params->fifo_size = 0;
-	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS))
-		params->msbits = 0;
-	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) {
-		params->rate_num = 0;
-		params->rate_den = 0;
-	}
-
-	err = constrain_mask_params(substream, params);
-	if (err < 0)
-		return err;
-
-	err = constrain_interval_params(substream, params);
-	if (err < 0)
-		return err;
-
 	for (k = 0; k < constrs->rules_num; k++)
 		rstamps[k] = 0;
 	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) 
@@ -407,6 +388,43 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 			stamp++;
 		}
 	} while (again);
+
+	return 0;
+}
+
+int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
+		      struct snd_pcm_hw_params *params)
+{
+	struct snd_pcm_hardware *hw;
+	struct snd_interval *i = NULL;
+	struct snd_mask *m = NULL;
+	int changed;
+	int err;
+
+	struct snd_mask __maybe_unused old_mask;
+	struct snd_interval __maybe_unused old_interval;
+
+	params->info = 0;
+	params->fifo_size = 0;
+	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS))
+		params->msbits = 0;
+	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) {
+		params->rate_num = 0;
+		params->rate_den = 0;
+	}
+
+	err = constrain_mask_params(substream, params);
+	if (err < 0)
+		return err;
+
+	err = constrain_interval_params(substream, params);
+	if (err < 0)
+		return err;
+
+	err = constrain_params_by_rules(substream, params);
+	if (err < 0)
+		return err;
+
 	if (!params->msbits) {
 		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
 		if (snd_interval_single(i))
-- 
2.11.0

[PATCH 4/8] ALSA: pcm: use goto statement instead of while statement to reduce indentation

[Takashi Sakamoto]

In a process to calculate parameters of PCM substream, application of all
rules is iterated several times till parameter dependencies are satisfied.
In current implementation, two loops are used for the design, however this
brings two-level indentation and decline readability.

This commit attempts to reduce the indentation by using goto statement,
instead of outer while loop.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 86 +++++++++++++++++++++++++------------------------
 1 file changed, 44 insertions(+), 42 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 76041faf95df..ae6499f1a049 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -340,54 +340,56 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 		rstamps[k] = 0;
 	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) 
 		vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0;
-	do {
-		again = 0;
-		for (k = 0; k < constrs->rules_num; k++) {
-			struct snd_pcm_hw_rule *r = &constrs->rules[k];
-			unsigned int d;
-			int doit = 0;
-			if (r->cond && !(r->cond & params->flags))
-				continue;
-			for (d = 0; r->deps[d] >= 0; d++) {
-				if (vstamps[r->deps[d]] > rstamps[k]) {
-					doit = 1;
-					break;
-				}
+retry:
+	again = 0;
+	for (k = 0; k < constrs->rules_num; k++) {
+		struct snd_pcm_hw_rule *r = &constrs->rules[k];
+		unsigned int d;
+		int doit = 0;
+		if (r->cond && !(r->cond & params->flags))
+			continue;
+		for (d = 0; r->deps[d] >= 0; d++) {
+			if (vstamps[r->deps[d]] > rstamps[k]) {
+				doit = 1;
+				break;
 			}
-			if (!doit)
-				continue;
+		}
+		if (!doit)
+			continue;
 
-			if (trace_hw_mask_param_enabled()) {
-				if (hw_is_mask(r->var))
-					old_mask = *hw_param_mask(params, r->var);
-			}
-			if (trace_hw_interval_param_enabled()) {
-				if (hw_is_interval(r->var))
-					old_interval = *hw_param_interval(params, r->var);
-			}
+		if (trace_hw_mask_param_enabled()) {
+			if (hw_is_mask(r->var))
+				old_mask = *hw_param_mask(params, r->var);
+		}
+		if (trace_hw_interval_param_enabled()) {
+			if (hw_is_interval(r->var))
+				old_interval = *hw_param_interval(params, r->var);
+		}
 
-			changed = r->func(params, r);
+		changed = r->func(params, r);
 
-			if (hw_is_mask(r->var)) {
-				trace_hw_mask_param(substream, r->var, k + 1,
-					&old_mask, hw_param_mask(params, r->var));
-			}
-			if (hw_is_interval(r->var)) {
-				trace_hw_interval_param(substream, r->var, k + 1,
-					&old_interval, hw_param_interval(params, r->var));
-			}
+		if (hw_is_mask(r->var)) {
+			trace_hw_mask_param(substream, r->var, k + 1,
+				&old_mask, hw_param_mask(params, r->var));
+		}
+		if (hw_is_interval(r->var)) {
+			trace_hw_interval_param(substream, r->var, k + 1,
+				&old_interval, hw_param_interval(params, r->var));
+		}
 
-			rstamps[k] = stamp;
-			if (changed && r->var >= 0) {
-				params->cmask |= (1 << r->var);
-				vstamps[r->var] = stamp;
-				again = 1;
-			}
-			if (changed < 0)
-				return changed;
-			stamp++;
+		rstamps[k] = stamp;
+		if (changed && r->var >= 0) {
+			params->cmask |= (1 << r->var);
+			vstamps[r->var] = stamp;
+			again = 1;
 		}
-	} while (again);
+		if (changed < 0)
+			return changed;
+		stamp++;
+	}
+
+	if (again)
+		goto retry;
 
 	return 0;
 }
-- 
2.11.0

[PATCH 5/8] ALSA: pcm: remove function local variable with alternative evaluation

[Takashi Sakamoto]

A local variable is used to judge whether a parameter should be handled
due to reverse dependency of the other rules. However, this can be
obsoleted by check of a sentinel in dependency array.

This commit removes the local variable and check the sentinel to reduce
stack usage.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ae6499f1a049..6fa5947d7db8 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -345,16 +345,13 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 	for (k = 0; k < constrs->rules_num; k++) {
 		struct snd_pcm_hw_rule *r = &constrs->rules[k];
 		unsigned int d;
-		int doit = 0;
 		if (r->cond && !(r->cond & params->flags))
 			continue;
 		for (d = 0; r->deps[d] >= 0; d++) {
-			if (vstamps[r->deps[d]] > rstamps[k]) {
-				doit = 1;
+			if (vstamps[r->deps[d]] > rstamps[k])
 				break;
-			}
 		}
-		if (!doit)
+		if (r->deps[d] < 0)
 			continue;
 
 		if (trace_hw_mask_param_enabled()) {
-- 
2.11.0

[PATCH 6/8] ALSA: pcm: adaption of code formatting

[Takashi Sakamoto]

This commit modifies current code according to fashion in kernel land:
 - within 80 characters per line as possible
 - removal of trailing linear white spaces
 - use int type variable for loop counter
 - use prefix operator for numerical increment

For readability:
 - use bool type variable instead of int type variable assigned to 0/1
 - move variable definition from loop to top of the function definition

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 6fa5947d7db8..7c74c293cab7 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -259,12 +259,13 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_constraints *constrs =
 					&substream->runtime->hw_constraints;
 	struct snd_mask *m;
-	unsigned int k;
+	int k;
 	int changed;
 
 	struct snd_mask __maybe_unused old_mask;
 
-	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK;
+	     k <= SNDRV_PCM_HW_PARAM_LAST_MASK; ++k) {
 		m = hw_param_mask(params, k);
 		if (snd_mask_empty(m))
 			return -EINVAL;
@@ -293,12 +294,13 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_constraints *constrs =
 					&substream->runtime->hw_constraints;
 	struct snd_interval *i;
-	unsigned int k;
+	int k;
 	int changed;
 
 	struct snd_interval __maybe_unused old_interval;
 
-	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
+	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL;
+	     k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; ++k) {
 		i = hw_param_interval(params, k);
 		if (snd_interval_empty(i))
 			return -EINVAL;
@@ -326,25 +328,28 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 {
 	struct snd_pcm_hw_constraints *constrs =
 					&substream->runtime->hw_constraints;
-	unsigned int k;
+	int k;
 	unsigned int rstamps[constrs->rules_num];
 	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
-	unsigned int stamp = 2;
-	int again;
+	unsigned int stamp;
+	struct snd_pcm_hw_rule *r;
+	int d;
+	bool again;
 	int changed;
 
 	struct snd_mask __maybe_unused old_mask;
 	struct snd_interval __maybe_unused old_interval;
 
-	for (k = 0; k < constrs->rules_num; k++)
+	for (k = 0; k < constrs->rules_num; ++k)
 		rstamps[k] = 0;
-	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) 
+	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; ++k)
 		vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0;
+
+	stamp = 2;
 retry:
-	again = 0;
+	again = false;
 	for (k = 0; k < constrs->rules_num; k++) {
-		struct snd_pcm_hw_rule *r = &constrs->rules[k];
-		unsigned int d;
+		r = &constrs->rules[k];
 		if (r->cond && !(r->cond & params->flags))
 			continue;
 		for (d = 0; r->deps[d] >= 0; d++) {
@@ -378,7 +383,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 		if (changed && r->var >= 0) {
 			params->cmask |= (1 << r->var);
 			vstamps[r->var] = stamp;
-			again = 1;
+			again = true;
 		}
 		if (changed < 0)
 			return changed;
@@ -397,7 +402,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	struct snd_pcm_hardware *hw;
 	struct snd_interval *i = NULL;
 	struct snd_mask *m = NULL;
-	int changed;
 	int err;
 
 	struct snd_mask __maybe_unused old_mask;
@@ -451,16 +455,15 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
 		if (snd_mask_min(m) == snd_mask_max(m) &&
                     snd_interval_min(i) == snd_interval_max(i)) {
-			changed = substream->ops->ioctl(substream,
+			err = substream->ops->ioctl(substream,
 					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
-			if (changed < 0)
-				return changed;
+			if (err < 0)
+				return err;
 		}
 	}
 	params->rmask = 0;
 	return 0;
 }
-
 EXPORT_SYMBOL(snd_pcm_hw_refine);
 
 static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
-- 
2.11.0

[PATCH 7/8] ALSA: pcm: use helper functions to check whether parameters are determined

[Takashi Sakamoto]

A commit 8bea869c5e56 ("ALSA: PCM midlevel: improve fifo_size handling")
allows drivers to implement calculation of fifo size in parameter
structure. This calculation runs only when two of the other parameters
have single value.

In ALSA PCM core, there're some helper functions for the case. This commit
applies the functions instead of value comparison.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 7c74c293cab7..a56319df89d9 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -453,8 +453,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	if (!params->fifo_size) {
 		m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
 		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
-		if (snd_mask_min(m) == snd_mask_max(m) &&
-                    snd_interval_min(i) == snd_interval_max(i)) {
+		if (snd_mask_single(m) && snd_interval_single(i)) {
 			err = substream->ops->ioctl(substream,
 					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
 			if (err < 0)
-- 
2.11.0

[PATCH 8/8] ALSA: pcm: add comment about application of rule to PCM parameters

[Takashi Sakamoto]

Drivers add rules of parameters to runtime of PCM substream, when
applications open ALSA PCM character device. When applications call
ioctl(2) with SNDRV_PCM_IOCTL_HW_REFINE or SNDRV_PCM_IOCTL_HW_PARAMS, the
rules are applied to the parameters and return the result to user space.

The rule can have dependency between parameters. Additionally, it can have
condition flags about application of rules. Userspace applications can
indicate the flags to suppress change of parameters.

This commit attempts to describe the mechanism in a shape of code
comments.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index a56319df89d9..8f0d9f237f20 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -269,6 +269,8 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
 		m = hw_param_mask(params, k);
 		if (snd_mask_empty(m))
 			return -EINVAL;
+
+		/* This parameter is not requested to change by a caller. */
 		if (!(params->rmask & (1 << k)))
 			continue;
 
@@ -279,6 +281,7 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
 
 		trace_hw_mask_param(substream, k, 0, &old_mask, m);
 
+		/* Set corresponding flag so that the caller gets it. */
 		if (changed)
 			params->cmask |= 1 << k;
 		if (changed < 0)
@@ -304,6 +307,8 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
 		i = hw_param_interval(params, k);
 		if (snd_interval_empty(i))
 			return -EINVAL;
+
+		/* This parameter is not requested to change by a caller. */
 		if (!(params->rmask & (1 << k)))
 			continue;
 
@@ -314,6 +319,7 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
 
 		trace_hw_interval_param(substream, k, 0, &old_interval, i);
 
+		/* Set corresponding flag so that the caller gets it. */
 		if (changed)
 			params->cmask |= 1 << k;
 		if (changed < 0)
@@ -340,18 +346,54 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 	struct snd_mask __maybe_unused old_mask;
 	struct snd_interval __maybe_unused old_interval;
 
+	/*
+	 * Each application of rule has own sequence number.
+	 *
+	 * Each member of 'rstamps' array represents the sequence number of
+	 * recent application of corresponding rule.
+	 */
 	for (k = 0; k < constrs->rules_num; ++k)
 		rstamps[k] = 0;
+
+	/*
+	 * Each member of 'vstamps' array represents the sequence number of
+	 * recent application of rule in which corresponding parameters were
+	 * changed.
+	 *
+	 * In initial state, elements corresponding to parameters requested by
+	 * a caller is 1. For unrequested parameters, corresponding members
+	 * have 0 so that the parameters are never changed anymore.
+	 */
 	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; ++k)
 		vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0;
 
+	/* Due to the above design, actual sequence number starts at 2. */
 	stamp = 2;
 retry:
+	/* Apply all rules in order. */
 	again = false;
 	for (k = 0; k < constrs->rules_num; k++) {
 		r = &constrs->rules[k];
+
+		/*
+		 * Check condition bits of this rule. When the rule has
+		 * some condition bits, parameter without the bits is
+		 * never processed. SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP
+		 * is an example of the condition bits.
+		 */
 		if (r->cond && !(r->cond & params->flags))
 			continue;
+
+		/*
+		 * The 'deps' array includes maximum three dependencies
+		 * to SNDRV_PCM_HW_PARAM_XXXs for this rule. The fourth
+		 * member of this array is a sentinel and should be
+		 * negative value.
+		 *
+		 * This rule should be processed in this time when dependent
+		 * parameters were changed at former applications of the other
+		 * rules.
+		 */
 		for (d = 0; r->deps[d] >= 0; d++) {
 			if (vstamps[r->deps[d]] > rstamps[k])
 				break;
@@ -380,6 +422,12 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 		}
 
 		rstamps[k] = stamp;
+
+		/*
+		 * When the parameters is changed, notify it to the caller
+		 * by corresponding returned bit, then preparing for next
+		 * iteration.
+		 */
 		if (changed && r->var >= 0) {
 			params->cmask |= (1 << r->var);
 			vstamps[r->var] = stamp;
@@ -390,6 +438,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 		stamp++;
 	}
 
+	/* Iterate to evaluate all rules till no parameters are changed. */
 	if (again)
 		goto retry;
 
-- 
2.11.0

Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Iwai]

On Thu, 08 Jun 2017 01:10:19 +0200,
Takashi Sakamoto wrote:
> 
> Application of constraints to mask-type parameters for PCM substream is
> done in a call of snd_pcm_hw_refine(), while the function includes much
> codes and is not enough friendly to readers.
> 
> This commit splits the codes to a separated function so that readers can
> get it easily. I leave desicion into compilers to merge the function into
> its callee.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 2bde07a4a87f..b3e8aab3915e 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -253,6 +253,40 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream)
>  	return true;
>  }
>  
> +static int constrain_mask_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params)
> +{
> +	struct snd_pcm_hw_constraints *constrs =
> +					&substream->runtime->hw_constraints;
> +	struct snd_mask *m;
> +	unsigned int k;
> +	int changed;
> +

A strange blank line is here.

> +	struct snd_mask __maybe_unused old_mask;

Do we really need __maybe_unused?  Drop it as much as possible.
IMO, it can be uglier than ifdef, since you don't know why it's
unused.  With ifdef, at least, you have an idea about the condition.


thanks,

Takashi

> +
> +	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
> +		m = hw_param_mask(params, k);
> +		if (snd_mask_empty(m))
> +			return -EINVAL;
> +		if (!(params->rmask & (1 << k)))
> +			continue;
> +
> +		if (trace_hw_mask_param_enabled())
> +			old_mask = *m;
> +
> +		changed = snd_mask_refine(m, constrs_mask(constrs, k));
> +
> +		trace_hw_mask_param(substream, k, 0, &old_mask, m);
> +
> +		if (changed)
> +			params->cmask |= 1 << k;
> +		if (changed < 0)
> +			return changed;
> +	}
> +
> +	return 0;
> +}
> +
>  int snd_pcm_hw_refine(struct snd_pcm_substream *substream, 
>  		      struct snd_pcm_hw_params *params)
>  {
> @@ -265,6 +299,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
>  	unsigned int stamp = 2;
>  	int changed, again;
> +	int err;
>  
>  	struct snd_mask __maybe_unused old_mask;
>  	struct snd_interval __maybe_unused old_interval;
> @@ -278,25 +313,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  		params->rate_den = 0;
>  	}
>  
> -	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
> -		m = hw_param_mask(params, k);
> -		if (snd_mask_empty(m))
> -			return -EINVAL;
> -		if (!(params->rmask & (1 << k)))
> -			continue;
> -
> -		if (trace_hw_mask_param_enabled())
> -			old_mask = *m;
> -
> -		changed = snd_mask_refine(m, constrs_mask(constrs, k));
> -
> -		trace_hw_mask_param(substream, k, 0, &old_mask, m);
> -
> -		if (changed)
> -			params->cmask |= 1 << k;
> -		if (changed < 0)
> -			return changed;
> -	}
> +	err = constrain_mask_params(substream, params);
> +	if (err < 0)
> +		return err;
>  
>  	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
>  		i = hw_param_interval(params, k);
> -- 
> 2.11.0
> 

Re: [PATCH 6/8] ALSA: pcm: adaption of code formatting

[Takashi Iwai]

On Thu, 08 Jun 2017 01:10:24 +0200,
Takashi Sakamoto wrote:
> 
> This commit modifies current code according to fashion in kernel land:
>  - within 80 characters per line as possible
>  - removal of trailing linear white spaces
>  - use int type variable for loop counter
>  - use prefix operator for numerical increment

This isn't always preferred, and I see no reason to change.


thanks,

Takashi

> For readability:
>  - use bool type variable instead of int type variable assigned to 0/1
>  - move variable definition from loop to top of the function definition
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/core/pcm_native.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 6fa5947d7db8..7c74c293cab7 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -259,12 +259,13 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
>  	struct snd_pcm_hw_constraints *constrs =
>  					&substream->runtime->hw_constraints;
>  	struct snd_mask *m;
> -	unsigned int k;
> +	int k;
>  	int changed;
>  
>  	struct snd_mask __maybe_unused old_mask;
>  
> -	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) {
> +	for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK;
> +	     k <= SNDRV_PCM_HW_PARAM_LAST_MASK; ++k) {
>  		m = hw_param_mask(params, k);
>  		if (snd_mask_empty(m))
>  			return -EINVAL;
> @@ -293,12 +294,13 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
>  	struct snd_pcm_hw_constraints *constrs =
>  					&substream->runtime->hw_constraints;
>  	struct snd_interval *i;
> -	unsigned int k;
> +	int k;
>  	int changed;
>  
>  	struct snd_interval __maybe_unused old_interval;
>  
> -	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) {
> +	for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL;
> +	     k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; ++k) {
>  		i = hw_param_interval(params, k);
>  		if (snd_interval_empty(i))
>  			return -EINVAL;
> @@ -326,25 +328,28 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
>  {
>  	struct snd_pcm_hw_constraints *constrs =
>  					&substream->runtime->hw_constraints;
> -	unsigned int k;
> +	int k;
>  	unsigned int rstamps[constrs->rules_num];
>  	unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1];
> -	unsigned int stamp = 2;
> -	int again;
> +	unsigned int stamp;
> +	struct snd_pcm_hw_rule *r;
> +	int d;
> +	bool again;
>  	int changed;
>  
>  	struct snd_mask __maybe_unused old_mask;
>  	struct snd_interval __maybe_unused old_interval;
>  
> -	for (k = 0; k < constrs->rules_num; k++)
> +	for (k = 0; k < constrs->rules_num; ++k)
>  		rstamps[k] = 0;
> -	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) 
> +	for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; ++k)
>  		vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0;
> +
> +	stamp = 2;
>  retry:
> -	again = 0;
> +	again = false;
>  	for (k = 0; k < constrs->rules_num; k++) {
> -		struct snd_pcm_hw_rule *r = &constrs->rules[k];
> -		unsigned int d;
> +		r = &constrs->rules[k];
>  		if (r->cond && !(r->cond & params->flags))
>  			continue;
>  		for (d = 0; r->deps[d] >= 0; d++) {
> @@ -378,7 +383,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
>  		if (changed && r->var >= 0) {
>  			params->cmask |= (1 << r->var);
>  			vstamps[r->var] = stamp;
> -			again = 1;
> +			again = true;
>  		}
>  		if (changed < 0)
>  			return changed;
> @@ -397,7 +402,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  	struct snd_pcm_hardware *hw;
>  	struct snd_interval *i = NULL;
>  	struct snd_mask *m = NULL;
> -	int changed;
>  	int err;
>  
>  	struct snd_mask __maybe_unused old_mask;
> @@ -451,16 +455,15 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
>  		if (snd_mask_min(m) == snd_mask_max(m) &&
>                      snd_interval_min(i) == snd_interval_max(i)) {
> -			changed = substream->ops->ioctl(substream,
> +			err = substream->ops->ioctl(substream,
>  					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
> -			if (changed < 0)
> -				return changed;
> +			if (err < 0)
> +				return err;
>  		}
>  	}
>  	params->rmask = 0;
>  	return 0;
>  }
> -
>  EXPORT_SYMBOL(snd_pcm_hw_refine);
>  
>  static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
> -- 
> 2.11.0
> 

Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Sakamoto]

Hi,

On Jun 8 2017 16:10, Takashi Iwai wrote:
> On Thu, 08 Jun 2017 01:10:19 +0200,
> Takashi Sakamoto wrote:
>>
>> Application of constraints to mask-type parameters for PCM substream is
>> done in a call of snd_pcm_hw_refine(), while the function includes much
>> codes and is not enough friendly to readers.
>>
>> This commit splits the codes to a separated function so that readers can
>> get it easily. I leave desicion into compilers to merge the function into
>> its callee.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
>>   sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 38 insertions(+), 19 deletions(-)
>>
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 2bde07a4a87f..b3e8aab3915e 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> ...
>> +	struct snd_mask __maybe_unused old_mask;
> 
> Do we really need __maybe_unused?  Drop it as much as possible.
> IMO, it can be uglier than ifdef, since you don't know why it's
> unused.  With ifdef, at least, you have an idea about the condition.

In kernel documentation[1], we can see below suggestion.

"20) Conditional Compilation
...
If you have a function or variable which may potentially go unused in a
particular configuration, and the compiler would warn about its 
definition going unused, mark the definition as __maybe_unused rather 
than wrapping it in a preprocessor conditional.  (However, if a function 
or variable *always* goes unused, delete it.)"

I'll follow this.


Regards

Takashi Sakamoto

Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Sakamoto]

On 2017年06月08日 16:28, Takashi Sakamoto wrote:
> Hi,
> 
> On Jun 8 2017 16:10, Takashi Iwai wrote:
>> On Thu, 08 Jun 2017 01:10:19 +0200,
>> Takashi Sakamoto wrote:
>>>
>>> Application of constraints to mask-type parameters for PCM substream is
>>> done in a call of snd_pcm_hw_refine(), while the function includes much
>>> codes and is not enough friendly to readers.
>>>
>>> This commit splits the codes to a separated function so that readers can
>>> get it easily. I leave desicion into compilers to merge the function 
>>> into
>>> its callee.
>>>
>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>> ---
>>>   sound/core/pcm_native.c | 57 
>>> ++++++++++++++++++++++++++++++++-----------------
>>>   1 file changed, 38 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>>> index 2bde07a4a87f..b3e8aab3915e 100644
>>> --- a/sound/core/pcm_native.c
>>> +++ b/sound/core/pcm_native.c
>>> ...
>>> +    struct snd_mask __maybe_unused old_mask;
>>
>> Do we really need __maybe_unused?  Drop it as much as possible.
>> IMO, it can be uglier than ifdef, since you don't know why it's
>> unused.  With ifdef, at least, you have an idea about the condition.
> 
> In kernel documentation[1], we can see below suggestion.
> 
> "20) Conditional Compilation
> ...
> If you have a function or variable which may potentially go unused in a
> particular configuration, and the compiler would warn about its 
> definition going unused, mark the definition as __maybe_unused rather 
> than wrapping it in a preprocessor conditional.  (However, if a function 
> or variable *always* goes unused, delete it.)"
> 
> I'll follow this.

Oops. This is URL for the [1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n993


Regards

Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Iwai]

On Thu, 08 Jun 2017 09:28:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Jun 8 2017 16:10, Takashi Iwai wrote:
> > On Thu, 08 Jun 2017 01:10:19 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> Application of constraints to mask-type parameters for PCM substream is
> >> done in a call of snd_pcm_hw_refine(), while the function includes much
> >> codes and is not enough friendly to readers.
> >>
> >> This commit splits the codes to a separated function so that readers can
> >> get it easily. I leave desicion into compilers to merge the function into
> >> its callee.
> >>
> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> >> ---
> >>   sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
> >>   1 file changed, 38 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> >> index 2bde07a4a87f..b3e8aab3915e 100644
> >> --- a/sound/core/pcm_native.c
> >> +++ b/sound/core/pcm_native.c
> >> ...
> >> +	struct snd_mask __maybe_unused old_mask;
> >
> > Do we really need __maybe_unused?  Drop it as much as possible.
> > IMO, it can be uglier than ifdef, since you don't know why it's
> > unused.  With ifdef, at least, you have an idea about the condition.
> 
> In kernel documentation[1], we can see below suggestion.
> 
> "20) Conditional Compilation
> ...
> If you have a function or variable which may potentially go unused in a
> particular configuration, and the compiler would warn about its
> definition going unused, mark the definition as __maybe_unused rather
> than wrapping it in a preprocessor conditional.  (However, if a
> function or variable *always* goes unused, delete it.)"
> 
> I'll follow this.

It doesn't answer my question.  Without __maybe_unused in your code,
do you get the compile warning?  If yes, why?

Well, I see the usage already in your patch for the tracepoints.
But __maybe_unused is really ugly, and should be avoided as much as
possible.

The text above doesn't recommend to use it blindly.   It's the last
resort.


thanks,

Takashi

Re: [PATCH 1/8] ALSA: pcm: add a helper function to constrain mask-type parameters

[Takashi Sakamoto]

Hi,

On Jun 8 2017 16:35, Takashi Iwai wrote:
> On Thu, 08 Jun 2017 09:28:37 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Jun 8 2017 16:10, Takashi Iwai wrote:
>>> On Thu, 08 Jun 2017 01:10:19 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> Application of constraints to mask-type parameters for PCM substream is
>>>> done in a call of snd_pcm_hw_refine(), while the function includes much
>>>> codes and is not enough friendly to readers.
>>>>
>>>> This commit splits the codes to a separated function so that readers can
>>>> get it easily. I leave desicion into compilers to merge the function into
>>>> its callee.
>>>>
>>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>>>> ---
>>>>    sound/core/pcm_native.c | 57 ++++++++++++++++++++++++++++++++-----------------
>>>>    1 file changed, 38 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>>>> index 2bde07a4a87f..b3e8aab3915e 100644
>>>> --- a/sound/core/pcm_native.c
>>>> +++ b/sound/core/pcm_native.c
>>>> ...
>>>> +	struct snd_mask __maybe_unused old_mask;
>>>
>>> Do we really need __maybe_unused?  Drop it as much as possible.
>>> IMO, it can be uglier than ifdef, since you don't know why it's
>>> unused.  With ifdef, at least, you have an idea about the condition.
>>
>> In kernel documentation[1], we can see below suggestion.
>>
>> "20) Conditional Compilation
>> ...
>> If you have a function or variable which may potentially go unused in a
>> particular configuration, and the compiler would warn about its
>> definition going unused, mark the definition as __maybe_unused rather
>> than wrapping it in a preprocessor conditional.  (However, if a
>> function or variable *always* goes unused, delete it.)"
>>
>> I'll follow this.
> 
> It doesn't answer my question.  Without __maybe_unused in your code,
> do you get the compile warning?  If yes, why?

Oh, I cannot see any warnings when disabling tracepoints or SND_DEBUG, 
as you said... I surely have a bias from my experiences for 
firewire-motu driver...

> Well, I see the usage already in your patch for the tracepoints.
> But __maybe_unused is really ugly, and should be avoided as much as
> possible.
> 
> The text above doesn't recommend to use it blindly.   It's the last
> resort.

OK. We can avoid using __maybe_unused for function local variable in 
this case. I'll drop the keyword in next v2 patchset.


Thank you

Takashi Sakamoto