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

[Ryan Mallon <rmallon@gmail.com>]

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