[alsa-devel] [PATCH 00/31] Kill BUG*() volume 2: ASoC
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
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, ¶m); if (ret)
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.
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:
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.
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;
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.
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)
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.
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 */
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.
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);
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.
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; }
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,
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.
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;
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.
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; }
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.
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);
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.
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;
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.
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; }
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;BUG_ON(!mem); region_name = "DM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "XM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "YM"; reg = wm_adsp_region_to_reg(mem, offset); break;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
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;BUG_ON(!mem); region_name = "DM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "XM"; reg = wm_adsp_region_to_reg(mem, offset); break;BUG_ON(!mem); region_name = "YM"; reg = wm_adsp_region_to_reg(mem, offset); break;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
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
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.
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
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.
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
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.
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;
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.
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);
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
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.
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();
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.
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);
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.
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; }
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
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.
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); }
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;-- 1.8.4.2
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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.
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);
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.
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;
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;
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.
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;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;
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;
We're not using reg here...
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;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
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;
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.
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;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
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.
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
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.
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
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.
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;
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.
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
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; }
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; }
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;
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; }
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; }
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; }
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; }
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; }
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; }
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.
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
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;
participants (5)
-
Charles Keepax -
Jarkko Nikula -
Kuninori Morimoto -
Mark Brown -
Takashi Iwai