* [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL @ 2011-09-28 14:01 Axel Lin 2011-09-28 14:02 ` [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel " Axel Lin 2011-09-28 23:15 ` [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel " Ryan Mallon 0 siblings, 2 replies; 14+ messages in thread From: Axel Lin @ 2011-09-28 14:01 UTC (permalink / raw) To: linux-kernel Cc: Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-28 14:01 [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL Axel Lin @ 2011-09-28 14:02 ` Axel Lin 2011-09-28 23:19 ` Ryan Mallon 2011-09-29 1:33 ` Dave Young 2011-09-28 23:15 ` [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel " Ryan Mallon 1 sibling, 2 replies; 14+ messages in thread From: Axel Lin @ 2011-09-28 14:02 UTC (permalink / raw) To: linux-kernel Cc: Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-28 14:02 ` [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel " Axel Lin @ 2011-09-28 23:19 ` Ryan Mallon 2011-09-29 1:35 ` Dave Young 2011-09-29 1:33 ` Dave Young 1 sibling, 1 reply; 14+ messages in thread From: Ryan Mallon @ 2011-09-28 23:19 UTC (permalink / raw) To: Axel Lin Cc: linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-28 23:19 ` Ryan Mallon @ 2011-09-29 1:35 ` Dave Young 2011-09-29 1:52 ` Ryan Mallon 0 siblings, 1 reply; 14+ messages in thread From: Dave Young @ 2011-09-29 1:35 UTC (permalink / raw) To: Ryan Mallon Cc: Axel Lin, linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-29 1:35 ` Dave Young @ 2011-09-29 1:52 ` Ryan Mallon 2011-09-29 1:59 ` Dave Young 0 siblings, 1 reply; 14+ messages in thread From: Ryan Mallon @ 2011-09-29 1:52 UTC (permalink / raw) To: Dave Young Cc: Axel Lin, linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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/ >> > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-29 1:52 ` Ryan Mallon @ 2011-09-29 1:59 ` Dave Young 2011-09-29 2:01 ` Ryan Mallon 0 siblings, 1 reply; 14+ messages in thread From: Dave Young @ 2011-09-29 1:59 UTC (permalink / raw) To: Ryan Mallon Cc: Axel Lin, linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-29 1:59 ` Dave Young @ 2011-09-29 2:01 ` Ryan Mallon 2011-09-29 2:06 ` Dave Young 0 siblings, 1 reply; 14+ messages in thread From: Ryan Mallon @ 2011-09-29 2:01 UTC (permalink / raw) To: Dave Young Cc: Axel Lin, linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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/ >>>> >>> >>> >>> >> >> >> > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-29 2:01 ` Ryan Mallon @ 2011-09-29 2:06 ` Dave Young 0 siblings, 0 replies; 14+ messages in thread From: Dave Young @ 2011-09-29 2:06 UTC (permalink / raw) To: Ryan Mallon Cc: Axel Lin, linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel returns -EINVAL 2011-09-28 14:02 ` [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel " Axel Lin 2011-09-28 23:19 ` Ryan Mallon @ 2011-09-29 1:33 ` Dave Young 1 sibling, 0 replies; 14+ messages in thread From: Dave Young @ 2011-09-29 1:33 UTC (permalink / raw) To: Axel Lin Cc: linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL 2011-09-28 14:01 [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL Axel Lin 2011-09-28 14:02 ` [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel " Axel Lin @ 2011-09-28 23:15 ` Ryan Mallon 2011-09-29 10:34 ` Mark Brown 1 sibling, 1 reply; 14+ messages in thread From: Ryan Mallon @ 2011-09-28 23:15 UTC (permalink / raw) To: Axel Lin Cc: linux-kernel, Liam Girdwood, Mark Brown, alsa-devel, Peter Hsiang, Jesse Marroquin 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL 2011-09-28 23:15 ` [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel " Ryan Mallon @ 2011-09-29 10:34 ` Mark Brown 2011-09-29 11:28 ` Ryan Mallon 2011-09-29 23:13 ` Ryan Mallon 0 siblings, 2 replies; 14+ messages in thread From: Mark Brown @ 2011-09-29 10:34 UTC (permalink / raw) To: Ryan Mallon Cc: Axel Lin, linux-kernel, Liam Girdwood, alsa-devel, Peter Hsiang, Jesse Marroquin 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL 2011-09-29 10:34 ` Mark Brown @ 2011-09-29 11:28 ` Ryan Mallon 2011-09-29 23:13 ` Ryan Mallon 1 sibling, 0 replies; 14+ messages in thread From: Ryan Mallon @ 2011-09-29 11:28 UTC (permalink / raw) To: Mark Brown Cc: Axel Lin, linux-kernel, Liam Girdwood, alsa-devel, Peter Hsiang, Jesse Marroquin 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL 2011-09-29 10:34 ` Mark Brown 2011-09-29 11:28 ` Ryan Mallon @ 2011-09-29 23:13 ` Ryan Mallon 2011-09-30 12:56 ` Mark Brown 1 sibling, 1 reply; 14+ messages in thread From: Ryan Mallon @ 2011-09-29 23:13 UTC (permalink / raw) To: Mark Brown Cc: Axel Lin, linux-kernel, Liam Girdwood, alsa-devel, Peter Hsiang, Jesse Marroquin 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; ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL 2011-09-29 23:13 ` Ryan Mallon @ 2011-09-30 12:56 ` Mark Brown 0 siblings, 0 replies; 14+ messages in thread From: Mark Brown @ 2011-09-30 12:56 UTC (permalink / raw) To: Ryan Mallon Cc: Axel Lin, linux-kernel, Liam Girdwood, alsa-devel, Peter Hsiang, Jesse Marroquin On Fri, Sep 30, 2011 at 09:13:42AM +1000, Ryan Mallon wrote: > How about something like this (untested) patch? Looks plausible. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-09-30 12:56 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-28 14:01 [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel returns -EINVAL Axel Lin 2011-09-28 14:02 ` [PATCH 2/2] ASoC: Add BUG() assertion if max98095_get_bq_channel " Axel Lin 2011-09-28 23:19 ` Ryan Mallon 2011-09-29 1:35 ` Dave Young 2011-09-29 1:52 ` Ryan Mallon 2011-09-29 1:59 ` Dave Young 2011-09-29 2:01 ` Ryan Mallon 2011-09-29 2:06 ` Dave Young 2011-09-29 1:33 ` Dave Young 2011-09-28 23:15 ` [PATCH 1/2] ASoC: Add BUG() assertion if max98088_get_channel " Ryan Mallon 2011-09-29 10:34 ` Mark Brown 2011-09-29 11:28 ` Ryan Mallon 2011-09-29 23:13 ` Ryan Mallon 2011-09-30 12:56 ` Mark Brown
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.