[PATCH 0/9] ASoC: Yet another small fixes

[PATCH 0/9] ASoC: Yet another small fixes

[Takashi Iwai]

Hi,

this is yet another series of small fixes for ASoC codec bugs spotted
by coverity.  All minor issues, obviously.


Takashi

[PATCH 1/9] ASoC: ab8500: Add missing of NULL check of devm_kzalloc()

[Takashi Iwai]

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);
-- 
1.8.4.1

[PATCH 2/9] ASoC: ab8500: Add missing return in ab8500_codec_set_dai_fmt()

[Takashi Iwai]

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",
-- 
1.8.4.1

[PATCH 3/9] ASoC: ab8500: Fix invalid cast to long pointer

[Takashi Iwai]

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),
-- 
1.8.4.1

[PATCH 4/9] ASoC: wm_hubs: Add missing break in hp_supply_event()

[Takashi Iwai]

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,
-- 
1.8.4.1

[PATCH 5/9] ASoC: wm0010: Fix possible out-of-bounds array read

[Takashi Iwai]

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;
-- 
1.8.4.1

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

[Takashi Iwai]

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

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/codecs/max98095.c | 4 ++++
 1 file changed, 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;
 
-- 
1.8.4.1

[PATCH 7/9] ASoC: ml26124: Fix negative array index read

[Takashi Iwai]

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);
 
-- 
1.8.4.1

[PATCH 8/9] ASoC: rt5640: Fix ignored error checks

[Takashi Iwai]

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:
-- 
1.8.4.1

[PATCH 9/9] ASoC: wm8996: Fix negative array index read

[Takashi Iwai]

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;
-- 
1.8.4.1

Re: [PATCH 1/9] ASoC: ab8500: Add missing of NULL check of devm_kzalloc()

[Mark Brown]

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

On Wed, Oct 30, 2013 at 08:34:59AM +0100, Takashi Iwai wrote:
> Spotted by coverity CID 712316.

Applied, thanks.

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

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

Re: [PATCH 3/9] ASoC: ab8500: Fix invalid cast to long pointer

[Mark Brown]

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

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.

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

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

Re: [PATCH 4/9] ASoC: wm_hubs: Add missing break in hp_supply_event()

[Mark Brown]

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

On Wed, Oct 30, 2013 at 08:35:02AM +0100, Takashi Iwai wrote:
> Spotted by coverity CID 115170.

Applied, thanks.

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

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

Re: [PATCH 5/9] ASoC: wm0010: Fix possible out-of-bounds array read

[Mark Brown]

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

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.

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

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

Re: [PATCH 6/9] ASoC: max98095: Add missing negative channel checks

[Mark Brown]

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

On Wed, Oct 30, 2013 at 08:35:04AM +0100, Takashi Iwai wrote:

> +	if (channel < 0)
> +		return channel;
>  	BUG_ON(channel > 1);

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.

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

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

Re: [PATCH 7/9] ASoC: ml26124: Fix negative array index read

[Mark Brown]

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

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).

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

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

Re: [PATCH 8/9] ASoC: rt5640: Fix ignored error checks

[Mark Brown]

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

On Wed, Oct 30, 2013 at 08:35:06AM +0100, Takashi Iwai wrote:
> The negative error value returned from get_sdp_info() is ignored
> because it's assigned to unsigned variables.

Applied, thanks.

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

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

Re: [PATCH 9/9] ASoC: wm8996: Fix negative array index read

[Mark Brown]

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

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;
>  	ucontrol->value.enumerated.item[0] = wm8996->retune_mobile_cfg[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.

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

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

Re: [PATCH 5/9] ASoC: wm0010: Fix possible out-of-bounds array read

[Takashi Iwai]

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

Re: [PATCH 9/9] ASoC: wm8996: Fix negative array index read

[Takashi Iwai]

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;
> >  	ucontrol->value.enumerated.item[0] = wm8996->retune_mobile_cfg[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

Re: [PATCH 6/9] ASoC: max98095: Add missing negative channel checks

[Takashi Iwai]

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;
> >  	BUG_ON(channel > 1);
> 
> 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

Re: [PATCH 7/9] ASoC: ml26124: Fix negative array index read

[Takashi Iwai]

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);
 
-- 
1.8.4.1

Re: [PATCH 6/9] ASoC: max98095: Add missing negative channel checks

[Mark Brown]

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

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;
> > >  	BUG_ON(channel > 1);

> > 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.

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

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

Re: [PATCH 5/9] ASoC: wm0010: Fix possible out-of-bounds array read

[Mark Brown]

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

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.

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

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

Re: [PATCH 7/9] ASoC: ml26124: Fix negative array index read

[Mark Brown]

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

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.

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

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

Re: [PATCH 6/9] ASoC: max98095: Add missing negative channel checks

[Takashi Iwai]

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;
> > > >  	BUG_ON(channel > 1);
> 
> > > 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