[PATCH 00/31] Kill BUG*() volume 2: ASoC

[PATCH 00/31] Kill BUG*() volume 2: ASoC

[Takashi Iwai]

Hi,

this is a second series of small patches to replace the usages of
BUG*() macros, at this time, in ASoC.  And the last patch is a
revival of the previous coverity fix.

The patches have been split to small pieces, but feel free to
combine them appropriately.

 [PATCH 01/31] ASoC: blackfin: Use WARN_ON() instead of BUG_ON()
 [PATCH 02/31] ASoC: max98088: Use WARN_ON() instead of BUG_ON()
 [PATCH 03/31] ASoC: max98095: Use WARN_ON() instead of BUG_ON()
 [PATCH 04/31] ASoC: tpa6130a2: Use WARN_ON() instead of BUG_ON()
 [PATCH 05/31] ASoC: wm0010: Use WARN_ON() instead of BUG_ON()
 [PATCH 06/31] ASoC: wm2000: Use WARN_ON() instead of BUG_ON()
 [PATCH 07/31] ASoC: wm8580: Use WARN() instead of BUG_ON()
 [PATCH 08/31] ASoC: wm5100: Use WARN_ON() instead of BUG_ON()
 [PATCH 09/31] ASoC: wm8776: Use WARN_ON() instead of BUG_ON()
 [PATCH 10/31] ASoC: wm8900: Use WARN_ON() instead of BUG_ON()
 [PATCH 11/31] ASoC: wm8904: Use WARN_ON() instead of BUG_ON()
 [PATCH 12/31] ASoC: wm9713: Use WARN_ON() instead of BUG_ON()
 [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages
 [PATCH 14/31] ASoC: mid-x86: Use WARN_ON() instead of BUG_ON()
 [PATCH 15/31] ASoC: omap: Use WARN_ON() instead of BUG_ON()
 [PATCH 16/31] ASoC: pxa: Use WARN_ON() instead of BUG_ON()
 [PATCH 17/31] ASoC: s6000: Use WARN_ON() instead of BUG_ON()
 [PATCH 18/31] ASoC: rcar: Use WARN_ON() instead of BUG_ON()
 [PATCH 19/31] ASoC: sh: Use WARN_ON() instead of BUG_ON()
 [PATCH 20/31] ASoC: txx9: Use WARN_ON() instead of BUG_ON()
 [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()
 [PATCH 22/31] ASoC: wm5100: Replace BUG() with snd_BUG()
 [PATCH 23/31] ASoC: wm8350: Replace BUG() with snd_BUG()
 [PATCH 24/31] ASoC: wm8900: Replace BUG() with snd_BUG()
 [PATCH 25/31] ASoC: wm8904: Replace BUG() with snd_BUG()
 [PATCH 26/31] ASoC: wm8958: Replace BUG() with snd_BUG()
 [PATCH 27/31] ASoC: wm8962: Replace BUG() with snd_BUG()
 [PATCH 28/31] ASoC: wm8996: Replace BUG() with snd_BUG()
 [PATCH 29/31] ASoC: wm_hubs: Replace BUG() with snd_BUG()
 [PATCH 30/31] ASoC: Replace BUG() with snd_BUG()
 [PATCH 31/31] ASoC: max98095: Add missing negative channel checks


thanks,

Takashi

[PATCH 01/31] ASoC: blackfin: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: uclinux-dist-devel@blackfin.uclinux.org
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/blackfin/bf5xx-sport.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/sound/soc/blackfin/bf5xx-sport.c b/sound/soc/blackfin/bf5xx-sport.c
index 695351241db8..9dfa1241ea66 100644
--- a/sound/soc/blackfin/bf5xx-sport.c
+++ b/sound/soc/blackfin/bf5xx-sport.c
@@ -179,8 +179,9 @@ static inline int sport_hook_rx_dummy(struct sport_device *sport)
 	struct dmasg *desc, temp_desc;
 	unsigned long flags;
 
-	BUG_ON(sport->dummy_rx_desc == NULL);
-	BUG_ON(sport->curr_rx_desc == sport->dummy_rx_desc);
+	if (WARN_ON(!sport->dummy_rx_desc) ||
+	    WARN_ON(sport->curr_rx_desc == sport->dummy_rx_desc))
+		return -EINVAL;
 
 	/* Maybe the dummy buffer descriptor ring is damaged */
 	sport->dummy_rx_desc->next_desc_addr = sport->dummy_rx_desc + 1;
@@ -250,8 +251,9 @@ int sport_rx_start(struct sport_device *sport)
 		return -EBUSY;
 	if (sport->tx_run) {
 		/* tx is running, rx is not running */
-		BUG_ON(sport->dma_rx_desc == NULL);
-		BUG_ON(sport->curr_rx_desc != sport->dummy_rx_desc);
+		if (WARN_ON(!sport->dma_rx_desc) ||
+		    WARN_ON(sport->curr_rx_desc != sport->dummy_rx_desc))
+			return -EINVAL;
 		local_irq_save(flags);
 		while ((get_dma_curr_desc_ptr(sport->dma_rx_chan) -
 			sizeof(struct dmasg)) != sport->dummy_rx_desc)
@@ -298,8 +300,9 @@ static inline int sport_hook_tx_dummy(struct sport_device *sport)
 	struct dmasg *desc, temp_desc;
 	unsigned long flags;
 
-	BUG_ON(sport->dummy_tx_desc == NULL);
-	BUG_ON(sport->curr_tx_desc == sport->dummy_tx_desc);
+	if (WARN_ON(!sport->dummy_tx_desc) ||
+	    WARN_ON(sport->curr_tx_desc == sport->dummy_tx_desc))
+		return -EINVAL;
 
 	sport->dummy_tx_desc->next_desc_addr = sport->dummy_tx_desc + 1;
 
@@ -331,8 +334,9 @@ int sport_tx_start(struct sport_device *sport)
 	if (sport->tx_run)
 		return -EBUSY;
 	if (sport->rx_run) {
-		BUG_ON(sport->dma_tx_desc == NULL);
-		BUG_ON(sport->curr_tx_desc != sport->dummy_tx_desc);
+		if (WARN_ON(!sport->dma_tx_desc) ||
+		    WARN_ON(sport->curr_tx_desc != sport->dummy_tx_desc))
+			return -EINVAL;
 		/* Hook the normal buffer descriptor */
 		local_irq_save(flags);
 		while ((get_dma_curr_desc_ptr(sport->dma_tx_chan) -
@@ -767,7 +771,8 @@ static irqreturn_t err_handler(int irq, void *dev_id)
 int sport_set_rx_callback(struct sport_device *sport,
 		       void (*rx_callback)(void *), void *rx_data)
 {
-	BUG_ON(rx_callback == NULL);
+	if (WARN_ON(!rx_callback))
+		return -EINVAL;
 	sport->rx_callback = rx_callback;
 	sport->rx_data = rx_data;
 
@@ -778,7 +783,8 @@ EXPORT_SYMBOL(sport_set_rx_callback);
 int sport_set_tx_callback(struct sport_device *sport,
 		void (*tx_callback)(void *), void *tx_data)
 {
-	BUG_ON(tx_callback == NULL);
+	if (WARN_ON(!tx_callback))
+		return -EINVAL;
 	sport->tx_callback = tx_callback;
 	sport->tx_data = tx_data;
 
@@ -789,7 +795,8 @@ EXPORT_SYMBOL(sport_set_tx_callback);
 int sport_set_err_callback(struct sport_device *sport,
 		void (*err_callback)(void *), void *err_data)
 {
-	BUG_ON(err_callback == NULL);
+	if (WARN_ON(!err_callback))
+		return -EINVAL;
 	sport->err_callback = err_callback;
 	sport->err_data = err_data;
 
@@ -856,7 +863,8 @@ struct sport_device *sport_init(struct platform_device *pdev,
 
 	param.wdsize = wdsize;
 	param.dummy_count = dummy_count;
-	BUG_ON(param.wdsize == 0 || param.dummy_count == 0);
+	if (WARN_ON(param.wdsize == 0 || param.dummy_count == 0))
+		return NULL;
 
 	ret = sport_config_pdev(pdev, &param);
 	if (ret)
-- 
1.8.4.2

Re: [PATCH 01/31] ASoC: blackfin: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:48PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 02/31] ASoC: max98088: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/max98088.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
index 66ceee22fdad..53d7dab4e054 100644
--- a/sound/soc/codecs/max98088.c
+++ b/sound/soc/codecs/max98088.c
@@ -568,8 +568,9 @@ static void m98088_eq_band(struct snd_soc_codec *codec, unsigned int dai,
        unsigned int eq_reg;
        unsigned int i;
 
-       BUG_ON(band > 4);
-       BUG_ON(dai > 1);
+	if (WARN_ON(band > 4) ||
+	    WARN_ON(dai > 1))
+		return;
 
        /* Load the base register address */
        eq_reg = dai ? M98088_REG_84_DAI2_EQ_BASE : M98088_REG_52_DAI1_EQ_BASE;
@@ -909,7 +910,8 @@ static int max98088_line_pga(struct snd_soc_dapm_widget *w,
        struct max98088_priv *max98088 = snd_soc_codec_get_drvdata(codec);
        u8 *state;
 
-       BUG_ON(!((channel == 1) || (channel == 2)));
+	if (WARN_ON(!(channel == 1 || channel == 2)))
+		return -EINVAL;
 
        switch (line) {
        case LINE_INA:
-- 
1.8.4.2

Re: [PATCH 02/31] ASoC: max98088: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:49PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 03/31] ASoC: max98095: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/max98095.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
index 8fb072455802..67244315c721 100644
--- a/sound/soc/codecs/max98095.c
+++ b/sound/soc/codecs/max98095.c
@@ -516,8 +516,9 @@ static void m98095_eq_band(struct snd_soc_codec *codec, unsigned int dai,
 	unsigned int eq_reg;
 	unsigned int i;
 
-	BUG_ON(band > 4);
-	BUG_ON(dai > 1);
+	if (WARN_ON(band > 4) ||
+	    WARN_ON(dai > 1))
+		return;
 
 	/* Load the base register address */
 	eq_reg = dai ? M98095_142_DAI2_EQ_BASE : M98095_110_DAI1_EQ_BASE;
@@ -541,8 +542,9 @@ static void m98095_biquad_band(struct snd_soc_codec *codec, unsigned int dai,
 	unsigned int bq_reg;
 	unsigned int i;
 
-	BUG_ON(band > 1);
-	BUG_ON(dai > 1);
+	if (WARN_ON(band > 1) ||
+	    WARN_ON(dai > 1))
+		return;
 
 	/* Load the base register address */
 	bq_reg = dai ? M98095_17E_DAI2_BQ_BASE : M98095_174_DAI1_BQ_BASE;
@@ -890,7 +892,8 @@ static int max98095_line_pga(struct snd_soc_dapm_widget *w,
 	struct max98095_priv *max98095 = snd_soc_codec_get_drvdata(codec);
 	u8 *state;
 
-	BUG_ON(!((channel == 1) || (channel == 2)));
+	if (WARN_ON(!(channel == 1 || channel == 2)))
+		return -EINVAL;
 
 	state = &max98095->lin_state;
 
@@ -1740,7 +1743,8 @@ static int max98095_put_eq_enum(struct snd_kcontrol *kcontrol,
 	int fs, best, best_val, i;
 	int regmask, regsave;
 
-	BUG_ON(channel > 1);
+	if (WARN_ON(channel > 1))
+		return -EINVAL;
 
 	if (!pdata || !max98095->eq_textcnt)
 		return 0;
-- 
1.8.4.2

Re: [PATCH 03/31] ASoC: max98095: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 295 bytes --]

On Tue, Nov 05, 2013 at 06:39:50PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().

That doesn't really follow, they print out an obvious error message and
force people to pay attention to it.  Sometimes that's the desired
effect.

Anyway, applied.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 04/31] ASoC: tpa6130a2: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/tpa6130a2.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
index 998555f2a8aa..b27c396037d4 100644
--- a/sound/soc/codecs/tpa6130a2.c
+++ b/sound/soc/codecs/tpa6130a2.c
@@ -56,7 +56,8 @@ static int tpa6130a2_i2c_read(int reg)
 	struct tpa6130a2_data *data;
 	int val;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	/* If powered off, return the cached value */
@@ -78,7 +79,8 @@ static int tpa6130a2_i2c_write(int reg, u8 value)
 	struct tpa6130a2_data *data;
 	int val = 0;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	if (data->power_state) {
@@ -99,7 +101,8 @@ static u8 tpa6130a2_read(int reg)
 {
 	struct tpa6130a2_data *data;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return 0;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	return data->regs[reg];
@@ -110,7 +113,8 @@ static int tpa6130a2_initialize(void)
 	struct tpa6130a2_data *data;
 	int i, ret = 0;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	for (i = 1; i < TPA6130A2_REG_VERSION; i++) {
@@ -128,7 +132,8 @@ static int tpa6130a2_power(u8 power)
 	u8	val;
 	int	ret = 0;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	mutex_lock(&data->mutex);
@@ -194,7 +199,8 @@ static int tpa6130a2_get_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int mask = (1 << fls(max)) - 1;
 	unsigned int invert = mc->invert;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	mutex_lock(&data->mutex);
@@ -224,7 +230,8 @@ static int tpa6130a2_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int val = (ucontrol->value.integer.value[0] & mask);
 	unsigned int val_reg;
 
-	BUG_ON(tpa6130a2_client == NULL);
+	if (WARN_ON(!tpa6130a2_client))
+		return -EINVAL;
 	data = i2c_get_clientdata(tpa6130a2_client);
 
 	if (invert)
-- 
1.8.4.2

Re: [PATCH 04/31] ASoC: tpa6130a2: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:51PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 05/31] ASoC: wm0010: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm0010.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm0010.c b/sound/soc/codecs/wm0010.c
index bf7804a12863..71ce3159a62e 100644
--- a/sound/soc/codecs/wm0010.c
+++ b/sound/soc/codecs/wm0010.c
@@ -372,7 +372,8 @@ static int wm0010_firmware_load(const char *name, struct snd_soc_codec *codec)
 	offset = 0;
 	dsp = inforec->dsp_target;
 	wm0010->boot_failed = false;
-	BUG_ON(!list_empty(&xfer_list));
+	if (WARN_ON(!list_empty(&xfer_list)))
+		return -EINVAL;
 	init_completion(&done);
 
 	/* First record should be INFO */
-- 
1.8.4.2

Re: [PATCH 05/31] ASoC: wm0010: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:52PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 06/31] ASoC: wm2000: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm2000.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/wm2000.c b/sound/soc/codecs/wm2000.c
index 7fefd766b582..8ae50274ea8f 100644
--- a/sound/soc/codecs/wm2000.c
+++ b/sound/soc/codecs/wm2000.c
@@ -137,7 +137,8 @@ static int wm2000_power_up(struct i2c_client *i2c, int analogue)
 	unsigned long rate;
 	int ret;
 
-	BUG_ON(wm2000->anc_mode != ANC_OFF);
+	if (WARN_ON(wm2000->anc_mode != ANC_OFF))
+		return -EINVAL;
 
 	dev_dbg(&i2c->dev, "Beginning power up\n");
 
@@ -277,7 +278,8 @@ static int wm2000_enter_bypass(struct i2c_client *i2c, int analogue)
 {
 	struct wm2000_priv *wm2000 = dev_get_drvdata(&i2c->dev);
 
-	BUG_ON(wm2000->anc_mode != ANC_ACTIVE);
+	if (WARN_ON(wm2000->anc_mode != ANC_ACTIVE))
+		return -EINVAL;
 
 	if (analogue) {
 		wm2000_write(i2c, WM2000_REG_SYS_MODE_CNTRL,
@@ -315,7 +317,8 @@ static int wm2000_exit_bypass(struct i2c_client *i2c, int analogue)
 {
 	struct wm2000_priv *wm2000 = dev_get_drvdata(&i2c->dev);
 
-	BUG_ON(wm2000->anc_mode != ANC_BYPASS);
+	if (WARN_ON(wm2000->anc_mode != ANC_BYPASS))
+		return -EINVAL;
 	
 	wm2000_write(i2c, WM2000_REG_SYS_CTL1, 0);
 
@@ -349,7 +352,8 @@ static int wm2000_enter_standby(struct i2c_client *i2c, int analogue)
 {
 	struct wm2000_priv *wm2000 = dev_get_drvdata(&i2c->dev);
 
-	BUG_ON(wm2000->anc_mode != ANC_ACTIVE);
+	if (WARN_ON(wm2000->anc_mode != ANC_ACTIVE))
+		return -EINVAL;
 
 	if (analogue) {
 		wm2000_write(i2c, WM2000_REG_ANA_VMID_PD_TIME, 248 / 4);
@@ -392,7 +396,8 @@ static int wm2000_exit_standby(struct i2c_client *i2c, int analogue)
 {
 	struct wm2000_priv *wm2000 = dev_get_drvdata(&i2c->dev);
 
-	BUG_ON(wm2000->anc_mode != ANC_STANDBY);
+	if (WARN_ON(wm2000->anc_mode != ANC_STANDBY))
+		return -EINVAL;
 
 	wm2000_write(i2c, WM2000_REG_SYS_CTL1, 0);
 
-- 
1.8.4.2

Re: [PATCH 06/31] ASoC: wm2000: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:53PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 07/31] ASoC: wm8580: Use WARN() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8580.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8580.c b/sound/soc/codecs/wm8580.c
index 5e9c40fa7eb2..08a414b57b1e 100644
--- a/sound/soc/codecs/wm8580.c
+++ b/sound/soc/codecs/wm8580.c
@@ -736,7 +736,7 @@ static int wm8580_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 		break;
 
 	default:
-		BUG_ON("Unknown DAI driver ID\n");
+		WARN(1, "Unknown DAI driver ID\n");
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

Re: [PATCH 07/31] ASoC: wm8580: Use WARN() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 169 bytes --]

On Tue, Nov 05, 2013 at 06:39:54PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN() instead.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 08/31] ASoC: wm5100: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm5100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c
index ac1745d030d6..4cf91deabc02 100644
--- a/sound/soc/codecs/wm5100.c
+++ b/sound/soc/codecs/wm5100.c
@@ -1972,7 +1972,8 @@ static void wm5100_set_detect_mode(struct wm5100_priv *wm5100, int the_mode)
 {
 	struct wm5100_jack_mode *mode = &wm5100->pdata.jack_modes[the_mode];
 
-	BUG_ON(the_mode >= ARRAY_SIZE(wm5100->pdata.jack_modes));
+	if (WARN_ON(the_mode >= ARRAY_SIZE(wm5100->pdata.jack_modes)))
+		return;
 
 	gpio_set_value_cansleep(wm5100->pdata.hp_pol, mode->hp_pol);
 	regmap_update_bits(wm5100->regmap, WM5100_ACCESSORY_DETECT_MODE_1,
-- 
1.8.4.2

Re: [PATCH 08/31] ASoC: wm5100: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:55PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 09/31] ASoC: wm8776: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8776.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8776.c b/sound/soc/codecs/wm8776.c
index f31017ed1381..942d58e455f3 100644
--- a/sound/soc/codecs/wm8776.c
+++ b/sound/soc/codecs/wm8776.c
@@ -325,7 +325,8 @@ static int wm8776_set_sysclk(struct snd_soc_dai *dai,
 	struct snd_soc_codec *codec = dai->codec;
 	struct wm8776_priv *wm8776 = snd_soc_codec_get_drvdata(codec);
 
-	BUG_ON(dai->driver->id >= ARRAY_SIZE(wm8776->sysclk));
+	if (WARN_ON(dai->driver->id >= ARRAY_SIZE(wm8776->sysclk)))
+		return -EINVAL;
 
 	wm8776->sysclk[dai->driver->id] = freq;
 
-- 
1.8.4.2

Re: [PATCH 09/31] ASoC: wm8776: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:56PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 10/31] ASoC: wm8900: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8900.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/wm8900.c b/sound/soc/codecs/wm8900.c
index 7c8257c5a17b..de67e74dca0c 100644
--- a/sound/soc/codecs/wm8900.c
+++ b/sound/soc/codecs/wm8900.c
@@ -691,7 +691,8 @@ static int fll_factors(struct _fll_div *fll_div, unsigned int Fref,
 	unsigned int K, Ndiv, Nmod, target;
 	unsigned int div;
 
-	BUG_ON(!Fout);
+	if (WARN_ON(!Fout))
+		return -EINVAL;
 
 	/* The FLL must run at 90-100MHz which is then scaled down to
 	 * the output value by FLLCLK_DIV. */
@@ -742,8 +743,9 @@ static int fll_factors(struct _fll_div *fll_div, unsigned int Fref,
 	/* Move down to proper range now rounding is done */
 	fll_div->k = K / 10;
 
-	BUG_ON(target != Fout * (fll_div->fllclk_div << 2));
-	BUG_ON(!K && target != Fref * fll_div->fll_ratio * fll_div->n);
+	if (WARN_ON(target != Fout * (fll_div->fllclk_div << 2)) ||
+	    WARN_ON(!K && target != Fref * fll_div->fll_ratio * fll_div->n))
+		return -EINVAL;
 
 	return 0;
 }
-- 
1.8.4.2

Re: [PATCH 10/31] ASoC: wm8900: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:57PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 11/31] ASoC: wm8904: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8904.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index 4dfa8dceeabf..c173ab3cd18a 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -658,7 +658,8 @@ SOC_SINGLE_TLV("EQ5 Volume", WM8904_EQ6, 0, 24, 0, eq_tlv),
 static int cp_event(struct snd_soc_dapm_widget *w,
 		    struct snd_kcontrol *kcontrol, int event)
 {
-	BUG_ON(event != SND_SOC_DAPM_POST_PMU);
+	if (WARN_ON(event != SND_SOC_DAPM_POST_PMU))
+		return -EINVAL;
 
 	/* Maximum startup time */
 	udelay(500);
-- 
1.8.4.2

Re: [PATCH 11/31] ASoC: wm8904: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:58PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 12/31] ASoC: wm9713: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm9713.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c
index a53e175c015a..acea8927905b 100644
--- a/sound/soc/codecs/wm9713.c
+++ b/sound/soc/codecs/wm9713.c
@@ -221,7 +221,8 @@ static int wm9713_voice_shutdown(struct snd_soc_dapm_widget *w,
 	struct snd_soc_codec *codec = w->codec;
 	u16 status, rate;
 
-	BUG_ON(event != SND_SOC_DAPM_PRE_PMD);
+	if (WARN_ON(event != SND_SOC_DAPM_PRE_PMD))
+		return -EINVAL;
 
 	/* Gracefully shut down the voice interface. */
 	status = ac97_read(codec, AC97_EXTENDED_MID) | 0x1000;
-- 
1.8.4.2

Re: [PATCH 12/31] ASoC: wm9713: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:39:59PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Also, the driver triggers BUG_ON() in the caller side, which can be
reduced easily by putting an assert in the callee side.

This patch does:
- Move the sanity check with WARN_ON() in wm_adsp_region_to_reg() and
  remove the checks in the callers,
- Fix wrong WARN_ON() usages, replaced with WARN(),
- Fix unreachable or wrong BUG_ON() usages and replace with WARN_ON().

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm_adsp.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index 53b6033658a6..83c255815a2f 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -341,6 +341,8 @@ static struct wm_adsp_region const *wm_adsp_find_region(struct wm_adsp *dsp,
 static unsigned int wm_adsp_region_to_reg(struct wm_adsp_region const *region,
 					  unsigned int offset)
 {
+	if (WARN_ON(!region))
+		return offset;
 	switch (region->type) {
 	case WMFW_ADSP1_PM:
 		return region->base + (offset * 3);
@@ -353,7 +355,7 @@ static unsigned int wm_adsp_region_to_reg(struct wm_adsp_region const *region,
 	case WMFW_ADSP1_ZM:
 		return region->base + (offset * 2);
 	default:
-		WARN_ON(NULL != "Unknown memory region type");
+		WARN(1, "Unknown memory region type");
 		return offset;
 	}
 }
@@ -605,7 +607,7 @@ static int wm_adsp_load(struct wm_adsp *dsp)
 		break;
 
 	default:
-		BUG_ON(NULL == "Unknown DSP type");
+		WARN(1, "Unknown DSP type");
 		goto out_fw;
 	}
 
@@ -645,27 +647,22 @@ static int wm_adsp_load(struct wm_adsp *dsp)
 			reg = offset;
 			break;
 		case WMFW_ADSP1_PM:
-			BUG_ON(!mem);
 			region_name = "PM";
 			reg = wm_adsp_region_to_reg(mem, offset);
 			break;
 		case WMFW_ADSP1_DM:
-			BUG_ON(!mem);
 			region_name = "DM";
 			reg = wm_adsp_region_to_reg(mem, offset);
 			break;
 		case WMFW_ADSP2_XM:
-			BUG_ON(!mem);
 			region_name = "XM";
 			reg = wm_adsp_region_to_reg(mem, offset);
 			break;
 		case WMFW_ADSP2_YM:
-			BUG_ON(!mem);
 			region_name = "YM";
 			reg = wm_adsp_region_to_reg(mem, offset);
 			break;
 		case WMFW_ADSP1_ZM:
-			BUG_ON(!mem);
 			region_name = "ZM";
 			reg = wm_adsp_region_to_reg(mem, offset);
 			break;
@@ -905,10 +902,8 @@ static int wm_adsp_setup_algs(struct wm_adsp *dsp)
 		break;
 	}
 
-	if (mem == NULL) {
-		BUG_ON(mem != NULL);
+	if (WARN_ON(!mem))
 		return -EINVAL;
-	}
 
 	switch (dsp->type) {
 	case WMFW_ADSP1:
@@ -1002,7 +997,7 @@ static int wm_adsp_setup_algs(struct wm_adsp *dsp)
 		break;
 
 	default:
-		BUG_ON(NULL == "Unknown DSP type");
+		WARN(1, "Unknown DSP type");
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Charles Keepax]

On Tue, Nov 05, 2013 at 06:40:00PM +0100, Takashi Iwai wrote:
> @@ -645,27 +647,22 @@ static int wm_adsp_load(struct wm_adsp *dsp)
>  			reg = offset;
>  			break;
>  		case WMFW_ADSP1_PM:
> -			BUG_ON(!mem);
>  			region_name = "PM";
>  			reg = wm_adsp_region_to_reg(mem, offset);
>  			break;
>  		case WMFW_ADSP1_DM:
> -			BUG_ON(!mem);
>  			region_name = "DM";
>  			reg = wm_adsp_region_to_reg(mem, offset);
>  			break;
>  		case WMFW_ADSP2_XM:
> -			BUG_ON(!mem);
>  			region_name = "XM";
>  			reg = wm_adsp_region_to_reg(mem, offset);
>  			break;
>  		case WMFW_ADSP2_YM:
> -			BUG_ON(!mem);
>  			region_name = "YM";
>  			reg = wm_adsp_region_to_reg(mem, offset);
>  			break;
>  		case WMFW_ADSP1_ZM:
> -			BUG_ON(!mem);
>  			region_name = "ZM";
>  			reg = wm_adsp_region_to_reg(mem, offset);
>  			break;

These do make me a little nervous previously we would panic here,
so everything would stop but now we may write random registers on
the CODEC which could cause some odd behaviour. I think it would
be better to update wm_adsp_region_to_reg to allow a failed
return code and then error out of wm_adsp_load rather than just
relying on the WARN_ON inside.

Otherwise the patch looks good to me though.

Thanks,
Charles

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Takashi Iwai]

At Wed, 6 Nov 2013 18:44:22 +0000,
Charles Keepax wrote:
> 
> On Tue, Nov 05, 2013 at 06:40:00PM +0100, Takashi Iwai wrote:
> > @@ -645,27 +647,22 @@ static int wm_adsp_load(struct wm_adsp *dsp)
> >  			reg = offset;
> >  			break;
> >  		case WMFW_ADSP1_PM:
> > -			BUG_ON(!mem);
> >  			region_name = "PM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP1_DM:
> > -			BUG_ON(!mem);
> >  			region_name = "DM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP2_XM:
> > -			BUG_ON(!mem);
> >  			region_name = "XM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP2_YM:
> > -			BUG_ON(!mem);
> >  			region_name = "YM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> >  		case WMFW_ADSP1_ZM:
> > -			BUG_ON(!mem);
> >  			region_name = "ZM";
> >  			reg = wm_adsp_region_to_reg(mem, offset);
> >  			break;
> 
> These do make me a little nervous previously we would panic here,
> so everything would stop but now we may write random registers on
> the CODEC which could cause some odd behaviour. I think it would
> be better to update wm_adsp_region_to_reg to allow a failed
> return code and then error out of wm_adsp_load rather than just
> relying on the WARN_ON inside.

Yeah, handling the error properly would be definitely better.
Care to enhance the patch?

Actually, I decided to work on and send these patch series just
because I casually found that there are so many BUG*() macros in the
tree.  Fixing bugs aren't my intention.

Note that it's perfectly fine that BUG*() are used in the development
code during the development phase.  It's a matter of debugging
techniques.

However, It's definitely not OK if these BUG*() are left in the
production code.  Particularly, the common code (like codec drivers)
that can be called by others shouldn't trigger the death button like
that.

This problem can be seen from several perspectives.

Firstly, leaving BUG*() means essentially that the driver can be
buggy.  If such a problem might happen in reality, you should have
written the driver code more cleanly to handle the exceptions.

Secondly, more importantly, it's end-users who would suffer from the
sudden death problem, not the developers.  This is the production
level, so you can't justify the sudden death.  And, with panic() by
BUG*(), developers would receive only news that machines are killed
without dying messages.


Takashi

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Charles Keepax]

On Wed, Nov 06, 2013 at 09:23:42PM +0100, Takashi Iwai wrote:
> At Wed, 6 Nov 2013 18:44:22 +0000,
> Charles Keepax wrote:
> > These do make me a little nervous previously we would panic here,
> > so everything would stop but now we may write random registers on
> > the CODEC which could cause some odd behaviour. I think it would
> > be better to update wm_adsp_region_to_reg to allow a failed
> > return code and then error out of wm_adsp_load rather than just
> > relying on the WARN_ON inside.
> 
> Yeah, handling the error properly would be definitely better.
> Care to enhance the patch?

Yeah fair enough, lets merge the patch as is and I will send out a
seperate patch once I get a few spare minutes to improve the
error handling in this bit of code.

Reviewed-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Thanks,
Charles

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 772 bytes --]

On Wed, Nov 06, 2013 at 09:23:42PM +0100, Takashi Iwai wrote:

> Secondly, more importantly, it's end-users who would suffer from the
> sudden death problem, not the developers.  This is the production
> level, so you can't justify the sudden death.  And, with panic() by
> BUG*(), developers would receive only news that machines are killed
> without dying messages.

With the sort of systems these things are sold into users don't have any
direct access to anything that'd let them trigger problems - the devices
are sold as full stack integrated systems and the system integrators
don't generally differentiate between what error handling would do and
direct failures that much.  It is better to handle things gracefully but
we need to be realistic about the benefits.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Takashi Iwai]

At Thu, 7 Nov 2013 10:47:25 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 06, 2013 at 09:23:42PM +0100, Takashi Iwai wrote:
> 
> > Secondly, more importantly, it's end-users who would suffer from the
> > sudden death problem, not the developers.  This is the production
> > level, so you can't justify the sudden death.  And, with panic() by
> > BUG*(), developers would receive only news that machines are killed
> > without dying messages.
> 
> With the sort of systems these things are sold into users don't have any
> direct access to anything that'd let them trigger problems - the devices
> are sold as full stack integrated systems and the system integrators
> don't generally differentiate between what error handling would do and
> direct failures that much.  It is better to handle things gracefully but
> we need to be realistic about the benefits.

Yes, and think that this is the first step.  At least, we can refuse
the use of BUG*() in any of merged materials in future.  Leaving
BUG*() is a bad sign of lazy error handling.

And, actually your vendor-lock assumption doesn't look valid any
longer.  There are more and more ARM-based systems that can install
OS later relatively easily.  Most of distros (openSUSE, Ubuntu,
Fedora, etc) already provide the installation images, and I know many
users of Chromebooks running such.  (And I know some of them burned
speakers due to the incorrect UCM setup or usage :-)

That is, ASoC is no longer a platform used only for embedded devices
that vendors have controls on, but it's deployed in wider fields.
There are (and will be more) end-users who are free from the vendor's
system.  We are still responsible for them.


thanks,

Takashi

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1339 bytes --]

On Thu, Nov 07, 2013 at 12:07:39PM +0100, Takashi Iwai wrote:

> Yes, and think that this is the first step.  At least, we can refuse
> the use of BUG*() in any of merged materials in future.  Leaving
> BUG*() is a bad sign of lazy error handling.

Send patches to remove the definition of BUG then...

> And, actually your vendor-lock assumption doesn't look valid any
> longer.  There are more and more ARM-based systems that can install
> OS later relatively easily.  Most of distros (openSUSE, Ubuntu,

Practically speaking those systems are all in the same class, the
general purpose end user ARM systems don't have audio hardware (things
like the Chromebook are still closed boxes with a developer mode which
isn't quite the same thing).  We certainly don't have anyone actually
working on any of the stuff that would make such environments viable,
the people trying are focusing on servers.

> Fedora, etc) already provide the installation images, and I know many
> users of Chromebooks running such.  (And I know some of them burned
> speakers due to the incorrect UCM setup or usage :-)

Right, there's a whole bunch more stuff to do if you actually want to
handle this stuff in production - the health and safety issues from
excessive volume are even more of a concern for me than the issues with
physical damage to the systems.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Takashi Iwai]

At Thu, 7 Nov 2013 11:51:58 +0000,
Mark Brown wrote:
> 
> On Thu, Nov 07, 2013 at 12:07:39PM +0100, Takashi Iwai wrote:
> 
> > Yes, and think that this is the first step.  At least, we can refuse
> > the use of BUG*() in any of merged materials in future.  Leaving
> > BUG*() is a bad sign of lazy error handling.
> 
> Send patches to remove the definition of BUG then...

Oh no, it's not my point.

BUG*() macros are good for the right places, per se.  It's just wrong
usages in drivers.  And I'm not against anyone using the macros for
debugging or developing.  My only point is not to leave them in the
final form in the sound drivers.

> > And, actually your vendor-lock assumption doesn't look valid any
> > longer.  There are more and more ARM-based systems that can install
> > OS later relatively easily.  Most of distros (openSUSE, Ubuntu,
> 
> Practically speaking those systems are all in the same class, the
> general purpose end user ARM systems don't have audio hardware (things
> like the Chromebook are still closed boxes with a developer mode which
> isn't quite the same thing).  We certainly don't have anyone actually
> working on any of the stuff that would make such environments viable,
> the people trying are focusing on servers.

Yeah, it's still few, but who knows.

> > Fedora, etc) already provide the installation images, and I know many
> > users of Chromebooks running such.  (And I know some of them burned
> > speakers due to the incorrect UCM setup or usage :-)
> 
> Right, there's a whole bunch more stuff to do if you actually want to
> handle this stuff in production - the health and safety issues from
> excessive volume are even more of a concern for me than the issues with
> physical damage to the systems.

It's a good question how to handle such a problem in general.

The driver could give fair limits, if it can know beforehand.
OTOH, conceptually, it'd be cleaner for a driver to give the raw
accessibility of the hardware as much as possible.  Most of ASoC codec
drivers follow the latter model.  It works well with the closed system
in practice, until a box is opened... 


Takashi

Re: [PATCH 13/31] ASoC: wm_adsp: Fix BUG_ON() and WARN_ON() usages

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 275 bytes --]

On Tue, Nov 05, 2013 at 06:40:00PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Also, the driver triggers BUG_ON() in the caller side, which can be
> reduced easily by putting an assert in the callee side.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 14/31] ASoC: mid-x86: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/mid-x86/sst_platform.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/mid-x86/sst_platform.c b/sound/soc/mid-x86/sst_platform.c
index 392fc0b8f5b8..3e94660d452b 100644
--- a/sound/soc/mid-x86/sst_platform.c
+++ b/sound/soc/mid-x86/sst_platform.c
@@ -40,7 +40,8 @@ static DEFINE_MUTEX(sst_lock);
 
 int sst_register_dsp(struct sst_device *dev)
 {
-	BUG_ON(!dev);
+	if (WARN_ON(!dev))
+		return _EINVAL;
 	if (!try_module_get(dev->dev->driver->owner))
 		return -ENODEV;
 	mutex_lock(&sst_lock);
@@ -59,7 +60,8 @@ EXPORT_SYMBOL_GPL(sst_register_dsp);
 
 int sst_unregister_dsp(struct sst_device *dev)
 {
-	BUG_ON(!dev);
+	if (WARN_ON(!dev))
+		return -EINVAL;
 	if (dev != sst)
 		return -EINVAL;
 
-- 
1.8.4.2

Re: [PATCH 14/31] ASoC: mid-x86: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:01PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 15/31] ASoC: omap: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Jarkko Nikula <jarkko.nikula@bitmer.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/omap/n810.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/soc/omap/n810.c b/sound/soc/omap/n810.c
index 5e8d640d314f..6d216cb6c19b 100644
--- a/sound/soc/omap/n810.c
+++ b/sound/soc/omap/n810.c
@@ -344,8 +344,11 @@ static int __init n810_soc_init(void)
 	clk_set_parent(sys_clkout2_src, func96m_clk);
 	clk_set_rate(sys_clkout2, 12000000);
 
-	BUG_ON((gpio_request(N810_HEADSET_AMP_GPIO, "hs_amp") < 0) ||
-	       (gpio_request(N810_SPEAKER_AMP_GPIO, "spk_amp") < 0));
+	if (WARN_ON((gpio_request(N810_HEADSET_AMP_GPIO, "hs_amp") < 0) ||
+		    (gpio_request(N810_SPEAKER_AMP_GPIO, "spk_amp") < 0))) {
+		err = -EINVAL;
+		goto err4;
+	}
 
 	gpio_direction_output(N810_HEADSET_AMP_GPIO, 0);
 	gpio_direction_output(N810_SPEAKER_AMP_GPIO, 0);
-- 
1.8.4.2

Re: [PATCH 15/31] ASoC: omap: Use WARN_ON() instead of BUG_ON()

[Jarkko Nikula]

On Tue, Nov 05, 2013 at 06:40:02PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.
> 
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: Jarkko Nikula <jarkko.nikula@bitmer.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/omap/n810.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/omap/n810.c b/sound/soc/omap/n810.c
> index 5e8d640d314f..6d216cb6c19b 100644
> --- a/sound/soc/omap/n810.c
> +++ b/sound/soc/omap/n810.c
> @@ -344,8 +344,11 @@ static int __init n810_soc_init(void)
>  	clk_set_parent(sys_clkout2_src, func96m_clk);
>  	clk_set_rate(sys_clkout2, 12000000);
>  
> -	BUG_ON((gpio_request(N810_HEADSET_AMP_GPIO, "hs_amp") < 0) ||
> -	       (gpio_request(N810_SPEAKER_AMP_GPIO, "spk_amp") < 0));
> +	if (WARN_ON((gpio_request(N810_HEADSET_AMP_GPIO, "hs_amp") < 0) ||
> +		    (gpio_request(N810_SPEAKER_AMP_GPIO, "spk_amp") < 0))) {

Strong ack! (/me wondering why I put it there in the first place)

Acked-by: Jarkko Nikula <jarkko.nikula@bitmer.com>

Re: [PATCH 15/31] ASoC: omap: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:02PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 16/31] ASoC: pxa: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Haojian Zhuang <haojian.zhuang@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/pxa/pxa2xx-i2s.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/pxa/pxa2xx-i2s.c b/sound/soc/pxa/pxa2xx-i2s.c
index d5340a088858..c0d648d3339f 100644
--- a/sound/soc/pxa/pxa2xx-i2s.c
+++ b/sound/soc/pxa/pxa2xx-i2s.c
@@ -165,7 +165,8 @@ static int pxa2xx_i2s_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_dmaengine_dai_dma_data *dma_data;
 
-	BUG_ON(IS_ERR(clk_i2s));
+	if (WARN_ON(IS_ERR(clk_i2s)))
+		return -EINVAL;
 	clk_prepare_enable(clk_i2s);
 	clk_ena = 1;
 	pxa_i2s_wait();
-- 
1.8.4.2

Re: [PATCH 16/31] ASoC: pxa: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:03PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 17/31] ASoC: s6000: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/s6000/s6000-pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/s6000/s6000-pcm.c b/sound/soc/s6000/s6000-pcm.c
index d0740a762963..5cfaa5464eba 100644
--- a/sound/soc/s6000/s6000-pcm.c
+++ b/sound/soc/s6000/s6000-pcm.c
@@ -90,7 +90,8 @@ static void s6000_pcm_enqueue_dma(struct snd_pcm_substream *substream)
 		return;
 	}
 
-	BUG_ON(period_size & 15);
+	if (WARN_ON(period_size & 15))
+		return;
 	s6dmac_put_fifo(DMA_MASK_DMAC(channel), DMA_INDEX_CHNL(channel),
 			src, dst, period_size);
 
-- 
1.8.4.2

Re: [PATCH 17/31] ASoC: s6000: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:04PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 18/31] ASoC: rcar: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/sh/rcar/scu.c | 3 ++-
 sound/soc/sh/rcar/ssi.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/rcar/scu.c b/sound/soc/sh/rcar/scu.c
index 1ab1bce6be7f..f4453e33a847 100644
--- a/sound/soc/sh/rcar/scu.c
+++ b/sound/soc/sh/rcar/scu.c
@@ -198,7 +198,8 @@ static struct rsnd_mod_ops rsnd_scu_ops = {
 
 struct rsnd_mod *rsnd_scu_mod_get(struct rsnd_priv *priv, int id)
 {
-	BUG_ON(id < 0 || id >= rsnd_scu_nr(priv));
+	if (WARN_ON(id < 0 || id >= rsnd_scu_nr(priv)))
+		id = 0;
 
 	return &((struct rsnd_scu *)(priv->scu) + id)->mod;
 }
diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
index b71cf9d7dd3f..5ac20cd5e006 100644
--- a/sound/soc/sh/rcar/ssi.c
+++ b/sound/soc/sh/rcar/ssi.c
@@ -611,7 +611,8 @@ struct rsnd_mod *rsnd_ssi_mod_get_frm_dai(struct rsnd_priv *priv,
 
 struct rsnd_mod *rsnd_ssi_mod_get(struct rsnd_priv *priv, int id)
 {
-	BUG_ON(id < 0 || id >= rsnd_ssi_nr(priv));
+	if (WARN_ON(id < 0 || id >= rsnd_ssi_nr(priv)))
+		id = 0;
 
 	return &(((struct rsnd_ssiu *)(priv->ssiu))->ssi + id)->mod;
 }
-- 
1.8.4.2

Re: [PATCH 18/31] ASoC: rcar: Use WARN_ON() instead of BUG_ON()

[Kuninori Morimoto]

At Tue,  5 Nov 2013 18:40:05 +0100,
Takashi Iwai wrote:
> 
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.
> 
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

>  sound/soc/sh/rcar/scu.c | 3 ++-
>  sound/soc/sh/rcar/ssi.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/sh/rcar/scu.c b/sound/soc/sh/rcar/scu.c
> index 1ab1bce6be7f..f4453e33a847 100644
> --- a/sound/soc/sh/rcar/scu.c
> +++ b/sound/soc/sh/rcar/scu.c
> @@ -198,7 +198,8 @@ static struct rsnd_mod_ops rsnd_scu_ops = {
>  
>  struct rsnd_mod *rsnd_scu_mod_get(struct rsnd_priv *priv, int id)
>  {
> -	BUG_ON(id < 0 || id >= rsnd_scu_nr(priv));
> +	if (WARN_ON(id < 0 || id >= rsnd_scu_nr(priv)))
> +		id = 0;
>  
>  	return &((struct rsnd_scu *)(priv->scu) + id)->mod;
>  }
> diff --git a/sound/soc/sh/rcar/ssi.c b/sound/soc/sh/rcar/ssi.c
> index b71cf9d7dd3f..5ac20cd5e006 100644
> --- a/sound/soc/sh/rcar/ssi.c
> +++ b/sound/soc/sh/rcar/ssi.c
> @@ -611,7 +611,8 @@ struct rsnd_mod *rsnd_ssi_mod_get_frm_dai(struct rsnd_priv *priv,
>  
>  struct rsnd_mod *rsnd_ssi_mod_get(struct rsnd_priv *priv, int id)
>  {
> -	BUG_ON(id < 0 || id >= rsnd_ssi_nr(priv));
> +	if (WARN_ON(id < 0 || id >= rsnd_ssi_nr(priv)))
> +		id = 0;
>  
>  	return &(((struct rsnd_ssiu *)(priv->ssiu))->ssi + id)->mod;
>  }
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 18/31] ASoC: rcar: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:05PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 19/31] ASoC: sh: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/sh/siu_dai.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sh/siu_dai.c b/sound/soc/sh/siu_dai.c
index 9dc24ffa892a..d55babee14f8 100644
--- a/sound/soc/sh/siu_dai.c
+++ b/sound/soc/sh/siu_dai.c
@@ -543,7 +543,8 @@ static void siu_dai_shutdown(struct snd_pcm_substream *substream,
 	/* Stop the siu if the other stream is not using it */
 	if (!port_info->play_cap) {
 		/* during stmread or stmwrite ? */
-		BUG_ON(port_info->playback.rw_flg || port_info->capture.rw_flg);
+		if (WARN_ON(port_info->playback.rw_flg || port_info->capture.rw_flg))
+			return;
 		siu_dai_spbstop(port_info);
 		siu_dai_stop(port_info);
 	}
-- 
1.8.4.2

Re: [PATCH 19/31] ASoC: sh: Use WARN_ON() instead of BUG_ON()

[Kuninori Morimoto]

At Tue,  5 Nov 2013 18:40:06 +0100,
Takashi Iwai wrote:
> 
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.
> 
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

>  sound/soc/sh/siu_dai.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/sh/siu_dai.c b/sound/soc/sh/siu_dai.c
> index 9dc24ffa892a..d55babee14f8 100644
> --- a/sound/soc/sh/siu_dai.c
> +++ b/sound/soc/sh/siu_dai.c
> @@ -543,7 +543,8 @@ static void siu_dai_shutdown(struct snd_pcm_substream *substream,
>  	/* Stop the siu if the other stream is not using it */
>  	if (!port_info->play_cap) {
>  		/* during stmread or stmwrite ? */
> -		BUG_ON(port_info->playback.rw_flg || port_info->capture.rw_flg);
> +		if (WARN_ON(port_info->playback.rw_flg || port_info->capture.rw_flg))
> +			return;
>  		siu_dai_spbstop(port_info);
>  		siu_dai_stop(port_info);
>  	}
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 19/31] ASoC: sh: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:06PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 20/31] ASoC: txx9: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/txx9/txx9aclc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/txx9/txx9aclc.c b/sound/soc/txx9/txx9aclc.c
index 45a6428cba8d..fbd077f4de72 100644
--- a/sound/soc/txx9/txx9aclc.c
+++ b/sound/soc/txx9/txx9aclc.c
@@ -115,8 +115,8 @@ static void txx9aclc_dma_complete(void *arg)
 	spin_lock_irqsave(&dmadata->dma_lock, flags);
 	if (dmadata->frag_count >= 0) {
 		dmadata->dmacount--;
-		BUG_ON(dmadata->dmacount < 0);
-		tasklet_schedule(&dmadata->tasklet);
+		if (!WARN_ON(dmadata->dmacount < 0))
+			tasklet_schedule(&dmadata->tasklet);
 	}
 	spin_unlock_irqrestore(&dmadata->dma_lock, flags);
 }
@@ -181,7 +181,10 @@ static void txx9aclc_dma_tasklet(unsigned long data)
 		spin_unlock_irqrestore(&dmadata->dma_lock, flags);
 		return;
 	}
-	BUG_ON(dmadata->dmacount >= NR_DMA_CHAIN);
+	if (WARN_ON(dmadata->dmacount >= NR_DMA_CHAIN)) {
+		spin_unlock_irqrestore(&dmadata->dma_lock, flags);
+		return;
+	}
 	while (dmadata->dmacount < NR_DMA_CHAIN) {
 		dmadata->dmacount++;
 		spin_unlock_irqrestore(&dmadata->dma_lock, flags);
-- 
1.8.4.2

Re: [PATCH 20/31] ASoC: txx9: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 203 bytes --]

On Tue, Nov 05, 2013 at 06:40:07PM +0100, Takashi Iwai wrote:
> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-dapm.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index cc36caaf6443..c89bbe5a65e7 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1460,7 +1460,8 @@ static void dapm_seq_run_coalesced(struct snd_soc_card *card,
 			       power_list)->reg;
 
 	list_for_each_entry(w, pending, power_list) {
-		BUG_ON(reg != w->reg);
+		if (WARN_ON(reg != w->reg))
+			return;
 		w->power = w->new_power;
 
 		mask |= w->mask << w->shift;
@@ -3359,8 +3360,10 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	u64 fmt;
 	int ret;
 
-	BUG_ON(!config);
-	BUG_ON(list_empty(&w->sources) || list_empty(&w->sinks));
+	if (WARN_ON(!config))
+		return -EINVAL;
+	if (WARN_ON(list_empty(&w->sources) || list_empty(&w->sinks)))
+		return -EINVAL;
 
 	/* We only support a single source and sink, pick the first */
 	source_p = list_first_entry(&w->sources, struct snd_soc_dapm_path,
@@ -3368,9 +3371,12 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	sink_p = list_first_entry(&w->sinks, struct snd_soc_dapm_path,
 				  list_source);
 
-	BUG_ON(!source_p || !sink_p);
-	BUG_ON(!sink_p->source || !source_p->sink);
-	BUG_ON(!source_p->source || !sink_p->sink);
+	if (WARN_ON(!source_p || !sink_p))
+		return -EINVAL;
+	if (WARN_ON(!sink_p->source || !source_p->sink))
+		return -EINVAL;
+	if (WARN_ON(!source_p->source || !sink_p->sink))
+		return -EINVAL;
 
 	source = source_p->source->priv;
 	sink = sink_p->sink->priv;
-- 
1.8.4.2

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 446 bytes --]

On Tue, Nov 05, 2013 at 06:40:08PM +0100, Takashi Iwai wrote:

>  	list_for_each_entry(w, pending, power_list) {
> -		BUG_ON(reg != w->reg);
> +		if (WARN_ON(reg != w->reg))
> +			return;
>  		w->power = w->new_power;

This doesn't seem at all sensible - it's going to suddenly abort the
entire sequence run over the error which if we're actually trying to
continue is just going to make things worse.  I'd expect to at most skip
over the entry.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 10:24:31 +0000,
Mark Brown wrote:
> 
> On Tue, Nov 05, 2013 at 06:40:08PM +0100, Takashi Iwai wrote:
> 
> >  	list_for_each_entry(w, pending, power_list) {
> > -		BUG_ON(reg != w->reg);
> > +		if (WARN_ON(reg != w->reg))
> > +			return;
> >  		w->power = w->new_power;
> 
> This doesn't seem at all sensible - it's going to suddenly abort the
> entire sequence run over the error which if we're actually trying to
> continue is just going to make things worse.  I'd expect to at most skip
> over the entry.

OK, what about this one?


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH v2] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

BUG_ON() is rather useless for debugging as it leads to panic().
Use WARN_ON() and handle the error cases accordingly.

Also another WARN_ON() check is added in dapm_seq_run_coalesced()
since now the show goes on after the first WARN_ON().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-dapm.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index cc36caaf6443..d191a1aee432 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1460,7 +1460,8 @@ static void dapm_seq_run_coalesced(struct snd_soc_card *card,
 			       power_list)->reg;
 
 	list_for_each_entry(w, pending, power_list) {
-		BUG_ON(reg != w->reg);
+		if (WARN_ON(reg != w->reg))
+			continue;
 		w->power = w->new_power;
 
 		mask |= w->mask << w->shift;
@@ -1493,6 +1494,8 @@ static void dapm_seq_run_coalesced(struct snd_soc_card *card,
 	}
 
 	list_for_each_entry(w, pending, power_list) {
+		if (WARN_ON(reg != w->reg))
+			continue;
 		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
 		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
 	}
@@ -3359,8 +3362,10 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	u64 fmt;
 	int ret;
 
-	BUG_ON(!config);
-	BUG_ON(list_empty(&w->sources) || list_empty(&w->sinks));
+	if (WARN_ON(!config))
+		return -EINVAL;
+	if (WARN_ON(list_empty(&w->sources) || list_empty(&w->sinks)))
+		return -EINVAL;
 
 	/* We only support a single source and sink, pick the first */
 	source_p = list_first_entry(&w->sources, struct snd_soc_dapm_path,
@@ -3368,9 +3373,12 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	sink_p = list_first_entry(&w->sinks, struct snd_soc_dapm_path,
 				  list_source);
 
-	BUG_ON(!source_p || !sink_p);
-	BUG_ON(!sink_p->source || !source_p->sink);
-	BUG_ON(!source_p->source || !sink_p->sink);
+	if (WARN_ON(!source_p || !sink_p))
+		return -EINVAL;
+	if (WARN_ON(!sink_p->source || !source_p->sink))
+		return -EINVAL;
+	if (WARN_ON(!source_p->source || !sink_p->sink))
+		return -EINVAL;
 
 	source = source_p->source->priv;
 	sink = sink_p->sink->priv;
-- 
1.8.4.2

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 666 bytes --]

On Wed, Nov 06, 2013 at 12:05:00PM +0100, Takashi Iwai wrote:

> BUG_ON() is rather useless for debugging as it leads to panic().
> Use WARN_ON() and handle the error cases accordingly.

Please fix the comment about BUG_ON() being useless; it does exactly
what it's supposed to do.

> Also another WARN_ON() check is added in dapm_seq_run_coalesced()
> since now the show goes on after the first WARN_ON().

Why?

>  	list_for_each_entry(w, pending, power_list) {
> +		if (WARN_ON(reg != w->reg))
> +			continue;
>  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
>  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
>  	}

We're not using reg here...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 11:15:29 +0000,
Mark Brown wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Wed, Nov 06, 2013 at 12:05:00PM +0100, Takashi Iwai wrote:
> 
> > BUG_ON() is rather useless for debugging as it leads to panic().
> > Use WARN_ON() and handle the error cases accordingly.
> 
> Please fix the comment about BUG_ON() being useless; it does exactly
> what it's supposed to do.

Well,  I can correct the word "useless" with "stupid" in the comment,
as BUG_ON() is the most stupid idea to do at such a point.  soc-dapm.c
is the common code that can be executed from any others. How you can
know that the machine *must* be killed suddenly at this point?  If you
didn't attach the console beforehand, you can't get even any debug
logs.

Using BUG() and BUG_ON() in buggy points without any really serious
damage (like heavy memory corruption or data corruption) has to be
avoided.  Linus stated this in the past, too.  (I have no pointer
now, unfortunately.)


> > Also another WARN_ON() check is added in dapm_seq_run_coalesced()
> > since now the show goes on after the first WARN_ON().
> 
> Why?
> 
> >  	list_for_each_entry(w, pending, power_list) {
> > +		if (WARN_ON(reg != w->reg))
> > +			continue;
> >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
> >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
> >  	}
> 
> We're not using reg here...

Because you must not issue *_POST_* events without *_PRE_* events.
*_PRE_* are skipped for these entries in the previous WARN_ON()
checks.


Takashi

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1715 bytes --]

On Wed, Nov 06, 2013 at 12:33:14PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Please fix the comment about BUG_ON() being useless; it does exactly
> > what it's supposed to do.

> Well,  I can correct the word "useless" with "stupid" in the comment,
> as BUG_ON() is the most stupid idea to do at such a point.  soc-dapm.c
> is the common code that can be executed from any others. How you can
> know that the machine *must* be killed suddenly at this point?  If you
> didn't attach the console beforehand, you can't get even any debug
> logs.

The point is that it's not appropriate in this situation but not
useless; if it were useless you should be sending a patch to remove it
from the kernel entirely.  

> Using BUG() and BUG_ON() in buggy points without any really serious
> damage (like heavy memory corruption or data corruption) has to be
> avoided.  Linus stated this in the past, too.  (I have no pointer
> now, unfortunately.)

The ones in DAPM are mostly about memory corruption; if this stuff is
getting confused then there's a good chance that the data structures
that it's looking at aren't what they're supposed to be.

> > >  	list_for_each_entry(w, pending, power_list) {
> > > +		if (WARN_ON(reg != w->reg))
> > > +			continue;
> > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
> > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
> > >  	}

> > We're not using reg here...

> Because you must not issue *_POST_* events without *_PRE_* events.
> *_PRE_* are skipped for these entries in the previous WARN_ON()
> checks.

No, that's not the case at all - it's very rare for devices to have both
events in the first place.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 11:48:36 +0000,
Mark Brown wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Wed, Nov 06, 2013 at 12:33:14PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > Please fix the comment about BUG_ON() being useless; it does exactly
> > > what it's supposed to do.
> 
> > Well,  I can correct the word "useless" with "stupid" in the comment,
> > as BUG_ON() is the most stupid idea to do at such a point.  soc-dapm.c
> > is the common code that can be executed from any others. How you can
> > know that the machine *must* be killed suddenly at this point?  If you
> > didn't attach the console beforehand, you can't get even any debug
> > logs.
> 
> The point is that it's not appropriate in this situation but not
> useless; if it were useless you should be sending a patch to remove it
> from the kernel entirely.  

OK, feel free to run s/useless/inappropriate/ in the patch.
I don't mind wording but only codes.

> > Using BUG() and BUG_ON() in buggy points without any really serious
> > damage (like heavy memory corruption or data corruption) has to be
> > avoided.  Linus stated this in the past, too.  (I have no pointer
> > now, unfortunately.)
> 
> The ones in DAPM are mostly about memory corruption; if this stuff is
> getting confused then there's a good chance that the data structures
> that it's looking at aren't what they're supposed to be.

This isn't so easy to guess.  Such a bug is often a memory corruption
indeed, but it's not necessarily so.  The data can be modified by the
device drivers that are calling dapm, so everything is possible.
Driver writers can do anything unbelievable you can never imagine...

Practically seen, using BUG_ON() makes such debugging even more
difficult.

> > > >  	list_for_each_entry(w, pending, power_list) {
> > > > +		if (WARN_ON(reg != w->reg))
> > > > +			continue;
> > > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMU);
> > > >  		dapm_seq_check_event(card, w, SND_SOC_DAPM_POST_PMD);
> > > >  	}
> 
> > > We're not using reg here...
> 
> > Because you must not issue *_POST_* events without *_PRE_* events.
> > *_PRE_* are skipped for these entries in the previous WARN_ON()
> > checks.
> 
> No, that's not the case at all - it's very rare for devices to have both
> events in the first place.

Hrm, are you saying that *_POST events can be issued without the
corresponding *_PRE events, as an official dpam design?  I didn't
expect such a bad consistency in API design level...


Takashi

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1976 bytes --]

On Wed, Nov 06, 2013 at 01:03:41PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > The ones in DAPM are mostly about memory corruption; if this stuff is
> > getting confused then there's a good chance that the data structures
> > that it's looking at aren't what they're supposed to be.

> This isn't so easy to guess.  Such a bug is often a memory corruption
> indeed, but it's not necessarily so.  The data can be modified by the
> device drivers that are calling dapm, so everything is possible.
> Driver writers can do anything unbelievable you can never imagine...

If the driver is modifying the DAPM data then it looses anyway.

> Practically seen, using BUG_ON() makes such debugging even more
> difficult.

That's not been my experience at all with these ones, it's much more
common for people to ignore soft failures and then wonder why they get
breakage further down the line.

This is also partly a function of the usage model for these drivers - if
someone is triggering an issue they will almost certainly be doing
development on the system.  There aren't the problems with random system
configurations or unknown hardware that you see with specs like HDA.

> > > Because you must not issue *_POST_* events without *_PRE_* events.
> > > *_PRE_* are skipped for these entries in the previous WARN_ON()
> > > checks.

> > No, that's not the case at all - it's very rare for devices to have both
> > events in the first place.

> Hrm, are you saying that *_POST events can be issued without the
> corresponding *_PRE events, as an official dpam design?  I didn't
> expect such a bad consistency in API design level...

They shouldn't be generated unmatched but it's extremely rare for them
both to actually be used by one widget.  To the extent that they're used
matched it's with the PRE_PMU matched with POST_PMD and vice versa,
though using just one is quite common for inserting delays.  It's going
to cause more harm than good to actively suppress events.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 16:03:04 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 06, 2013 at 01:03:41PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > The ones in DAPM are mostly about memory corruption; if this stuff is
> > > getting confused then there's a good chance that the data structures
> > > that it's looking at aren't what they're supposed to be.
> 
> > This isn't so easy to guess.  Such a bug is often a memory corruption
> > indeed, but it's not necessarily so.  The data can be modified by the
> > device drivers that are calling dapm, so everything is possible.
> > Driver writers can do anything unbelievable you can never imagine...
> 
> If the driver is modifying the DAPM data then it looses anyway.

Yes, but you cannot know in DAPM level.  That's the point.
In soc-dapm.c, you know only something is wrong.  But you can never
know how severely wrong it is.

> > Practically seen, using BUG_ON() makes such debugging even more
> > difficult.
> 
> That's not been my experience at all with these ones, it's much more
> common for people to ignore soft failures and then wonder why they get
> breakage further down the line.
> 
> This is also partly a function of the usage model for these drivers - if
> someone is triggering an issue they will almost certainly be doing
> development on the system.  There aren't the problems with random system
> configurations or unknown hardware that you see with specs like HDA.

My concern here is that dapm isn't in the position to decide whether
you press the self destruction switch at all.  DAPM, or in general
ASoC core, should serve at best it can, should return an error if
something looks weird.  As repeatedly mentioned, in the layer of DAPM,
you cannot know what could be wrong, have too little to judge whether
it's an emergency exit or not.

So, basically what such lazy people would need is a knob to make
WARN*() triggering panic().  But this shouldn't be done
unconditionally for all DAPM usages, or better in all sound driver
usages.

BTW, another substantial problem is that this dapm code doesn't handle
any errors...

> > > > Because you must not issue *_POST_* events without *_PRE_* events.
> > > > *_PRE_* are skipped for these entries in the previous WARN_ON()
> > > > checks.
> 
> > > No, that's not the case at all - it's very rare for devices to have both
> > > events in the first place.
> 
> > Hrm, are you saying that *_POST events can be issued without the
> > corresponding *_PRE events, as an official dpam design?  I didn't
> > expect such a bad consistency in API design level...
> 
> They shouldn't be generated unmatched but it's extremely rare for them
> both to actually be used by one widget.

So, the unprocessed list item must be filtered out.

>  To the extent that they're used
> matched it's with the PRE_PMU matched with POST_PMD and vice versa,
> though using just one is quite common for inserting delays.  It's going
> to cause more harm than good to actively suppress events.

It won't suppress any events that have been working.
Remember that there's been BUG_ON() with the very same condition
before the patch.  Thus any good-working cases must not hit with this
conversion.


Takashi

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2076 bytes --]

On Wed, Nov 06, 2013 at 06:21:36PM +0100, Takashi Iwai wrote:

> So, basically what such lazy people would need is a knob to make
> WARN*() triggering panic().  But this shouldn't be done
> unconditionally for all DAPM usages, or better in all sound driver
> usages.

Sure, it's a bit nicer in some situations.  My point is that you're
greatly overstating the problems with asserting in practical usage.
The environment and productisation process used with this stuff is
different to that for PCs.

> BTW, another substantial problem is that this dapm code doesn't handle
> any errors...

Feel free to send patches.  I've never considered it a high priority to
go through and add stuff there since the unwinding code is way more
effort to write than it's worth and would never get tested - generally
where constructive error recovery is possible the natural user reaction
of stopping and restarting things is about as constructive as it gets
anyway.  There's always something more pressing to work on.

> > They shouldn't be generated unmatched but it's extremely rare for them
> > both to actually be used by one widget.

> So, the unprocessed list item must be filtered out.

I don't understand what you mean by this?

> >  To the extent that they're used
> > matched it's with the PRE_PMU matched with POST_PMD and vice versa,
> > though using just one is quite common for inserting delays.  It's going
> > to cause more harm than good to actively suppress events.

> It won't suppress any events that have been working.
> Remember that there's been BUG_ON() with the very same condition
> before the patch.  Thus any good-working cases must not hit with this
> conversion.

But then we're still left with bombing out if the first half of the
function ever changes.  The check is there for the benefit of the
register handling, the fact that it's affecting events is just a side
effect rather than the intention.

Remember also that BUG() can be disabled so there is a faint chance that
we'd not trigger, and of course the issue might be introduced between
the two events.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Wed, 6 Nov 2013 23:48:06 +0000,
Mark Brown wrote:
> 
> On Wed, Nov 06, 2013 at 06:21:36PM +0100, Takashi Iwai wrote:
> 
> > So, basically what such lazy people would need is a knob to make
> > WARN*() triggering panic().  But this shouldn't be done
> > unconditionally for all DAPM usages, or better in all sound driver
> > usages.
> 
> Sure, it's a bit nicer in some situations.  My point is that you're
> greatly overstating the problems with asserting in practical usage.
> The environment and productisation process used with this stuff is
> different to that for PCs.

I do understand that BUG*() can be used in the development phase.
But my intention in the whole silly patchsets is to eliminate BUG*()
in the production code.  What we have in our git trees are supposed to
be all in the production level.  At such a stage, we shouldn't leave
mines.  As already mentioned in another post, it'd be end-users who
suffer from the explosion, and the developer would get much less
benefit by that.  You'll see only a dead body after explosion.

So, just kill these BUG*() in your code.  It's irresponsible to leave
such things in the final code.


> > BTW, another substantial problem is that this dapm code doesn't handle
> > any errors...
> 
> Feel free to send patches.  I've never considered it a high priority to
> go through and add stuff there since the unwinding code is way more
> effort to write than it's worth and would never get tested - generally
> where constructive error recovery is possible the natural user reaction
> of stopping and restarting things is about as constructive as it gets
> anyway.  There's always something more pressing to work on.
> 
> > > They shouldn't be generated unmatched but it's extremely rare for them
> > > both to actually be used by one widget.
> 
> > So, the unprocessed list item must be filtered out.
> 
> I don't understand what you mean by this?

The insertion of the very same check is to avoid applying the POST
events for the items that haven't been processed for PRE events.

> > >  To the extent that they're used
> > > matched it's with the PRE_PMU matched with POST_PMD and vice versa,
> > > though using just one is quite common for inserting delays.  It's going
> > > to cause more harm than good to actively suppress events.
> 
> > It won't suppress any events that have been working.
> > Remember that there's been BUG_ON() with the very same condition
> > before the patch.  Thus any good-working cases must not hit with this
> > conversion.
> 
> But then we're still left with bombing out if the first half of the
> function ever changes.  The check is there for the benefit of the
> register handling, the fact that it's affecting events is just a side
> effect rather than the intention.

Actually the check reg == w->reg must be always true for all pending
items, no matter at which point.  Otherwise BUG() would have hit.

With the patch, the behavior is changed to that the item that doesn't
satisfy reg == w->reg will be skipped in the first loop.  At the
second loop, these items need to be skipped as well.  They are broken
items, and the PRE_ events weren't applied, thus you must not apply
POST_ events to them --- it's the very reason of the second loop
check.

That being said, maybe a cleaner way would be to run the check over
the loop once, and quit the function if anything inconsistent found.

> Remember also that BUG() can be disabled so there is a faint chance that
> we'd not trigger, and of course the issue might be introduced between
> the two events.

If the issue were to be introduced between two loops, it's rather
nicer if you can catch there? :)

Well, I really don't mind how to implement.  Again, my wish is just to
kill crappy BUG*() calls from my tree.


thanks,

Takashi

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1773 bytes --]

On Thu, Nov 07, 2013 at 08:31:59AM +0100, Takashi Iwai wrote:

> > But then we're still left with bombing out if the first half of the
> > function ever changes.  The check is there for the benefit of the
> > register handling, the fact that it's affecting events is just a side
> > effect rather than the intention.

> Actually the check reg == w->reg must be always true for all pending
> items, no matter at which point.  Otherwise BUG() would have hit.

Right, but that's not actually what the check is there for.  The loop is
both doing an event check and building up the register value to write,
the check is to tell us if we're confused about the register value since
we'll end up writing to the wrong register during coalescing.

> That being said, maybe a cleaner way would be to run the check over
> the loop once, and quit the function if anything inconsistent found.

Or do two passes, one for the registers and one for the events.  You
could also reorder the loop to pull the events to the top and then do
the check and skip between them and the value update, or simply remove
the BUG().

Or just don't worry about it and leave the second loop alone since none
of this is ideal - possibly the most helpful thing would be to insert a
non-coalesced write for the out of place widget, though depending on why
things are going wrong perhaps that'll just make things worse and we
should be trying to unwind the entire transaction instead.  Unwinding
the entire thing is going to be the generally safest but least helpful
option.

My main concern here is not to confuse the code by having the check for
the register value protecting an event call, that'll be confusing later
on.  If you just want to fix the BUG()s do that alone, change them to
WARN()s or whatever.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Thu, 7 Nov 2013 11:31:45 +0000,
Mark Brown wrote:
> 
> On Thu, Nov 07, 2013 at 08:31:59AM +0100, Takashi Iwai wrote:
> 
> > > But then we're still left with bombing out if the first half of the
> > > function ever changes.  The check is there for the benefit of the
> > > register handling, the fact that it's affecting events is just a side
> > > effect rather than the intention.
> 
> > Actually the check reg == w->reg must be always true for all pending
> > items, no matter at which point.  Otherwise BUG() would have hit.
> 
> Right, but that's not actually what the check is there for.  The loop is
> both doing an event check and building up the register value to write,
> the check is to tell us if we're confused about the register value since
> we'll end up writing to the wrong register during coalescing.
> 
> > That being said, maybe a cleaner way would be to run the check over
> > the loop once, and quit the function if anything inconsistent found.
> 
> Or do two passes, one for the registers and one for the events.  You
> could also reorder the loop to pull the events to the top and then do
> the check and skip between them and the value update, or simply remove
> the BUG().
> 
> Or just don't worry about it and leave the second loop alone since none
> of this is ideal - possibly the most helpful thing would be to insert a
> non-coalesced write for the out of place widget, though depending on why
> things are going wrong perhaps that'll just make things worse and we
> should be trying to unwind the entire transaction instead.  Unwinding
> the entire thing is going to be the generally safest but least helpful
> option.
> 
> My main concern here is not to confuse the code by having the check for
> the register value protecting an event call, that'll be confusing later
> on.  If you just want to fix the BUG()s do that alone, change them to
> WARN()s or whatever.

OK, back to square, I simplified the patch without extra error
handling, just replacing with WARN_ON() at that point.  The revised
patch is below.


Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

Leaving BUG_ON() in a core layer like dapm is rather inappropriate as
it leads to panic(), even though sanity checks might be still useful
for debugging.
Instead, Use WARN_ON(), and handle the error cases accordingly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-dapm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index cc36caaf6443..36f4553a2973 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1460,7 +1460,7 @@ static void dapm_seq_run_coalesced(struct snd_soc_card *card,
 			       power_list)->reg;
 
 	list_for_each_entry(w, pending, power_list) {
-		BUG_ON(reg != w->reg);
+		WARN_ON(reg != w->reg);
 		w->power = w->new_power;
 
 		mask |= w->mask << w->shift;
@@ -3359,8 +3359,9 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	u64 fmt;
 	int ret;
 
-	BUG_ON(!config);
-	BUG_ON(list_empty(&w->sources) || list_empty(&w->sinks));
+	if (WARN_ON(!config) ||
+	    WARN_ON(list_empty(&w->sources) || list_empty(&w->sinks)))
+		return -EINVAL;
 
 	/* We only support a single source and sink, pick the first */
 	source_p = list_first_entry(&w->sources, struct snd_soc_dapm_path,
@@ -3368,9 +3369,10 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 	sink_p = list_first_entry(&w->sinks, struct snd_soc_dapm_path,
 				  list_source);
 
-	BUG_ON(!source_p || !sink_p);
-	BUG_ON(!sink_p->source || !source_p->sink);
-	BUG_ON(!source_p->source || !sink_p->sink);
+	if (WARN_ON(!source_p || !sink_p) ||
+	    WARN_ON(!sink_p->source || !source_p->sink) ||
+	    WARN_ON(!source_p->source || !sink_p->sink))
+		return -EINVAL;
 
 	source = source_p->source->priv;
 	sink = sink_p->sink->priv;
-- 
1.8.4.2

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 558 bytes --]

On Thu, Nov 07, 2013 at 06:38:47PM +0100, Takashi Iwai wrote:

> OK, back to square, I simplified the patch without extra error
> handling, just replacing with WARN_ON() at that point.  The revised
> patch is below.

Looks good, applied thanks.  By the way...

> ---
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

...if you do this it makes things even harder to apply - the --- gets
the rest cut off by git am so I don't just have to delete the quoted
message but also need to paste in the changelog.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 21/31] ASoC: dapm: Use WARN_ON() instead of BUG_ON()

[Takashi Iwai]

At Thu, 7 Nov 2013 20:01:44 +0000,
Mark Brown wrote:
> 
> On Thu, Nov 07, 2013 at 06:38:47PM +0100, Takashi Iwai wrote:
> 
> > OK, back to square, I simplified the patch without extra error
> > handling, just replacing with WARN_ON() at that point.  The revised
> > patch is below.
> 
> Looks good, applied thanks.  By the way...
> 
> > ---
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ASoC: dapm: Use WARN_ON() instead of BUG_ON()
> 
> ...if you do this it makes things even harder to apply - the --- gets
> the rest cut off by git am so I don't just have to delete the quoted
> message but also need to paste in the changelog.

Ah, OK, will avoid in future.


thanks,

Takashi

[PATCH 22/31] ASoC: wm5100: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm5100.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm5100.c b/sound/soc/codecs/wm5100.c
index 4cf91deabc02..1bc8af51866a 100644
--- a/sound/soc/codecs/wm5100.c
+++ b/sound/soc/codecs/wm5100.c
@@ -1621,7 +1621,7 @@ static int wm5100_set_sysclk(struct snd_soc_codec *codec, int clk_id,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		audio_rate = 0;
 		break;
 	}
-- 
1.8.4.2

[PATCH 23/31] ASoC: wm8350: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8350.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8350.c b/sound/soc/codecs/wm8350.c
index af1318ddb062..26cf60a3b0a8 100644
--- a/sound/soc/codecs/wm8350.c
+++ b/sound/soc/codecs/wm8350.c
@@ -274,7 +274,7 @@ static int pga_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		return -1;
 	}
 
-- 
1.8.4.2

[PATCH 24/31] ASoC: wm8900: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8900.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8900.c b/sound/soc/codecs/wm8900.c
index de67e74dca0c..64c9a3d062d4 100644
--- a/sound/soc/codecs/wm8900.c
+++ b/sound/soc/codecs/wm8900.c
@@ -279,7 +279,7 @@ static int wm8900_hp_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 	}
 
 	return 0;
-- 
1.8.4.2

[PATCH 25/31] ASoC: wm8904: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8904.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c
index c173ab3cd18a..a667726fa79d 100644
--- a/sound/soc/codecs/wm8904.c
+++ b/sound/soc/codecs/wm8904.c
@@ -741,7 +741,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
 		dcs_r = 3;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

[PATCH 26/31] ASoC: wm8958: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8958-dsp2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm8958-dsp2.c b/sound/soc/codecs/wm8958-dsp2.c
index b0710d817a65..a6952e6b9fe3 100644
--- a/sound/soc/codecs/wm8958-dsp2.c
+++ b/sound/soc/codecs/wm8958-dsp2.c
@@ -348,7 +348,7 @@ static void wm8958_dsp_apply(struct snd_soc_codec *codec, int path, int start)
 		aif = 1;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return;
 	}
 
-- 
1.8.4.2

[PATCH 27/31] ASoC: wm8962: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8962.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 3a2f96c5442c..21e2f3cb3407 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -1853,7 +1853,7 @@ static int cp_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
@@ -1945,7 +1945,7 @@ static int hp_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	
 	}
@@ -1974,7 +1974,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
 		reg = WM8962_SPKOUTL_VOLUME;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
@@ -1982,7 +1982,7 @@ static int out_pga_event(struct snd_soc_dapm_widget *w,
 	case SND_SOC_DAPM_POST_PMU:
 		return snd_soc_write(codec, reg, snd_soc_read(codec, reg));
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 }
@@ -2005,7 +2005,7 @@ static int dsp2_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

[PATCH 28/31] ASoC: wm8996: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm8996.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/wm8996.c b/sound/soc/codecs/wm8996.c
index b70379ebd142..7b90f188ecbf 100644
--- a/sound/soc/codecs/wm8996.c
+++ b/sound/soc/codecs/wm8996.c
@@ -610,7 +610,7 @@ static int bg_event(struct snd_soc_dapm_widget *w,
 		wm8996_bg_disable(codec);
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		ret = -EINVAL;
 	}
 
@@ -627,7 +627,7 @@ static int cp_event(struct snd_soc_dapm_widget *w,
 		msleep(5);
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		ret = -EINVAL;
 	}
 
@@ -648,7 +648,7 @@ static int rmv_short_event(struct snd_soc_dapm_widget *w,
 		wm8996->hpout_pending |= w->shift;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
@@ -769,7 +769,7 @@ static int dcs_start(struct snd_soc_dapm_widget *w,
 		wm8996->dcs_pending |= 1 << w->shift;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
@@ -1658,7 +1658,7 @@ static int wm8996_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		lrclk_rx_reg = WM8996_AIF2_RX_LRCLK_2;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
@@ -1770,7 +1770,7 @@ static int wm8996_hw_params(struct snd_pcm_substream *substream,
 		dsp_shift = WM8996_DSP2_DIV_SHIFT;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

[PATCH 29/31] ASoC: wm_hubs: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Cc: patches@opensource.wolfsonmicro.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/wm_hubs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c
index 01daf655e20b..f1cfad3a7e38 100644
--- a/sound/soc/codecs/wm_hubs.c
+++ b/sound/soc/codecs/wm_hubs.c
@@ -611,7 +611,7 @@ static int earpiece_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		break;
 	}
 
-- 
1.8.4.2

[PATCH 30/31] ASoC: Replace BUG() with snd_BUG()

[Takashi Iwai]

BUG() used in the driver is just to spit the stack trace on buggy
points, not really needed to stop the whole operation.  For that
purpose, it'd be more convenient to use snd_BUG() instead.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-cache.c | 4 ++--
 sound/soc/soc-dapm.c  | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index 1b6663f45b34..4737ffcdd589 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -36,7 +36,7 @@ static bool snd_soc_set_cache_val(void *base, unsigned int idx,
 		break;
 	}
 	default:
-		BUG();
+		snd_BUG();
 	}
 	return false;
 }
@@ -57,7 +57,7 @@ static unsigned int snd_soc_get_cache_val(const void *base, unsigned int idx,
 		return cache[idx];
 	}
 	default:
-		BUG();
+		snd_BUG();
 	}
 	/* unreachable */
 	return -1;
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index c89bbe5a65e7..eedb81ec4db4 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -1427,7 +1427,7 @@ static void dapm_seq_check_event(struct snd_soc_card *card,
 		power = 0;
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		return;
 	}
 
@@ -2027,7 +2027,7 @@ static ssize_t dapm_bias_read_file(struct file *file, char __user *user_buf,
 		level = "Off\n";
 		break;
 	default:
-		BUG();
+		snd_BUG();
 		level = "Unknown\n";
 		break;
 	}
@@ -3452,7 +3452,7 @@ static int snd_soc_dai_link_event(struct snd_soc_dapm_widget *w,
 		break;
 
 	default:
-		BUG();
+		snd_BUG();
 		return -EINVAL;
 	}
 
-- 
1.8.4.2

Re: [PATCH 30/31] ASoC: Replace BUG() with snd_BUG()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 582 bytes --]

On Tue, Nov 05, 2013 at 06:40:17PM +0100, Takashi Iwai wrote:
> BUG() used in the driver is just to spit the stack trace on buggy
> points, not really needed to stop the whole operation.  For that
> purpose, it'd be more convenient to use snd_BUG() instead.

snd_BUG() doesn't look like it's quite doing what was desired here -
it's conditional on SND_DEBUG but these are all for really bad problems
which should never happen and will generally cause serious problems if
they don't happen.  It seems better to replace them (or at least most of
them) with the direct WARN() instead.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 30/31] ASoC: Replace BUG() with snd_BUG()

[Takashi Iwai]

At Wed, 6 Nov 2013 09:39:47 +0000,
Mark Brown wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Tue, Nov 05, 2013 at 06:40:17PM +0100, Takashi Iwai wrote:
> > BUG() used in the driver is just to spit the stack trace on buggy
> > points, not really needed to stop the whole operation.  For that
> > purpose, it'd be more convenient to use snd_BUG() instead.
> 
> snd_BUG() doesn't look like it's quite doing what was desired here -
> it's conditional on SND_DEBUG but these are all for really bad problems
> which should never happen and will generally cause serious problems if
> they don't happen.  It seems better to replace them (or at least most of
> them) with the direct WARN() instead.

OK, I don't mind either case, so I'll resend the patches fixing BUG()
(although I myself prefer a possibility to remove such debug things for
the production systems...)


Takashi

[PATCH 31/31] ASoC: max98095: Add missing negative channel checks

[Takashi Iwai]

Otherwise it'd lead to negative array index read.
Spotted by coverity CIDs 143182, 143184.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/max98095.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
index 67244315c721..a70f35cc652e 100644
--- a/sound/soc/codecs/max98095.c
+++ b/sound/soc/codecs/max98095.c
@@ -1743,7 +1743,7 @@ static int max98095_put_eq_enum(struct snd_kcontrol *kcontrol,
 	int fs, best, best_val, i;
 	int regmask, regsave;
 
-	if (WARN_ON(channel > 1))
+	if (WARN_ON(channel < 0 || channel > 1))
 		return -EINVAL;
 
 	if (!pdata || !max98095->eq_textcnt)
@@ -1802,6 +1802,8 @@ static int max98095_get_eq_enum(struct snd_kcontrol *kcontrol,
 	int channel = max98095_get_eq_channel(kcontrol->id.name);
 	struct max98095_cdata *cdata;
 
+	if (WARN_ON(channel < 0 || channel > 1))
+		return -EINVAL;
 	cdata = &max98095->dai[channel];
 	ucontrol->value.enumerated.item[0] = cdata->eq_sel;
 
-- 
1.8.4.2