[PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()

[PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()

[weiyj_lk]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Add the missing unlock before return from function special_clk_ctl_put()
in the error handling case.

Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
 sound/firewire/bebob/bebob_maudio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 6af50eb..6748515 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -382,8 +382,10 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
 	mutex_lock(&bebob->mutex);
 
 	id = uval->value.enumerated.item[0];
-	if (id >= ARRAY_SIZE(special_clk_labels))
+	if (id >= ARRAY_SIZE(special_clk_labels)) {
+		mutex_unlock(&bebob->mutex);
 		return 0;
+	}
 
 	err = avc_maudio_set_special_clk(bebob, id,
 					 params->dig_in_fmt,

Re: [PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()

[Takashi Sakamoto]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Wei,

Thanks for this patch, while I found the other issues in this file. I
would like to post new patches instead of yours, later.


Thanks

Takashi Sakamoto
o-takashi@sakamocchi.jp

(Jul 20 2014 13:50), weiyj_lk@163.com wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> Add the missing unlock before return from function
> special_clk_ctl_put() in the error handling case.
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn> --- 
> sound/firewire/bebob/bebob_maudio.c | 4 +++- 1 file changed, 3
> insertions(+), 1 deletion(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c
> b/sound/firewire/bebob/bebob_maudio.c index 6af50eb..6748515
> 100644 --- a/sound/firewire/bebob/bebob_maudio.c +++
> b/sound/firewire/bebob/bebob_maudio.c @@ -382,8 +382,10 @@ static
> int special_clk_ctl_put(struct snd_kcontrol *kctl, 
> mutex_lock(&bebob->mutex);
> 
> id = uval->value.enumerated.item[0]; -	if (id >=
> ARRAY_SIZE(special_clk_labels)) +	if (id >=
> ARRAY_SIZE(special_clk_labels)) { +		mutex_unlock(&bebob->mutex); 
> return 0; +	}
> 
> err = avc_maudio_set_special_clk(bebob, id, params->dig_in_fmt,
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTy3UIAAoJENbkvsBXhK8as9AH+wYN2lxFlzBdMhPgRigp/bkv
mw770Hpyb50TE3ILkIcGpgec1TFrK/QwQjUmunmJLQnvXPBNVNGiVaKsOhhHwmqG
7hDWp8swTSdxZQeSFWjjxAc+AntjEoUkOdiwclzT+1M1tO1vjZdRoXAos4o3G6Od
xKSl0xO4Qi+Wv6ib1p5yneOKEGZLmEZTLJY2PXXKhHQjybzYS1cRRlK9+afJYLhT
sEHPknz00OCbvFRAXIK0GMuaQzncZOFYA2Ovczei7Y+ugJuGJbvNfxhxYO6j7Zc6
j1dE79iBY0hhH32zdUn7zcWR8Zbxbpfv8oA0dkeQRisV3RB9F2P7bSKHtNj0cOw=
=Or5u
-----END PGP SIGNATURE-----

[PATCH 0/6] bebob: Improvements for M-Audio specific operations

[Takashi Sakamoto]

This series of patch improves some control elements for M-Audio specific
operations. Current implementation includes dead lock and my misunderstanding
about hardware specification and ALSA Control interface.

Takashi Sakamoto (6):
  bebob: Arrangement for critical section to be shorter
  bebob: Reducing function callers for simplicity
  bebob: Add dev_err() for debugging
  bebob: Use different labels for digital input/output interfaces
  bebob: Correction for return value of .put callback functions
  bebob: Arrangement for a control element to which two settings relate

 sound/firewire/bebob/bebob_maudio.c | 181 ++++++++++++++++++++++--------------
 1 file changed, 111 insertions(+), 70 deletions(-)

-- 
1.9.1

[PATCH 1/6] bebob: Arrangement for critical section to be shorter

[Takashi Sakamoto]

Cc: Wei Yongjun <weiyj_lk@163.com>

This commit move some mutex_lock() to shorten critical section in some
functions. This commit is my solution for this post.

[PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()
https://lkml.org/lkml/2014/7/20/12

This commit also renames a function for my naming consistency.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 6af50eb..0a33045 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -372,23 +372,24 @@ static int special_clk_ctl_get(struct snd_kcontrol *kctl,
 	uval->value.enumerated.item[0] = params->clk_src;
 	return 0;
 }
-static int special_clk_ctl_put(struct snd_kcontrol *kctl,
+static int special_clk_ctl_set(struct snd_kcontrol *kctl,
 			       struct snd_ctl_elem_value *uval)
 {
 	struct snd_bebob *bebob = snd_kcontrol_chip(kctl);
 	struct special_params *params = bebob->maudio_special_quirk;
 	int err, id;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
 	if (id >= ARRAY_SIZE(special_clk_labels))
 		return 0;
 
+	mutex_lock(&bebob->mutex);
+
 	err = avc_maudio_set_special_clk(bebob, id,
 					 params->dig_in_fmt,
 					 params->dig_out_fmt,
 					 params->clk_lock);
+
 	mutex_unlock(&bebob->mutex);
 
 	return err >= 0;
@@ -399,7 +400,7 @@ static struct snd_kcontrol_new special_clk_ctl = {
 	.access	= SNDRV_CTL_ELEM_ACCESS_READWRITE,
 	.info	= special_clk_ctl_info,
 	.get	= special_clk_ctl_get,
-	.put	= special_clk_ctl_put
+	.put	= special_clk_ctl_set
 };
 
 /* Clock synchronization control for special firmware */
@@ -491,14 +492,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 	unsigned int id, dig_in_fmt, dig_in_iface;
 	int err;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
 
 	/* decode user value */
 	dig_in_fmt = (id >> 1) & 0x01;
 	dig_in_iface = id & 0x01;
 
+	mutex_lock(&bebob->mutex);
+
 	err = avc_maudio_set_special_clk(bebob,
 					 params->clk_src,
 					 dig_in_fmt,
@@ -558,10 +559,10 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
 	unsigned int id;
 	int err;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
 
+	mutex_lock(&bebob->mutex);
+
 	err = avc_maudio_set_special_clk(bebob,
 					 params->clk_src,
 					 params->dig_in_fmt,
-- 
1.9.1

[PATCH 2/6] bebob: Reducing function callers for simplicity

[Takashi Sakamoto]

M-Audio Firewire 1814 and ProjectMix I/O change their stream formation
according to current digital format for input/output, although drivers
cannot recognize current formation by any commands. Thus the drivers
need to remember the formations. The special_stream_formation_set() is
for this purpose.

This function is expected to be called just after current digital formats
are changed. The way to change current digital format is to call
avc_maudio_set_special_clk() and there is no other ways.

For simplicity, this commit make the avc_maudio_set_special_clk() as a sole
caller for the special_stream_formation_set().

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 62 ++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 0a33045..83f8d5b 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -160,6 +160,35 @@ end:
 	return err;
 }
 
+static void
+special_stream_formation_set(struct snd_bebob *bebob)
+{
+	static const unsigned int ch_table[2][2][3] = {
+		/* AMDTP_OUT_STREAM */
+		{ {  6,  6,  4 },	/* SPDIF */
+		  { 12,  8,  4 } },	/* ADAT */
+		/* AMDTP_IN_STREAM */
+		{ { 10, 10,  2 },	/* SPDIF */
+		  { 16, 12,  2 } }	/* ADAT */
+	};
+	struct special_params *params = bebob->maudio_special_quirk;
+	unsigned int i, max;
+
+	max = SND_BEBOB_STRM_FMT_ENTRIES - 1;
+	if (!params->is1814)
+		max -= 2;
+
+	for (i = 0; i < max; i++) {
+		bebob->tx_stream_formations[i + 1].pcm =
+			ch_table[AMDTP_IN_STREAM][params->dig_in_fmt][i / 2];
+		bebob->tx_stream_formations[i + 1].midi = 1;
+
+		bebob->rx_stream_formations[i + 1].pcm =
+			ch_table[AMDTP_OUT_STREAM][params->dig_out_fmt][i / 2];
+		bebob->rx_stream_formations[i + 1].midi = 1;
+	}
+}
+
 /*
  * dig_fmt: 0x00:S/PDIF, 0x01:ADAT
  * clk_lock: 0x00:unlock, 0x01:lock
@@ -212,6 +241,8 @@ avc_maudio_set_special_clk(struct snd_bebob *bebob, unsigned int clk_src,
 	params->dig_out_fmt	= buf[8];
 	params->clk_lock	= buf[9];
 
+	special_stream_formation_set(bebob);
+
 	if (params->ctl_id_sync)
 		snd_ctl_notify(bebob->card, SNDRV_CTL_EVENT_MASK_VALUE,
 			       params->ctl_id_sync);
@@ -221,34 +252,6 @@ end:
 	kfree(buf);
 	return err;
 }
-static void
-special_stream_formation_set(struct snd_bebob *bebob)
-{
-	static const unsigned int ch_table[2][2][3] = {
-		/* AMDTP_OUT_STREAM */
-		{ {  6,  6,  4 },	/* SPDIF */
-		  { 12,  8,  4 } },	/* ADAT */
-		/* AMDTP_IN_STREAM */
-		{ { 10, 10,  2 },	/* SPDIF */
-		  { 16, 12,  2 } }	/* ADAT */
-	};
-	struct special_params *params = bebob->maudio_special_quirk;
-	unsigned int i, max;
-
-	max = SND_BEBOB_STRM_FMT_ENTRIES - 1;
-	if (!params->is1814)
-		max -= 2;
-
-	for (i = 0; i < max; i++) {
-		bebob->tx_stream_formations[i + 1].pcm =
-			ch_table[AMDTP_IN_STREAM][params->dig_in_fmt][i / 2];
-		bebob->tx_stream_formations[i + 1].midi = 1;
-
-		bebob->rx_stream_formations[i + 1].pcm =
-			ch_table[AMDTP_OUT_STREAM][params->dig_out_fmt][i / 2];
-		bebob->rx_stream_formations[i + 1].midi = 1;
-	}
-}
 
 static int add_special_controls(struct snd_bebob *bebob);
 int
@@ -280,8 +283,6 @@ snd_bebob_maudio_special_discover(struct snd_bebob *bebob, bool is1814)
 	if (err < 0)
 		goto end;
 
-	special_stream_formation_set(bebob);
-
 	if (params->is1814) {
 		bebob->midi_input_ports = 1;
 		bebob->midi_output_ports = 1;
@@ -513,7 +514,6 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 		dev_err(&bebob->unit->device,
 			"fail to set digital input interface: %d\n", err);
 end:
-	special_stream_formation_set(bebob);
 	mutex_unlock(&bebob->mutex);
 	return err;
 }
-- 
1.9.1

[PATCH 3/6] bebob: Add dev_err() for debugging

[Takashi Sakamoto]

This commit adds some dev_err() to help debug when
avc_maudio_set_special_clk() failed.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 83f8d5b..008ff2c 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -393,6 +393,10 @@ static int special_clk_ctl_set(struct snd_kcontrol *kctl,
 
 	mutex_unlock(&bebob->mutex);
 
+	if (err < 0)
+		dev_err(&bebob->unit->device,
+			"fail to change clock source: %d\n", err);
+
 	return err >= 0;
 }
 static struct snd_kcontrol_new special_clk_ctl = {
@@ -506,7 +510,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 					 dig_in_fmt,
 					 params->dig_out_fmt,
 					 params->clk_lock);
-	if ((err < 0) || (params->dig_in_fmt > 0)) /* ADAT */
+	if (err < 0) {
+		dev_err(&bebob->unit->device,
+			"fail to change clock source: %d\n", err);
+		goto end;
+	}
+
+	/* For ADAT, optical interface is only available. */
+	if (params->dig_in_fmt > 0)
 		goto end;
 
 	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
@@ -567,10 +578,13 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
 					 params->clk_src,
 					 params->dig_in_fmt,
 					 id, params->clk_lock);
-	if (err >= 0)
-		special_stream_formation_set(bebob);
 
 	mutex_unlock(&bebob->mutex);
+
+	if (err < 0)
+		dev_err(&bebob->unit->device,
+			"fail to change digital output interface: %d\n", err);
+
 	return err;
 }
 static struct snd_kcontrol_new special_dig_out_iface_ctl = {
-- 
1.9.1

[PATCH 4/6] bebob: Use different labels for digital input/output interfaces

[Takashi Sakamoto]

This commit use different labels for control elements of digital input/output
interfaces to correct my misunderstanding about M-Audio Firewire 1814 and
ProjectMix I/O.

According to user manuals for these two models, they have two modes for
digital input; one is S/PDIF in both of optical and coaxial interfaces,
another is ADAT in optical interface only.

But in current implementation, a control element for digital input uses
reduced labels which a control element for digital output uses because of my
misunderstanding that optical interface is not available for digital input
with S/PDIF mode.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 008ff2c..f2277ce 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -440,8 +440,8 @@ static struct snd_kcontrol_new special_sync_ctl = {
 	.get	= special_sync_ctl_get,
 };
 
-/* Digital interface control for special firmware */
-static char *const special_dig_iface_labels[] = {
+/* Digital input interface control for special firmware */
+static char *const special_dig_in_iface_labels[] = {
 	"S/PDIF Optical", "S/PDIF Coaxial", "ADAT Optical"
 };
 static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
@@ -449,13 +449,13 @@ static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
 {
 	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	einf->count = 1;
-	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels);
+	einf->value.enumerated.items = ARRAY_SIZE(special_dig_in_iface_labels);
 
 	if (einf->value.enumerated.item >= einf->value.enumerated.items)
 		einf->value.enumerated.item = einf->value.enumerated.items - 1;
 
 	strcpy(einf->value.enumerated.name,
-	       special_dig_iface_labels[einf->value.enumerated.item]);
+	       special_dig_in_iface_labels[einf->value.enumerated.item]);
 
 	return 0;
 }
@@ -498,6 +498,8 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 	int err;
 
 	id = uval->value.enumerated.item[0];
+	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
+		return 0;
 
 	/* decode user value */
 	dig_in_fmt = (id >> 1) & 0x01;
@@ -537,18 +539,22 @@ static struct snd_kcontrol_new special_dig_in_iface_ctl = {
 	.put	= special_dig_in_iface_ctl_set
 };
 
+/* Digital output interface control for special firmware */
+static char *const special_dig_out_iface_labels[] = {
+	"S/PDIF Optical and Coaxial", "ADAT Optical"
+};
 static int special_dig_out_iface_ctl_info(struct snd_kcontrol *kctl,
 					  struct snd_ctl_elem_info *einf)
 {
 	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	einf->count = 1;
-	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels) - 1;
+	einf->value.enumerated.items = ARRAY_SIZE(special_dig_out_iface_labels);
 
 	if (einf->value.enumerated.item >= einf->value.enumerated.items)
 		einf->value.enumerated.item = einf->value.enumerated.items - 1;
 
 	strcpy(einf->value.enumerated.name,
-	       special_dig_iface_labels[einf->value.enumerated.item + 1]);
+	       special_dig_out_iface_labels[einf->value.enumerated.item]);
 
 	return 0;
 }
@@ -571,6 +577,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
 	int err;
 
 	id = uval->value.enumerated.item[0];
+	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
+		return 0;
 
 	mutex_lock(&bebob->mutex);
 
-- 
1.9.1

[PATCH 5/6] bebob: Correction for return value of .put callback functions

[Takashi Sakamoto]

This commit is for correction of my misunderstanding for return value of
.put callback of ALSA Control interface.

According to 'Writing ALSA Driver' (*1), return value of the callback has
three patterns; 1: changed, 0: not changed, an negative value: fatal error.

But I misunderstood that it's boolean; zero or nonzero.

*1: Writing an ALSA Driver (2005, Takashi Iwai)
http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index f2277ce..acfe94a 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -396,8 +396,10 @@ static int special_clk_ctl_set(struct snd_kcontrol *kctl,
 	if (err < 0)
 		dev_err(&bebob->unit->device,
 			"fail to change clock source: %d\n", err);
+	else
+		err = 1;
 
-	return err >= 0;
+	return err;
 }
 static struct snd_kcontrol_new special_clk_ctl = {
 	.name	= "Clock Source",
@@ -519,13 +521,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 	}
 
 	/* For ADAT, optical interface is only available. */
-	if (params->dig_in_fmt > 0)
+	if (params->dig_in_fmt > 0) {
+		err = 1;
 		goto end;
+	}
 
 	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
 	if (err < 0)
 		dev_err(&bebob->unit->device,
 			"fail to set digital input interface: %d\n", err);
+	else
+		err = 1;
 end:
 	mutex_unlock(&bebob->mutex);
 	return err;
@@ -592,6 +598,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
 	if (err < 0)
 		dev_err(&bebob->unit->device,
 			"fail to change digital output interface: %d\n", err);
+	else
+		err = 1;
 
 	return err;
 }
-- 
1.9.1

[PATCH 6/6] bebob: Arrangement for a control element to which two settings relate

[Takashi Sakamoto]

A control element for digital input interface includes two settings; one
is for digital format (S/PDIF or ADAT), another is for interface (optical or
coaxial).

The setting for interface is meaningful just for S/PDIF, therefore to consider
recovery at failure of a operation for the second setting, a operation for
the first setting should be for interface, not digital format.

Additionally, this commit changes special_dig_in_iface_ctl_get() to reduce
needless processing for the interface when the digital format is ADAT.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 70 +++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index acfe94a..7899b56 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -471,21 +471,24 @@ static int special_dig_in_iface_ctl_get(struct snd_kcontrol *kctl,
 
 	mutex_lock(&bebob->mutex);
 
-	err = avc_audio_get_selector(bebob->unit, 0x00, 0x04,
-				     &dig_in_iface);
-	if (err < 0) {
-		dev_err(&bebob->unit->device,
-			"fail to get digital input interface: %d\n", err);
-		goto end;
+	/* For ADAT, optical interface is only available. */
+	if (params->dig_in_fmt > 0) {
+		err = 0;
+		dig_in_iface = 0;
+	} else {
+		err = avc_audio_get_selector(bebob->unit, 0x00, 0x04,
+					     &dig_in_iface);
+		if (err < 0) {
+			dev_err(&bebob->unit->device,
+				"fail to get digital input interface: %d\n",
+				err);
+			goto end;
+		}
 	}
 
 	/* encoded id for user value */
 	val = (params->dig_in_fmt << 1) | (dig_in_iface & 0x01);
 
-	/* for ADAT Optical */
-	if (val > 2)
-		val = 2;
-
 	uval->value.enumerated.item[0] = val;
 end:
 	mutex_unlock(&bebob->mutex);
@@ -497,7 +500,7 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 	struct snd_bebob *bebob = snd_kcontrol_chip(kctl);
 	struct special_params *params = bebob->maudio_special_quirk;
 	unsigned int id, dig_in_fmt, dig_in_iface;
-	int err;
+	int err = 0;
 
 	id = uval->value.enumerated.item[0];
 	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
@@ -509,29 +512,36 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 
 	mutex_lock(&bebob->mutex);
 
-	err = avc_maudio_set_special_clk(bebob,
-					 params->clk_src,
-					 dig_in_fmt,
-					 params->dig_out_fmt,
-					 params->clk_lock);
-	if (err < 0) {
-		dev_err(&bebob->unit->device,
-			"fail to change clock source: %d\n", err);
-		goto end;
-	}
-
-	/* For ADAT, optical interface is only available. */
-	if (params->dig_in_fmt > 0) {
+	/* For S/PDIF, optical/coaxial interfaces are selectable. */
+	if (dig_in_fmt == 0) {
+		if (amdtp_stream_running(&bebob->rx_stream))
+			goto end;
+
+		err = avc_audio_set_selector(bebob->unit, 0x00, 0x04,
+					     dig_in_iface);
+		if (err < 0) {
+			dev_err(&bebob->unit->device,
+				"fail to change digital input interface: %d\n",
+				err);
+			goto end;
+		}
 		err = 1;
-		goto end;
 	}
 
-	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
-	if (err < 0)
-		dev_err(&bebob->unit->device,
-			"fail to set digital input interface: %d\n", err);
-	else
+	if (params->dig_in_fmt != dig_in_fmt) {
+		err = avc_maudio_set_special_clk(bebob,
+						 params->clk_src,
+						 dig_in_fmt,
+						 params->dig_out_fmt,
+						 params->clk_lock);
+		if (err < 0) {
+			dev_err(&bebob->unit->device,
+				"fail to change digital input format: %d\n",
+				err);
+			goto end;
+		}
 		err = 1;
+	}
 end:
 	mutex_unlock(&bebob->mutex);
 	return err;
-- 
1.9.1

Re: [PATCH 0/6] bebob: Improvements for M-Audio specific operations

[Takashi Iwai]

At Mon, 21 Jul 2014 11:09:59 +0900,
Takashi Sakamoto wrote:
> 
> This series of patch improves some control elements for M-Audio specific
> operations. Current implementation includes dead lock and my misunderstanding
> about hardware specification and ALSA Control interface.

I don't want to apply such a big series at the late stage of 3.16-rc.
Please create short patches "just doing fix" for 3.16 as Yongjun did,
and rebase comprehensive cleanups on it.


thanks,

Takashi

> 
> Takashi Sakamoto (6):
>   bebob: Arrangement for critical section to be shorter
>   bebob: Reducing function callers for simplicity
>   bebob: Add dev_err() for debugging
>   bebob: Use different labels for digital input/output interfaces
>   bebob: Correction for return value of .put callback functions
>   bebob: Arrangement for a control element to which two settings relate
> 
>  sound/firewire/bebob/bebob_maudio.c | 181 ++++++++++++++++++++++--------------
>  1 file changed, 111 insertions(+), 70 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH 0/6] bebob: Improvements for M-Audio specific operations

[Takashi Sakamoto]

Hi Iwai-San,

(Jul 21 2014 16:06), Takashi Iwai wrote:
> I don't want to apply such a big series at the late stage of 3.16-rc.
> Please create short patches "just doing fix" for 3.16 as Yongjun did,
> and rebase comprehensive cleanups on it.

Exactly. In this time, I want to fix three bugs:
1. Missing unlock which Yongjun reported
2. My misunderstanding about return value of .put callback
3. Wrong label for a control element of digital input interface

I think these should be fixed in this release cycle.

Additionally, I want to add some dev_err() to reduce my maintaining
efforts, because the fireworks/bebob drivers newly supports 60-70 models
in Linux 3.16, while the number of developers who can support users is a
few, maybe I and Clemens.

I'll post four patches for these items, instead of the six patches, and
post additional patches for next cycle, 3.17. Is it OK?


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

Re: [PATCH 4/6] bebob: Use different labels for digital input/output interfaces

[Takashi Iwai]

At Mon, 21 Jul 2014 11:10:03 +0900,
Takashi Sakamoto wrote:
> 
> This commit use different labels for control elements of digital input/output
> interfaces to correct my misunderstanding about M-Audio Firewire 1814 and
> ProjectMix I/O.
> 
> According to user manuals for these two models, they have two modes for
> digital input; one is S/PDIF in both of optical and coaxial interfaces,
> another is ADAT in optical interface only.
> 
> But in current implementation, a control element for digital input uses
> reduced labels which a control element for digital output uses because of my
> misunderstanding that optical interface is not available for digital input
> with S/PDIF mode.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/bebob/bebob_maudio.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> index 008ff2c..f2277ce 100644
> --- a/sound/firewire/bebob/bebob_maudio.c
> +++ b/sound/firewire/bebob/bebob_maudio.c
> @@ -440,8 +440,8 @@ static struct snd_kcontrol_new special_sync_ctl = {
>  	.get	= special_sync_ctl_get,
>  };
>  
> -/* Digital interface control for special firmware */
> -static char *const special_dig_iface_labels[] = {
> +/* Digital input interface control for special firmware */
> +static char *const special_dig_in_iface_labels[] = {
>  	"S/PDIF Optical", "S/PDIF Coaxial", "ADAT Optical"
>  };
>  static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
> @@ -449,13 +449,13 @@ static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
>  {
>  	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
>  	einf->count = 1;
> -	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels);
> +	einf->value.enumerated.items = ARRAY_SIZE(special_dig_in_iface_labels);
>  
>  	if (einf->value.enumerated.item >= einf->value.enumerated.items)
>  		einf->value.enumerated.item = einf->value.enumerated.items - 1;
>  
>  	strcpy(einf->value.enumerated.name,
> -	       special_dig_iface_labels[einf->value.enumerated.item]);
> +	       special_dig_in_iface_labels[einf->value.enumerated.item]);
>  
>  	return 0;
>  }
> @@ -498,6 +498,8 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
>  	int err;
>  
>  	id = uval->value.enumerated.item[0];
> +	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
> +		return 0;

This should return an error.

>  
>  	/* decode user value */
>  	dig_in_fmt = (id >> 1) & 0x01;
> @@ -537,18 +539,22 @@ static struct snd_kcontrol_new special_dig_in_iface_ctl = {
>  	.put	= special_dig_in_iface_ctl_set
>  };
>  
> +/* Digital output interface control for special firmware */
> +static char *const special_dig_out_iface_labels[] = {
> +	"S/PDIF Optical and Coaxial", "ADAT Optical"
> +};
>  static int special_dig_out_iface_ctl_info(struct snd_kcontrol *kctl,
>  					  struct snd_ctl_elem_info *einf)
>  {
>  	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
>  	einf->count = 1;
> -	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels) - 1;
> +	einf->value.enumerated.items = ARRAY_SIZE(special_dig_out_iface_labels);
>  
>  	if (einf->value.enumerated.item >= einf->value.enumerated.items)
>  		einf->value.enumerated.item = einf->value.enumerated.items - 1;
>  
>  	strcpy(einf->value.enumerated.name,
> -	       special_dig_iface_labels[einf->value.enumerated.item + 1]);
> +	       special_dig_out_iface_labels[einf->value.enumerated.item]);
>  
>  	return 0;
>  }
> @@ -571,6 +577,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
>  	int err;
>  
>  	id = uval->value.enumerated.item[0];
> +	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
> +		return 0;

Ditto.


thanks,

Takashi


>  
>  	mutex_lock(&bebob->mutex);
>  
> -- 
> 1.9.1
> 

Re: [PATCH 0/6] bebob: Improvements for M-Audio specific operations

[Takashi Iwai]

At Mon, 21 Jul 2014 18:50:23 +0900,
Takashi Sakamoto wrote:
> 
> Hi Iwai-San,
> 
> (Jul 21 2014 16:06), Takashi Iwai wrote:
> > I don't want to apply such a big series at the late stage of 3.16-rc.
> > Please create short patches "just doing fix" for 3.16 as Yongjun did,
> > and rebase comprehensive cleanups on it.
> 
> Exactly. In this time, I want to fix three bugs:
> 1. Missing unlock which Yongjun reported
> 2. My misunderstanding about return value of .put callback
> 3. Wrong label for a control element of digital input interface
> 
> I think these should be fixed in this release cycle.
> 
> Additionally, I want to add some dev_err() to reduce my maintaining
> efforts, because the fireworks/bebob drivers newly supports 60-70 models
> in Linux 3.16, while the number of developers who can support users is a
> few, maybe I and Clemens.
> 
> I'll post four patches for these items, instead of the six patches, and
> post additional patches for next cycle, 3.17. Is it OK?

Yes.


Takashi

Re: [PATCH 3/6] bebob: Add dev_err() for debugging

[Takashi Iwai]

At Mon, 21 Jul 2014 11:10:02 +0900,
Takashi Sakamoto wrote:
> 
> This commit adds some dev_err() to help debug when
> avc_maudio_set_special_clk() failed.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/bebob/bebob_maudio.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> index 83f8d5b..008ff2c 100644
> --- a/sound/firewire/bebob/bebob_maudio.c
> +++ b/sound/firewire/bebob/bebob_maudio.c
> @@ -393,6 +393,10 @@ static int special_clk_ctl_set(struct snd_kcontrol *kctl,
>  
>  	mutex_unlock(&bebob->mutex);
>  
> +	if (err < 0)
> +		dev_err(&bebob->unit->device,
> +			"fail to change clock source: %d\n", err);
> +

The error message at the end of put callback is often useless.
If it's an error, it's notified to user-space and you'll see the error
there.  The unconditional error message via dev_err() may flood the
kernel messages, too.

For the debugging purpose, rather put dev_dbg() to each point
receiving an error and give the individual message.


thanks,

Takashi

>  	return err >= 0;
>  }
>  static struct snd_kcontrol_new special_clk_ctl = {
> @@ -506,7 +510,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
>  					 dig_in_fmt,
>  					 params->dig_out_fmt,
>  					 params->clk_lock);
> -	if ((err < 0) || (params->dig_in_fmt > 0)) /* ADAT */
> +	if (err < 0) {
> +		dev_err(&bebob->unit->device,
> +			"fail to change clock source: %d\n", err);
> +		goto end;
> +	}
> +
> +	/* For ADAT, optical interface is only available. */
> +	if (params->dig_in_fmt > 0)
>  		goto end;
>  
>  	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
> @@ -567,10 +578,13 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
>  					 params->clk_src,
>  					 params->dig_in_fmt,
>  					 id, params->clk_lock);
> -	if (err >= 0)
> -		special_stream_formation_set(bebob);
>  
>  	mutex_unlock(&bebob->mutex);
> +
> +	if (err < 0)
> +		dev_err(&bebob->unit->device,
> +			"fail to change digital output interface: %d\n", err);
> +
>  	return err;
>  }
>  static struct snd_kcontrol_new special_dig_out_iface_ctl = {
> -- 
> 1.9.1
> 

Re: [PATCH 4/6] bebob: Use different labels for digital input/output interfaces

[Takashi Sakamoto]

(Jul 21 2014 18:58), Takashi Iwai wrote:
>> @@ -498,6 +498,8 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
>>  	int err;
>>  
>>  	id = uval->value.enumerated.item[0];
>> +	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
>> +		return 0;
> 
> This should return an error.

>> @@ -571,6 +577,8 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
>>  	int err;
>>  
>>  	id = uval->value.enumerated.item[0];
>> +	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
>> +		return 0;
> 
> Ditto.

Oops, exactly. I use -EINVAL for the error code. Thank you.


Takashi Sakamoto
o-takashi@sakamocchi.jp

[PATCH 1/3] bebob: Fix a missing to unlock mutex in error handling case

[Takashi Sakamoto]

In error handling case, special_clk_ctl_put() returns without unlock_mutex(),
therefore the mutex is still locked. This commit moves mutex_lock() after
the error handling case.

This commit is my solution for this post.

[PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()
https://lkml.org/lkml/2014/7/20/12

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 6af50eb..fc470c6 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -379,12 +379,12 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
 	struct special_params *params = bebob->maudio_special_quirk;
 	int err, id;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
 	if (id >= ARRAY_SIZE(special_clk_labels))
 		return 0;
 
+	mutex_lock(&bebob->mutex);
+
 	err = avc_maudio_set_special_clk(bebob, id,
 					 params->dig_in_fmt,
 					 params->dig_out_fmt,
-- 
1.9.1

[PATCH 2/3] bebob: Use different labels for digital input/output

[Takashi Sakamoto]

This commit uses different labels for control elements of digital input/output
interfaces to correct my misunderstanding about M-Audio Firewire 1814 and
ProjectMix I/O.

According to user manuals for these two models, they have two modes for
digital input; one is S/PDIF in both of optical and coaxial interfaces,
another is ADAT in optical interface only.

But in current implementation, a control element for it reduced labels which
a control element for digital output uses because of my misunderstanding
that optical interface is not available for digital input with S/PDIF mode.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index fc470c6..42e6f22 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -434,8 +434,8 @@ static struct snd_kcontrol_new special_sync_ctl = {
 	.get	= special_sync_ctl_get,
 };
 
-/* Digital interface control for special firmware */
-static char *const special_dig_iface_labels[] = {
+/* Digital input interface control for special firmware */
+static char *const special_dig_in_iface_labels[] = {
 	"S/PDIF Optical", "S/PDIF Coaxial", "ADAT Optical"
 };
 static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
@@ -443,13 +443,13 @@ static int special_dig_in_iface_ctl_info(struct snd_kcontrol *kctl,
 {
 	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	einf->count = 1;
-	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels);
+	einf->value.enumerated.items = ARRAY_SIZE(special_dig_in_iface_labels);
 
 	if (einf->value.enumerated.item >= einf->value.enumerated.items)
 		einf->value.enumerated.item = einf->value.enumerated.items - 1;
 
 	strcpy(einf->value.enumerated.name,
-	       special_dig_iface_labels[einf->value.enumerated.item]);
+	       special_dig_in_iface_labels[einf->value.enumerated.item]);
 
 	return 0;
 }
@@ -504,9 +504,14 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 					 dig_in_fmt,
 					 params->dig_out_fmt,
 					 params->clk_lock);
-	if ((err < 0) || (params->dig_in_fmt > 0)) /* ADAT */
+	if (err < 0)
+		goto end;
+
+	/* For ADAT, optical interface is only available. */
+	if (params->dig_in_fmt > 0)
 		goto end;
 
+	/* For S/PDIF, optical/coaxial interfaces are selectable. */
 	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
 	if (err < 0)
 		dev_err(&bebob->unit->device,
@@ -525,18 +530,22 @@ static struct snd_kcontrol_new special_dig_in_iface_ctl = {
 	.put	= special_dig_in_iface_ctl_set
 };
 
+/* Digital output interface control for special firmware */
+static char *const special_dig_out_iface_labels[] = {
+	"S/PDIF Optical and Coaxial", "ADAT Optical"
+};
 static int special_dig_out_iface_ctl_info(struct snd_kcontrol *kctl,
 					  struct snd_ctl_elem_info *einf)
 {
 	einf->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
 	einf->count = 1;
-	einf->value.enumerated.items = ARRAY_SIZE(special_dig_iface_labels) - 1;
+	einf->value.enumerated.items = ARRAY_SIZE(special_dig_out_iface_labels);
 
 	if (einf->value.enumerated.item >= einf->value.enumerated.items)
 		einf->value.enumerated.item = einf->value.enumerated.items - 1;
 
 	strcpy(einf->value.enumerated.name,
-	       special_dig_iface_labels[einf->value.enumerated.item + 1]);
+	       special_dig_out_iface_labels[einf->value.enumerated.item]);
 
 	return 0;
 }
-- 
1.9.1

[PATCH 3/3] bebob: Correction for return value of .put callback

[Takashi Sakamoto]

This commit is for correction of my misunderstanding about return value of
.put callback in ALSA Control interface.

According to 'Writing ALSA Driver' (*1), return value of the callback has
three patterns; 1: changed, 0: not changed, an negative value: fatal error.

But I misunderstood that it's boolean; zero or nonzero.

*1: Writing an ALSA Driver (2005, Takashi Iwai)
http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index 42e6f22..d6d6ff8 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -391,7 +391,10 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
 					 params->clk_lock);
 	mutex_unlock(&bebob->mutex);
 
-	return err >= 0;
+	if (err >= 0)
+		err = 1;
+
+	return err;
 }
 static struct snd_kcontrol_new special_clk_ctl = {
 	.name	= "Clock Source",
@@ -491,14 +494,16 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 	unsigned int id, dig_in_fmt, dig_in_iface;
 	int err;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
+	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
+		return -EINVAL;
 
 	/* decode user value */
 	dig_in_fmt = (id >> 1) & 0x01;
 	dig_in_iface = id & 0x01;
 
+	mutex_lock(&bebob->mutex);
+
 	err = avc_maudio_set_special_clk(bebob,
 					 params->clk_src,
 					 dig_in_fmt,
@@ -508,14 +513,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
 		goto end;
 
 	/* For ADAT, optical interface is only available. */
-	if (params->dig_in_fmt > 0)
+	if (params->dig_in_fmt > 0) {
+		err = 1;
 		goto end;
+	}
 
 	/* For S/PDIF, optical/coaxial interfaces are selectable. */
 	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
 	if (err < 0)
 		dev_err(&bebob->unit->device,
 			"fail to set digital input interface: %d\n", err);
+	err = 1;
 end:
 	special_stream_formation_set(bebob);
 	mutex_unlock(&bebob->mutex);
@@ -567,16 +575,20 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
 	unsigned int id;
 	int err;
 
-	mutex_lock(&bebob->mutex);
-
 	id = uval->value.enumerated.item[0];
+	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
+		return -EINVAL;
+
+	mutex_lock(&bebob->mutex);
 
 	err = avc_maudio_set_special_clk(bebob,
 					 params->clk_src,
 					 params->dig_in_fmt,
 					 id, params->clk_lock);
-	if (err >= 0)
+	if (err >= 0) {
 		special_stream_formation_set(bebob);
+		err = 1;
+	}
 
 	mutex_unlock(&bebob->mutex);
 	return err;
-- 
1.9.1

Re: [PATCH 1/3] bebob: Fix a missing to unlock mutex in error handling case

[Takashi Iwai]

At Tue, 22 Jul 2014 23:11:03 +0900,
Takashi Sakamoto wrote:
> 
> In error handling case, special_clk_ctl_put() returns without unlock_mutex(),
> therefore the mutex is still locked. This commit moves mutex_lock() after
> the error handling case.
> 
> This commit is my solution for this post.
> 
> [PATCH -next] ALSA: bebob: Fix missing unlock on error in special_clk_ctl_put()
> https://lkml.org/lkml/2014/7/20/12
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Thanks, applied all three patches now.

BTW, at the next time, put "v2" or such in the subject line so that
people can distinguish the new patch series from the previous ones.
You can use --subject-prefix option for git-format-patch or else.

Preferably, write the changes since the previous revision, too
(usually below the --- line, so that it's not merged into the git
 changelog; some people like to have them in the final commit,
 though.)

Last but not least, don't forget to put a proper subject prefix.
For ALSA codes, we put "ALSA:" prefix in the subject.  Study git
changelogs.


Takashi

> ---
>  sound/firewire/bebob/bebob_maudio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> index 6af50eb..fc470c6 100644
> --- a/sound/firewire/bebob/bebob_maudio.c
> +++ b/sound/firewire/bebob/bebob_maudio.c
> @@ -379,12 +379,12 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
>  	struct special_params *params = bebob->maudio_special_quirk;
>  	int err, id;
>  
> -	mutex_lock(&bebob->mutex);
> -
>  	id = uval->value.enumerated.item[0];
>  	if (id >= ARRAY_SIZE(special_clk_labels))
>  		return 0;
>  
> +	mutex_lock(&bebob->mutex);
> +
>  	err = avc_maudio_set_special_clk(bebob, id,
>  					 params->dig_in_fmt,
>  					 params->dig_out_fmt,
> -- 
> 1.9.1
> 

Re: [PATCH 3/3] bebob: Correction for return value of .put callback

[Takashi Sakamoto]

Oops. I miss a line for this patch. Please drop this patch and apply
another one which I will send now.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

(Jul 22 2014 23:13), Takashi Sakamoto wrote:
> This commit is for correction of my misunderstanding about return value of
> .put callback in ALSA Control interface.
> 
> According to 'Writing ALSA Driver' (*1), return value of the callback has
> three patterns; 1: changed, 0: not changed, an negative value: fatal error.
> 
> But I misunderstood that it's boolean; zero or nonzero.
> 
> *1: Writing an ALSA Driver (2005, Takashi Iwai)
> http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  sound/firewire/bebob/bebob_maudio.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> index 42e6f22..d6d6ff8 100644
> --- a/sound/firewire/bebob/bebob_maudio.c
> +++ b/sound/firewire/bebob/bebob_maudio.c
> @@ -391,7 +391,10 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
>  					 params->clk_lock);
>  	mutex_unlock(&bebob->mutex);
>  
> -	return err >= 0;
> +	if (err >= 0)
> +		err = 1;
> +
> +	return err;
>  }
>  static struct snd_kcontrol_new special_clk_ctl = {
>  	.name	= "Clock Source",
> @@ -491,14 +494,16 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
>  	unsigned int id, dig_in_fmt, dig_in_iface;
>  	int err;
>  
> -	mutex_lock(&bebob->mutex);
> -
>  	id = uval->value.enumerated.item[0];
> +	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
> +		return -EINVAL;
>  
>  	/* decode user value */
>  	dig_in_fmt = (id >> 1) & 0x01;
>  	dig_in_iface = id & 0x01;
>  
> +	mutex_lock(&bebob->mutex);
> +
>  	err = avc_maudio_set_special_clk(bebob,
>  					 params->clk_src,
>  					 dig_in_fmt,
> @@ -508,14 +513,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
>  		goto end;
>  
>  	/* For ADAT, optical interface is only available. */
> -	if (params->dig_in_fmt > 0)
> +	if (params->dig_in_fmt > 0) {
> +		err = 1;
>  		goto end;
> +	}
>  
>  	/* For S/PDIF, optical/coaxial interfaces are selectable. */
>  	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
>  	if (err < 0)
>  		dev_err(&bebob->unit->device,
>  			"fail to set digital input interface: %d\n", err);
> +	err = 1;
>  end:
>  	special_stream_formation_set(bebob);
>  	mutex_unlock(&bebob->mutex);
> @@ -567,16 +575,20 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
>  	unsigned int id;
>  	int err;
>  
> -	mutex_lock(&bebob->mutex);
> -
>  	id = uval->value.enumerated.item[0];
> +	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
> +		return -EINVAL;
> +
> +	mutex_lock(&bebob->mutex);
>  
>  	err = avc_maudio_set_special_clk(bebob,
>  					 params->clk_src,
>  					 params->dig_in_fmt,
>  					 id, params->clk_lock);
> -	if (err >= 0)
> +	if (err >= 0) {
>  		special_stream_formation_set(bebob);
> +		err = 1;
> +	}
>  
>  	mutex_unlock(&bebob->mutex);
>  	return err;
> 

Re: [PATCH 3/3] bebob: Correction for return value of .put callback

[Takashi Iwai]

At Tue, 22 Jul 2014 23:27:04 +0900,
Takashi Sakamoto wrote:
> 
> Oops. I miss a line for this patch. Please drop this patch and apply
> another one which I will send now.

Too late.  Submit the incremental fix patch instead, please.


thanks,

Takashi

> 
> 
> Regards
> 
> Takashi Sakamoto
> o-takashi@sakamocchi.jp
> 
> (Jul 22 2014 23:13), Takashi Sakamoto wrote:
> > This commit is for correction of my misunderstanding about return value of
> > .put callback in ALSA Control interface.
> > 
> > According to 'Writing ALSA Driver' (*1), return value of the callback has
> > three patterns; 1: changed, 0: not changed, an negative value: fatal error.
> > 
> > But I misunderstood that it's boolean; zero or nonzero.
> > 
> > *1: Writing an ALSA Driver (2005, Takashi Iwai)
> > http://www.alsa-project.org/main/index.php/ALSA_Driver_Documentation
> > 
> > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > ---
> >  sound/firewire/bebob/bebob_maudio.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> > index 42e6f22..d6d6ff8 100644
> > --- a/sound/firewire/bebob/bebob_maudio.c
> > +++ b/sound/firewire/bebob/bebob_maudio.c
> > @@ -391,7 +391,10 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
> >  					 params->clk_lock);
> >  	mutex_unlock(&bebob->mutex);
> >  
> > -	return err >= 0;
> > +	if (err >= 0)
> > +		err = 1;
> > +
> > +	return err;
> >  }
> >  static struct snd_kcontrol_new special_clk_ctl = {
> >  	.name	= "Clock Source",
> > @@ -491,14 +494,16 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
> >  	unsigned int id, dig_in_fmt, dig_in_iface;
> >  	int err;
> >  
> > -	mutex_lock(&bebob->mutex);
> > -
> >  	id = uval->value.enumerated.item[0];
> > +	if (id >= ARRAY_SIZE(special_dig_in_iface_labels))
> > +		return -EINVAL;
> >  
> >  	/* decode user value */
> >  	dig_in_fmt = (id >> 1) & 0x01;
> >  	dig_in_iface = id & 0x01;
> >  
> > +	mutex_lock(&bebob->mutex);
> > +
> >  	err = avc_maudio_set_special_clk(bebob,
> >  					 params->clk_src,
> >  					 dig_in_fmt,
> > @@ -508,14 +513,17 @@ static int special_dig_in_iface_ctl_set(struct snd_kcontrol *kctl,
> >  		goto end;
> >  
> >  	/* For ADAT, optical interface is only available. */
> > -	if (params->dig_in_fmt > 0)
> > +	if (params->dig_in_fmt > 0) {
> > +		err = 1;
> >  		goto end;
> > +	}
> >  
> >  	/* For S/PDIF, optical/coaxial interfaces are selectable. */
> >  	err = avc_audio_set_selector(bebob->unit, 0x00, 0x04, dig_in_iface);
> >  	if (err < 0)
> >  		dev_err(&bebob->unit->device,
> >  			"fail to set digital input interface: %d\n", err);
> > +	err = 1;
> >  end:
> >  	special_stream_formation_set(bebob);
> >  	mutex_unlock(&bebob->mutex);
> > @@ -567,16 +575,20 @@ static int special_dig_out_iface_ctl_set(struct snd_kcontrol *kctl,
> >  	unsigned int id;
> >  	int err;
> >  
> > -	mutex_lock(&bebob->mutex);
> > -
> >  	id = uval->value.enumerated.item[0];
> > +	if (id >= ARRAY_SIZE(special_dig_out_iface_labels))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&bebob->mutex);
> >  
> >  	err = avc_maudio_set_special_clk(bebob,
> >  					 params->clk_src,
> >  					 params->dig_in_fmt,
> >  					 id, params->clk_lock);
> > -	if (err >= 0)
> > +	if (err >= 0) {
> >  		special_stream_formation_set(bebob);
> > +		err = 1;
> > +	}
> >  
> >  	mutex_unlock(&bebob->mutex);
> >  	return err;
> > 
> 

[PATCH] ALSA: bebob: Correction for return value of special_clk_ctl_put() in error

[Takashi Sakamoto]

This commit is a supplement to my previous patch.
http://mailman.alsa-project.org/pipermail/alsa-devel/2014-July/079190.html

The special_clk_ctl_put() still returns 0 in error handling case. It should
return -EINVAL.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/bebob/bebob_maudio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
index d6d6ff8..70faa3a 100644
--- a/sound/firewire/bebob/bebob_maudio.c
+++ b/sound/firewire/bebob/bebob_maudio.c
@@ -381,7 +381,7 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
 
 	id = uval->value.enumerated.item[0];
 	if (id >= ARRAY_SIZE(special_clk_labels))
-		return 0;
+		return -EINVAL;
 
 	mutex_lock(&bebob->mutex);
 
-- 
1.9.1

Re: [PATCH 1/3] bebob: Fix a missing to unlock mutex in error handling case

[Takashi Sakamoto]

(Jul 22 2014 23:26), Takashi Iwai wrote:
> BTW, at the next time, put "v2" or such in the subject line so that
> people can distinguish the new patch series from the previous ones.
> You can use --subject-prefix option for git-format-patch or else.

In this time I newly revised these patches, thus there was a lack of
patches in previous series. I thought it better to post them as a new
series.

But I should have added v2 for the series as you indicate.

> Preferably, write the changes since the previous revision, too
> (usually below the --- line, so that it's not merged into the git
>  changelog; some people like to have them in the final commit,
>  though.)

OK.

> Last but not least, don't forget to put a proper subject prefix.
> For ALSA codes, we put "ALSA:" prefix in the subject.  Study git
> changelogs.

Next time, I remember to add the prefix. In my repository, I use the
prefix to distinguish my local patches from upstream patches. But I
should change this workflow.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

Re: [PATCH] ALSA: bebob: Correction for return value of special_clk_ctl_put() in error

[Takashi Iwai]

At Wed, 23 Jul 2014 00:02:08 +0900,
Takashi Sakamoto wrote:
> 
> This commit is a supplement to my previous patch.
> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-July/079190.html
> 
> The special_clk_ctl_put() still returns 0 in error handling case. It should
> return -EINVAL.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/firewire/bebob/bebob_maudio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/firewire/bebob/bebob_maudio.c b/sound/firewire/bebob/bebob_maudio.c
> index d6d6ff8..70faa3a 100644
> --- a/sound/firewire/bebob/bebob_maudio.c
> +++ b/sound/firewire/bebob/bebob_maudio.c
> @@ -381,7 +381,7 @@ static int special_clk_ctl_put(struct snd_kcontrol *kctl,
>  
>  	id = uval->value.enumerated.item[0];
>  	if (id >= ARRAY_SIZE(special_clk_labels))
> -		return 0;
> +		return -EINVAL;
>  
>  	mutex_lock(&bebob->mutex);
>  
> -- 
> 1.9.1
>