[alsa-devel] [PATCH 0/9] ASoC: Yet another small fixes
Hi,
this is yet another series of small fixes for ASoC codec bugs spotted by coverity. All minor issues, obviously.
Takashi
Spotted by coverity CID 712316.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ab8500-codec.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index a0394a8f2257..6e3dda1ca734 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2575,6 +2575,8 @@ static int ab8500_codec_driver_probe(struct platform_device *pdev) /* Create driver private-data struct */ drvdata = devm_kzalloc(&pdev->dev, sizeof(struct ab8500_codec_drvdata), GFP_KERNEL); + if (!drvdata) + return -ENOMEM; drvdata->sid_status = SID_UNCONFIGURED; drvdata->anc_status = ANC_UNCONFIGURED; dev_set_drvdata(&pdev->dev, drvdata);
Otherwise you'll see unrelated error message, too.
Spotted by coverity CID 715173.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ab8500-codec.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 6e3dda1ca734..0693faf7effb 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2132,6 +2132,7 @@ static int ab8500_codec_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) dev_err(dai->codec->dev, "%s: ERROR: The device is either a master or a slave.\n", __func__); + return -EINVAL; default: dev_err(dai->codec->dev, "%s: ERROR: Unsupporter master mask 0x%x\n",
Don't cast to long pointers blindly just for using find_first_bit() and co. This is certainly not portable at all.
Reimplement the code with ffs() and fls() instead. This is a slight optimization, too.
Spotted by coverity CID 1056484 and 1056485.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ab8500-codec.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 0693faf7effb..272df538c1d5 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -2301,17 +2301,17 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, case 0: break; case 1: - slot = find_first_bit((unsigned long *)&tx_mask, 32); + slot = ffs(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; case 2: - slot = find_first_bit((unsigned long *)&tx_mask, 32); + slot = ffs(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, slot); - slot = find_next_bit((unsigned long *)&tx_mask, 32, slot + 1); + slot = fls(tx_mask); snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, slot); snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, slot); break; @@ -2342,18 +2342,18 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai, case 0: break; case 1: - slot = find_first_bit((unsigned long *)&rx_mask, 32); + slot = ffs(rx_mask); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); break; case 2: - slot = find_first_bit((unsigned long *)&rx_mask, 32); + slot = ffs(rx_mask); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot), AB8500_ADSLOTSELX_AD_OUT_TO_SLOT(AB8500_AD_OUT3, slot)); - slot = find_next_bit((unsigned long *)&rx_mask, 32, slot + 1); + slot = fls(rx_mask); snd_soc_update_bits(codec, AB8500_ADSLOTSEL(slot), AB8500_MASK_SLOT(slot),
On Wed, Oct 30, 2013 at 08:35:01AM +0100, Takashi Iwai wrote:
Don't cast to long pointers blindly just for using find_first_bit() and co. This is certainly not portable at all.
Applied, thanks. Please CC people working on the relevant code.
Spotted by coverity CID 115170.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/wm_hubs.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/codecs/wm_hubs.c b/sound/soc/codecs/wm_hubs.c index 8b50e5958de5..01daf655e20b 100644 --- a/sound/soc/codecs/wm_hubs.c +++ b/sound/soc/codecs/wm_hubs.c @@ -530,6 +530,7 @@ static int hp_supply_event(struct snd_soc_dapm_widget *w, hubs->hp_startup_mode); break; } + break;
case SND_SOC_DAPM_PRE_PMD: snd_soc_update_bits(codec, WM8993_CHARGE_PUMP_1,
Spotted by coverity CID 744701.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/wm0010.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm0010.c b/sound/soc/codecs/wm0010.c index d5ebcb00019b..bf7804a12863 100644 --- a/sound/soc/codecs/wm0010.c +++ b/sound/soc/codecs/wm0010.c @@ -793,11 +793,11 @@ static int wm0010_set_sysclk(struct snd_soc_codec *codec, int source, wm0010->max_spi_freq = 0; } else { for (i = 0; i < ARRAY_SIZE(pll_clock_map); i++) - if (freq >= pll_clock_map[i].max_sysclk) + if (freq >= pll_clock_map[i].max_sysclk) { + wm0010->max_spi_freq = pll_clock_map[i].max_pll_spi_speed; + wm0010->pll_clkctrl1 = pll_clock_map[i].pll_clkctrl1; break; - - wm0010->max_spi_freq = pll_clock_map[i].max_pll_spi_speed; - wm0010->pll_clkctrl1 = pll_clock_map[i].pll_clkctrl1; + } }
return 0;
At Wed, 30 Oct 2013 09:38:57 -0700, Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:03AM +0100, Takashi Iwai wrote:
Spotted by coverity CID 744701.
Applied, thanks, though once more please do CC the people working on the code.
... for blaming people who wrote such a buggy code? ;)
Takashi
On Wed, Oct 30, 2013 at 06:32:39PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:03AM +0100, Takashi Iwai wrote:
Spotted by coverity CID 744701.
Applied, thanks, though once more please do CC the people working on the code.
... for blaming people who wrote such a buggy code? ;)
So they can review the change, learn from it and take the fix into any vendor releases they do.
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, 4 insertions(+)
diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c index 8fb072455802..bffcf4e6a2a2 100644 --- a/sound/soc/codecs/max98095.c +++ b/sound/soc/codecs/max98095.c @@ -1740,6 +1740,8 @@ static int max98095_put_eq_enum(struct snd_kcontrol *kcontrol, int fs, best, best_val, i; int regmask, regsave;
+ if (channel < 0) + return channel; BUG_ON(channel > 1);
if (!pdata || !max98095->eq_textcnt) @@ -1798,6 +1800,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 (channel < 0) + return channel; cdata = &max98095->dai[channel]; ucontrol->value.enumerated.item[0] = cdata->eq_sel;
On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:
- if (channel < 0)
return channel;
This doesn't seem to fit in well with the adjacent code - the handling of out of bounds data isn't consistent though it looks like they're both doing essentially the same thing here.
At Wed, 30 Oct 2013 09:46:53 -0700, Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:
- if (channel < 0)
return channel;This doesn't seem to fit in well with the adjacent code - the handling of out of bounds data isn't consistent though it looks like they're both doing essentially the same thing here.
Actually BUG_ON(channel > 1) is a pretty stupid line and should be removed. You'll see that this never happens if you read the code.
Overall, this driver has way too many BUG_ON() without considerations. Such BUT_ON()'s are worse than nothing, IMO.
Takashi
On Wed, Oct 30, 2013 at 06:36:21PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:
- if (channel < 0)
return channel;
This doesn't seem to fit in well with the adjacent code - the handling of out of bounds data isn't consistent though it looks like they're both doing essentially the same thing here.
Actually BUG_ON(channel > 1) is a pretty stupid line and should be removed. You'll see that this never happens if you read the code.
Yeah, but then nor does a return of less than zero...
Overall, this driver has way too many BUG_ON() without considerations. Such BUT_ON()'s are worse than nothing, IMO.
Yeah, I'm ambivalent. I think they're not too bad if it's about a fairly small part of the driver agreeing with itself which is the case here.
At Wed, 30 Oct 2013 14:31:21 -0700, Mark Brown wrote:
On Wed, Oct 30, 2013 at 06:36:21PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:
- if (channel < 0)
return channel;This doesn't seem to fit in well with the adjacent code - the handling of out of bounds data isn't consistent though it looks like they're both doing essentially the same thing here.
Actually BUG_ON(channel > 1) is a pretty stupid line and should be removed. You'll see that this never happens if you read the code.
Yeah, but then nor does a return of less than zero...
Well, max98095_get_eq_channel() returns 0, 1, or a negative error. It's the interface definition. So, as long as the function is written in that way, checking a negative value is logical there, while the condition "channel > 1" never happens, thus a pure garbage.
Of course, if you are paranoid enough and cannot trust the return value from any function, you'd like to check it in allover places. However, in that case, use of BUG_ON() for channel < 0 is still wrong, because a negative value is a valid return value from the function. If you handle it a fatal error (as BUG_ON() does), it means that the design of the function itself is wrong.
Overall, this driver has way too many BUG_ON() without considerations. Such BUT_ON()'s are worse than nothing, IMO.
Yeah, I'm ambivalent. I think they're not too bad if it's about a fairly small part of the driver agreeing with itself which is the case here.
The biggest problem of BUG_ON() is it immediately triggers panic(). BUG_ON() is useful only in the situation where the system can't do anything better than stopping the whole operation at this point. And the only postmortem would be kdump, which is practically never available for embedded devices.
In other words, BUG_ON() is the worst choice in almost all sound/* codes. If a sanity check is needed, use WARN_ON() and handle the error. (Or use snd_BUG_ON() that invokes WARN_ON() when CONFIG_SND_DEBUG is set. Why it's called snd_BUG_ON(), not snd_WARN_ON()? Don't ask me :)
And, if you still insist to use BUG_ON(), use it in the right place; at the cause, not at the result. In this channel check case, it'd be in max98095_get_eq_channel(), put BUG_ON() instead of returning an error. Then all other BUG_ON()'s wrt channel can be omitted.
OK, go back from bikeshedding...
Takashi
get_coeff() may return an error.
Spotted by coverity CID 703394.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ml26124.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/ml26124.c b/sound/soc/codecs/ml26124.c index 26118828782b..4725aa445cc8 100644 --- a/sound/soc/codecs/ml26124.c +++ b/sound/soc/codecs/ml26124.c @@ -342,6 +342,8 @@ static int ml26124_hw_params(struct snd_pcm_substream *substream, struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec); int i = get_coeff(priv->mclk, params_rate(hw_params));
+ if (i < 0) + return -EINVAL; priv->substream = substream; priv->rate = params_rate(hw_params);
On Wed, Oct 30, 2013 at 08:35:05AM +0100, Takashi Iwai wrote:
int i = get_coeff(priv->mclk, params_rate(hw_params));
- if (i < 0)
return -EINVAL;
This is ignoring the supplied error code, I'd expect to see the error code being passed back (and fixed at the point where it's injected if it's just returning -1 or something).
At Wed, 30 Oct 2013 09:48:38 -0700, Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:05AM +0100, Takashi Iwai wrote:
int i = get_coeff(priv->mclk, params_rate(hw_params));
- if (i < 0)
return -EINVAL;This is ignoring the supplied error code, I'd expect to see the error code being passed back (and fixed at the point where it's injected if it's just returning -1 or something).
Ah right. The revised patch is attached below.
Takashi
--- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ASoC: ml26124: Fix negative array index read
get_coeff() may return an error.
Spotted by coverity CID 703394.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/ml26124.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/ml26124.c b/sound/soc/codecs/ml26124.c index 26118828782b..185fa3bc3052 100644 --- a/sound/soc/codecs/ml26124.c +++ b/sound/soc/codecs/ml26124.c @@ -342,6 +342,8 @@ static int ml26124_hw_params(struct snd_pcm_substream *substream, struct ml26124_priv *priv = snd_soc_codec_get_drvdata(codec); int i = get_coeff(priv->mclk, params_rate(hw_params));
+ if (i < 0) + return i; priv->substream = substream; priv->rate = params_rate(hw_params);
On Wed, Oct 30, 2013 at 06:40:04PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
This is ignoring the supplied error code, I'd expect to see the error code being passed back (and fixed at the point where it's injected if it's just returning -1 or something).
Ah right. The revised patch is attached below.
Applied, thanks.
The negative error value returned from get_sdp_info() is ignored because it's assigned to unsigned variables.
Spotted by coverity CIDs 1042657, 1042658.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/rt5640.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c index 4d041d376f31..a3fb41179636 100644 --- a/sound/soc/codecs/rt5640.c +++ b/sound/soc/codecs/rt5640.c @@ -1604,8 +1604,8 @@ static int rt5640_hw_params(struct snd_pcm_substream *substream, struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_codec *codec = rtd->codec; struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); - unsigned int val_len = 0, val_clk, mask_clk, dai_sel; - int pre_div, bclk_ms, frame_size; + unsigned int val_len = 0, val_clk, mask_clk; + int dai_sel, pre_div, bclk_ms, frame_size;
rt5640->lrck[dai->id] = params_rate(params); pre_div = get_clk_info(rt5640->sysclk, rt5640->lrck[dai->id]); @@ -1675,7 +1675,8 @@ static int rt5640_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) { struct snd_soc_codec *codec = dai->codec; struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec); - unsigned int reg_val = 0, dai_sel; + unsigned int reg_val = 0; + int dai_sel;
switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBM_CFM:
Spotted by coverity CID 146355.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/soc/codecs/wm8996.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/wm8996.c b/sound/soc/codecs/wm8996.c index 46fe83d2b224..b70379ebd142 100644 --- a/sound/soc/codecs/wm8996.c +++ b/sound/soc/codecs/wm8996.c @@ -438,6 +438,8 @@ static int wm8996_get_retune_mobile_enum(struct snd_kcontrol *kcontrol, struct wm8996_priv *wm8996 = snd_soc_codec_get_drvdata(codec); int block = wm8996_get_retune_mobile_block(kcontrol->id.name);
+ if (block < 0) + return block; ucontrol->value.enumerated.item[0] = wm8996->retune_mobile_cfg[block];
return 0;
On Wed, Oct 30, 2013 at 08:35:07AM +0100, Takashi Iwai wrote:
int block = wm8996_get_retune_mobile_block(kcontrol->id.name);
- if (block < 0)
return block;
Applied, but this only fixes some of the problem - there's nothing validating that block is in bounds for the array, this now adds code which validates the lower but not upper bound for this.
At Wed, 30 Oct 2013 09:51:03 -0700, Mark Brown wrote:
On Wed, Oct 30, 2013 at 08:35:07AM +0100, Takashi Iwai wrote:
int block = wm8996_get_retune_mobile_block(kcontrol->id.name);
- if (block < 0)
return block;Applied, but this only fixes some of the problem - there's nothing validating that block is in bounds for the array, this now adds code which validates the lower but not upper bound for this.
The function returns either -EINVAL, 0 or 1, so no need for upper bound check.
Takashi
participants (2)
-
Mark Brown -
Takashi Iwai