[PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Axel Lin]

The callers use the return value of max98088_get_channel as array index to
access max98088->dai[] array.
Add BUG() assertion for out of bound access of max98088->dai[] array.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 sound/soc/codecs/max98088.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
index ac65a2d..aaca91c 100644
--- a/sound/soc/codecs/max98088.c
+++ b/sound/soc/codecs/max98088.c
@@ -1703,6 +1703,7 @@ static int max98088_get_channel(const char *name)
                return 0;
        if (strcmp(name, "EQ2 Mode") == 0)
                return 1;
+	BUG();
        return -EINVAL;
 }
 
-- 
1.7.4.1

[PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Axel Lin]

The callers use the return value of max98095_get_bq_channel as array index to
access max98095->dai[] array.
Add BUG() assertion for out of bound access of max98095->dai[] array.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 sound/soc/codecs/max98095.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
index 668434d..973c02d 100644
--- a/sound/soc/codecs/max98095.c
+++ b/sound/soc/codecs/max98095.c
@@ -1998,6 +1998,7 @@ static int max98095_get_bq_channel(const char *name)
 		return 0;
 	if (strcmp(name, "Biquad2 Mode") == 0)
 		return 1;
+	BUG();
 	return -EINVAL;
 }
 
-- 
1.7.4.1

Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 00:01, Axel Lin wrote:
> The callers use the return value of max98088_get_channel as array index to
> access max98088->dai[] array.
> Add BUG() assertion for out of bound access of max98088->dai[] array.

BUG() is pretty heavy handed for a driver. Why not fix the problem
properly in the callers?

----
Check the return value of max98088_get_channel in the callers and
propagate any errors up.

Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---

diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
index ac65a2d..e07700e 100644
--- a/sound/soc/codecs/max98088.c
+++ b/sound/soc/codecs/max98088.c
@@ -1811,6 +1811,9 @@ static int max98088_put_eq_enum(struct snd_kcontrol *kcontrol,
        struct max98088_cdata *cdata;
        int sel = ucontrol->value.integer.value[0];
 
+       if (channel < 0)
+	       return channel;
+
        cdata = &max98088->dai[channel];
 
        if (sel >= pdata->eq_cfgcnt)
@@ -1838,6 +1841,9 @@ static int max98088_get_eq_enum(struct snd_kcontrol *kcontrol,
        int channel = max98088_get_channel(kcontrol->id.name);
        struct max98088_cdata *cdata;
 
+       if (channel < 0)
+	       return channel;
+
        cdata = &max98088->dai[channel];
        ucontrol->value.enumerated.item[0] = cdata->eq_sel;
        return 0;

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 00:02, Axel Lin wrote:
> The callers use the return value of max98095_get_bq_channel as array index to
> access max98095->dai[] array.
> Add BUG() assertion for out of bound access of max98095->dai[] array.

Same here, fix the problem in the callers.

----
Check the return value of max98095_get_bq_channel in the callers and
propagate any errors up. Remove the BUG_ON(channel > 1) since
max98095_get_bq_channel never returns a value larger than 1.

Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---

diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
index 668434d..55eccea 100644
--- a/sound/soc/codecs/max98095.c
+++ b/sound/soc/codecs/max98095.c
@@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
 	int fs, best, best_val, i;
 	int regmask, regsave;
 
-	BUG_ON(channel > 1);
+	if (channel < 0)
+		return channel;
 
 	if (!pdata || !max98095->bq_textcnt)
 		return 0;
@@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
 	int channel = max98095_get_bq_channel(kcontrol->id.name);
 	struct max98095_cdata *cdata;
 
+	if (channel < 0)
+		return channel;
+
 	cdata = &max98095->dai[channel];
 	ucontrol->value.enumerated.item[0] = cdata->bq_sel;
 

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Dave Young]

On Wed, Sep 28, 2011 at 10:02 PM, Axel Lin <axel.lin@gmail.com> wrote:
> The callers use the return value of max98095_get_bq_channel as array index to
> access max98095->dai[] array.
> Add BUG() assertion for out of bound access of max98095->dai[] array.
>
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
>  sound/soc/codecs/max98095.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
> index 668434d..973c02d 100644
> --- a/sound/soc/codecs/max98095.c
> +++ b/sound/soc/codecs/max98095.c
> @@ -1998,6 +1998,7 @@ static int max98095_get_bq_channel(const char *name)
>                return 0;
>        if (strcmp(name, "Biquad2 Mode") == 0)
>                return 1;
> +       BUG();
>        return -EINVAL;

below better?
BUG_ON(strcmp(name, "Biquad2 Mode"));
return 1;

Or BUG_ON(channel < 0) in caller

>  }
>
> --
> 1.7.4.1
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Regards
Dave

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Dave Young]

On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 29/09/11 00:02, Axel Lin wrote:
>> The callers use the return value of max98095_get_bq_channel as array index to
>> access max98095->dai[] array.
>> Add BUG() assertion for out of bound access of max98095->dai[] array.
>
> Same here, fix the problem in the callers.
>
> ----
> Check the return value of max98095_get_bq_channel in the callers and
> propagate any errors up. Remove the BUG_ON(channel > 1) since
> max98095_get_bq_channel never returns a value larger than 1.
>
> Signed-off-by: Ryan Mallon <rmallon@gmail.com>
> ---
>
> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
> index 668434d..55eccea 100644
> --- a/sound/soc/codecs/max98095.c
> +++ b/sound/soc/codecs/max98095.c
> @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
>        int fs, best, best_val, i;
>        int regmask, regsave;
>
> -       BUG_ON(channel > 1);
> +       if (channel < 0)
> +               return channel;

If use BUG() happens in  max98095_get_bq_channel, it will not return here?

>
>        if (!pdata || !max98095->bq_textcnt)
>                return 0;
> @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
>        int channel = max98095_get_bq_channel(kcontrol->id.name);
>        struct max98095_cdata *cdata;
>
> +       if (channel < 0)
> +               return channel;
> +
>        cdata = &max98095->dai[channel];
>        ucontrol->value.enumerated.item[0] = cdata->bq_sel;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Regards
Dave

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 11:35, Dave Young wrote:

> On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 29/09/11 00:02, Axel Lin wrote:
>>> The callers use the return value of max98095_get_bq_channel as array index to
>>> access max98095->dai[] array.
>>> Add BUG() assertion for out of bound access of max98095->dai[] array.
>>
>> Same here, fix the problem in the callers.
>>
>> ----
>> Check the return value of max98095_get_bq_channel in the callers and
>> propagate any errors up. Remove the BUG_ON(channel > 1) since
>> max98095_get_bq_channel never returns a value larger than 1.
>>
>> Signed-off-by: Ryan Mallon <rmallon@gmail.com>
>> ---
>>
>> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
>> index 668434d..55eccea 100644
>> --- a/sound/soc/codecs/max98095.c
>> +++ b/sound/soc/codecs/max98095.c
>> @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
>>        int fs, best, best_val, i;
>>        int regmask, regsave;
>>
>> -       BUG_ON(channel > 1);
>> +       if (channel < 0)
>> +               return channel;
> 
> If use BUG() happens in  max98095_get_bq_channel, it will not return here?


Not quite sure what you mean?

If CONFIG_BUG was not enabled for the original version, then it would
not return at the BUG_ON and would either crash or cause odd behaviour
if it tried to index channel as -1.

My patch is removing the BUG_ON and replacing it with a proper check and
return. It doesn't need to check > 1 since max98095_get_bq_channel never
returns that.

My understanding is that device drivers, in general, should not call
BUG. BUG is for unrecoverable errors which leave the kernel in some
unstable state. Here we can just return an error code.

~Ryan

> 
>>
>>        if (!pdata || !max98095->bq_textcnt)
>>                return 0;
>> @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
>>        int channel = max98095_get_bq_channel(kcontrol->id.name);
>>        struct max98095_cdata *cdata;
>>
>> +       if (channel < 0)
>> +               return channel;
>> +
>>        cdata = &max98095->dai[channel];
>>        ucontrol->value.enumerated.item[0] = cdata->bq_sel;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 
> 
> 

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Dave Young]

On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 29/09/11 11:35, Dave Young wrote:
>
>> On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>>> On 29/09/11 00:02, Axel Lin wrote:
>>>> The callers use the return value of max98095_get_bq_channel as array index to
>>>> access max98095->dai[] array.
>>>> Add BUG() assertion for out of bound access of max98095->dai[] array.
>>>
>>> Same here, fix the problem in the callers.
>>>
>>> ----
>>> Check the return value of max98095_get_bq_channel in the callers and
>>> propagate any errors up. Remove the BUG_ON(channel > 1) since
>>> max98095_get_bq_channel never returns a value larger than 1.
>>>
>>> Signed-off-by: Ryan Mallon <rmallon@gmail.com>
>>> ---
>>>
>>> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
>>> index 668434d..55eccea 100644
>>> --- a/sound/soc/codecs/max98095.c
>>> +++ b/sound/soc/codecs/max98095.c
>>> @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
>>>        int fs, best, best_val, i;
>>>        int regmask, regsave;
>>>
>>> -       BUG_ON(channel > 1);
>>> +       if (channel < 0)
>>> +               return channel;
>>
>> If use BUG() happens in  max98095_get_bq_channel, it will not return here?
>
>
> Not quite sure what you mean?

I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will
panic firstly the if condition will be never entered.

>
> If CONFIG_BUG was not enabled for the original version, then it would
> not return at the BUG_ON and would either crash or cause odd behaviour
> if it tried to index channel as -1.
>
> My patch is removing the BUG_ON and replacing it with a proper check and
> return. It doesn't need to check > 1 since max98095_get_bq_channel never
> returns that.
>
> My understanding is that device drivers, in general, should not call
> BUG. BUG is for unrecoverable errors which leave the kernel in some
> unstable state. Here we can just return an error code.

Agree

>
> ~Ryan
>
>>
>>>
>>>        if (!pdata || !max98095->bq_textcnt)
>>>                return 0;
>>> @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
>>>        int channel = max98095_get_bq_channel(kcontrol->id.name);
>>>        struct max98095_cdata *cdata;
>>>
>>> +       if (channel < 0)
>>> +               return channel;
>>> +
>>>        cdata = &max98095->dai[channel];
>>>        ucontrol->value.enumerated.item[0] = cdata->bq_sel;
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>
>>
>>
>
>
>



-- 
Regards
Dave

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 11:59, Dave Young wrote:

> On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 29/09/11 11:35, Dave Young wrote:
>>
>>> On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>>>> On 29/09/11 00:02, Axel Lin wrote:
>>>>> The callers use the return value of max98095_get_bq_channel as array index to
>>>>> access max98095->dai[] array.
>>>>> Add BUG() assertion for out of bound access of max98095->dai[] array.
>>>>
>>>> Same here, fix the problem in the callers.
>>>>
>>>> ----
>>>> Check the return value of max98095_get_bq_channel in the callers and
>>>> propagate any errors up. Remove the BUG_ON(channel > 1) since
>>>> max98095_get_bq_channel never returns a value larger than 1.
>>>>
>>>> Signed-off-by: Ryan Mallon <rmallon@gmail.com>
>>>> ---
>>>>
>>>> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
>>>> index 668434d..55eccea 100644
>>>> --- a/sound/soc/codecs/max98095.c
>>>> +++ b/sound/soc/codecs/max98095.c
>>>> @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
>>>>        int fs, best, best_val, i;
>>>>        int regmask, regsave;
>>>>
>>>> -       BUG_ON(channel > 1);
>>>> +       if (channel < 0)
>>>> +               return channel;
>>>
>>> If use BUG() happens in  max98095_get_bq_channel, it will not return here?
>>
>>
>> Not quite sure what you mean?
> 
> I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will
> panic firstly the if condition will be never entered.

My patch is a replacement for Axel's patch, not on top of it. For Axel's
patch it would panic if channel was less than zero if CONFIG_BUG was
enabled, but would still have undefined behaviour if CONFIG_BUG was not
enabled.

~Ryan

>>
>> If CONFIG_BUG was not enabled for the original version, then it would
>> not return at the BUG_ON and would either crash or cause odd behaviour
>> if it tried to index channel as -1.
>>
>> My patch is removing the BUG_ON and replacing it with a proper check and
>> return. It doesn't need to check > 1 since max98095_get_bq_channel never
>> returns that.
>>
>> My understanding is that device drivers, in general, should not call
>> BUG. BUG is for unrecoverable errors which leave the kernel in some
>> unstable state. Here we can just return an error code.
> 
> Agree
> 
>>
>> ~Ryan
>>
>>>
>>>>
>>>>        if (!pdata || !max98095->bq_textcnt)
>>>>                return 0;
>>>> @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
>>>>        int channel = max98095_get_bq_channel(kcontrol->id.name);
>>>>        struct max98095_cdata *cdata;
>>>>
>>>> +       if (channel < 0)
>>>> +               return channel;
>>>> +
>>>>        cdata = &max98095->dai[channel];
>>>>        ucontrol->value.enumerated.item[0] = cdata->bq_sel;
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 

Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL

[Dave Young]

On Thu, Sep 29, 2011 at 10:01 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 29/09/11 11:59, Dave Young wrote:
>
>> On Thu, Sep 29, 2011 at 9:52 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>>> On 29/09/11 11:35, Dave Young wrote:
>>>
>>>> On Thu, Sep 29, 2011 at 7:19 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>>>>> On 29/09/11 00:02, Axel Lin wrote:
>>>>>> The callers use the return value of max98095_get_bq_channel as array index to
>>>>>> access max98095->dai[] array.
>>>>>> Add BUG() assertion for out of bound access of max98095->dai[] array.
>>>>>
>>>>> Same here, fix the problem in the callers.
>>>>>
>>>>> ----
>>>>> Check the return value of max98095_get_bq_channel in the callers and
>>>>> propagate any errors up. Remove the BUG_ON(channel > 1) since
>>>>> max98095_get_bq_channel never returns a value larger than 1.
>>>>>
>>>>> Signed-off-by: Ryan Mallon <rmallon@gmail.com>
>>>>> ---
>>>>>
>>>>> diff --git a/sound/soc/codecs/max98095.c b/sound/soc/codecs/max98095.c
>>>>> index 668434d..55eccea 100644
>>>>> --- a/sound/soc/codecs/max98095.c
>>>>> +++ b/sound/soc/codecs/max98095.c
>>>>> @@ -2014,7 +2014,8 @@ static int max98095_put_bq_enum(struct snd_kcontrol *kcontrol,
>>>>>        int fs, best, best_val, i;
>>>>>        int regmask, regsave;
>>>>>
>>>>> -       BUG_ON(channel > 1);
>>>>> +       if (channel < 0)
>>>>> +               return channel;
>>>>
>>>> If use BUG() happens in  max98095_get_bq_channel, it will not return here?
>>>
>>>
>>> Not quite sure what you mean?
>>
>> I means if Axel Lin's patch applied, and CONFIG_BUG is on, it will
>> panic firstly the if condition will be never entered.
>
> My patch is a replacement for Axel's patch, not on top of it. For Axel's
> patch it would panic if channel was less than zero if CONFIG_BUG was
> enabled, but would still have undefined behaviour if CONFIG_BUG was not
> enabled.

So that's good, thanks

>
> ~Ryan
>
>>>
>>> If CONFIG_BUG was not enabled for the original version, then it would
>>> not return at the BUG_ON and would either crash or cause odd behaviour
>>> if it tried to index channel as -1.
>>>
>>> My patch is removing the BUG_ON and replacing it with a proper check and
>>> return. It doesn't need to check > 1 since max98095_get_bq_channel never
>>> returns that.
>>>
>>> My understanding is that device drivers, in general, should not call
>>> BUG. BUG is for unrecoverable errors which leave the kernel in some
>>> unstable state. Here we can just return an error code.
>>
>> Agree
>>
>>>
>>> ~Ryan
>>>
>>>>
>>>>>
>>>>>        if (!pdata || !max98095->bq_textcnt)
>>>>>                return 0;
>>>>> @@ -2069,6 +2070,9 @@ static int max98095_get_bq_enum(struct snd_kcontrol *kcontrol,
>>>>>        int channel = max98095_get_bq_channel(kcontrol->id.name);
>>>>>        struct max98095_cdata *cdata;
>>>>>
>>>>> +       if (channel < 0)
>>>>> +               return channel;
>>>>> +
>>>>>        cdata = &max98095->dai[channel];
>>>>>        ucontrol->value.enumerated.item[0] = cdata->bq_sel;
>>>>>
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>



-- 
Regards
Dave

Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Mark Brown]

On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
> On 29/09/11 00:01, Axel Lin wrote:
> > The callers use the return value of max98088_get_channel as array index to
> > access max98088->dai[] array.
> > Add BUG() assertion for out of bound access of max98088->dai[] array.

> BUG() is pretty heavy handed for a driver. Why not fix the problem
> properly in the callers?

There's nothing constructive that any of the callers can do with an
error code - it's a clear bug in something (probably the driver) if we
get called for a bad control.  Simply returning an error code isn't
terribly helpful, it's very obscure what's gone wrong and why.  We at
least need a log message.

Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 20:34, Mark Brown wrote:
> On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
>> On 29/09/11 00:01, Axel Lin wrote:
>>> The callers use the return value of max98088_get_channel as array index to
>>> access max98088->dai[] array.
>>> Add BUG() assertion for out of bound access of max98088->dai[] array.
>> BUG() is pretty heavy handed for a driver. Why not fix the problem
>> properly in the callers?
> There's nothing constructive that any of the callers can do with an
> error code - it's a clear bug in something (probably the driver) if we
> get called for a bad control.  Simply returning an error code isn't
> terribly helpful, it's very obscure what's gone wrong and why.  We at
> least need a log message.

Yeah, it can basically only happen if there is a mismatch between the
kcontrol definition and the get_channel function in the driver. Would
you be happy with adding a:

  dev_err(codec->dev, "Bad kcontrol channel name\n");

and then returning the error? It doesn't seem worth panicking the whole
driver/system for a bug like this.

~Ryan

Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Ryan Mallon]

On 29/09/11 20:34, Mark Brown wrote:
> On Thu, Sep 29, 2011 at 09:15:03AM +1000, Ryan Mallon wrote:
>> On 29/09/11 00:01, Axel Lin wrote:
>>> The callers use the return value of max98088_get_channel as array index to
>>> access max98088->dai[] array.
>>> Add BUG() assertion for out of bound access of max98088->dai[] array.
>> BUG() is pretty heavy handed for a driver. Why not fix the problem
>> properly in the callers?
> There's nothing constructive that any of the callers can do with an
> error code - it's a clear bug in something (probably the driver) if we
> get called for a bad control.  Simply returning an error code isn't
> terribly helpful, it's very obscure what's gone wrong and why.  We at
> least need a log message.

How about something like this (untested) patch?
---
Make the max98088 codec controls[] array a static global and iterate
over it in max98088_get_channel rather than duplicating the hardcoded
strings. Add a warning if the channel name is not found and check for
the error value in the callers.

Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---

diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c
index ac65a2d..bc2b922 100644
--- a/sound/soc/codecs/max98088.c
+++ b/sound/soc/codecs/max98088.c
@@ -1697,13 +1697,28 @@ static struct snd_soc_dai_driver max98088_dai[] = {
 }
 };
 
-static int max98088_get_channel(const char *name)
+static struct snd_kcontrol_new controls[] = {
+	SOC_ENUM_EXT("EQ1 Mode",
+		     max98088->eq_enum,
+		     max98088_get_eq_enum,
+		     max98088_put_eq_enum),
+	SOC_ENUM_EXT("EQ2 Mode",
+		     max98088->eq_enum,
+		     max98088_get_eq_enum,
+		     max98088_put_eq_enum),
+};
+
+static int max98088_get_channel(struct snd_soc_codec *codec, const char *name)
 {
-       if (strcmp(name, "EQ1 Mode") == 0)
-               return 0;
-       if (strcmp(name, "EQ2 Mode") == 0)
-               return 1;
-       return -EINVAL;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(controls); i++)
+		if (strcmp(name, controls[i].name) == 0)
+			return i;
+
+	/* Shouldn't happen */
+	dev_err(codec->dev, "Bad channel name '%s'\n", name);
+	return -EINVAL;
 }
 
 static void max98088_setup_eq1(struct snd_soc_codec *codec)
@@ -1807,10 +1822,13 @@ static int max98088_put_eq_enum(struct snd_kcontrol *kcontrol,
        struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
        struct max98088_priv *max98088 = snd_soc_codec_get_drvdata(codec);
        struct max98088_pdata *pdata = max98088->pdata;
-       int channel = max98088_get_channel(kcontrol->id.name);
+       int channel = max98088_get_channel(codec, kcontrol->id.name);
        struct max98088_cdata *cdata;
        int sel = ucontrol->value.integer.value[0];
 
+       if (channel < 0)
+	       return channel;
+
        cdata = &max98088->dai[channel];
 
        if (sel >= pdata->eq_cfgcnt)
@@ -1835,9 +1853,12 @@ static int max98088_get_eq_enum(struct snd_kcontrol *kcontrol,
 {
        struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
        struct max98088_priv *max98088 = snd_soc_codec_get_drvdata(codec);
-       int channel = max98088_get_channel(kcontrol->id.name);
+       int channel = max98088_get_channel(codec, kcontrol->id.name);
        struct max98088_cdata *cdata;
 
+       if (channel < 0)
+	       return channel;
+
        cdata = &max98088->dai[channel];
        ucontrol->value.enumerated.item[0] = cdata->eq_sel;
        return 0;
@@ -1853,17 +1874,6 @@ static void max98088_handle_eq_pdata(struct snd_soc_codec *codec)
        const char **t;
        int ret;
 
-       struct snd_kcontrol_new controls[] = {
-               SOC_ENUM_EXT("EQ1 Mode",
-                       max98088->eq_enum,
-                       max98088_get_eq_enum,
-                       max98088_put_eq_enum),
-               SOC_ENUM_EXT("EQ2 Mode",
-                       max98088->eq_enum,
-                       max98088_get_eq_enum,
-                       max98088_put_eq_enum),
-       };
-
        cfg = pdata->eq_cfg;
        cfgcnt = pdata->eq_cfgcnt;
 

Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL

[Mark Brown]

On Fri, Sep 30, 2011 at 09:13:42AM +1000, Ryan Mallon wrote:

> How about something like this (untested) patch?

Looks plausible.