lkml.org 
[lkml]   [2017]   [Apr]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] ASoC: codec: wm9860: avoid maybe-uninitialized warning
On Thu, Apr 20, 2017 at 10:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Apr 20, 2017 at 8:48 AM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>> Hi Arnd,
>>
>> On Wed, Apr 19, 2017 at 8:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> The new PLL configuration code triggers a harmless warning:
>>>
>>> sound/soc/codecs/wm8960.c: In function 'wm8960_configure_clocking':
>>> sound/soc/codecs/wm8960.c:735:3: error: 'best_freq_out' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>> wm8960_set_pll(codec, freq_in, best_freq_out);
>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> sound/soc/codecs/wm8960.c:699:12: note: 'best_freq_out' was declared here
>>>
>>> I think the warning was introduced by Daniel's bugfix. I've come up
>>> with a way to simplify the code in a way that is more readable to
>>> both humans and to gcc, which gets us rid of the warning.
>>
>> I've struggled with this kind of warning when reworking wm8960
>> bitclock computation.
>>
>> Anyhow, for this exact patch the warning didn't showed up.
>>
>> I used:
>>
>> gcc version 6.2.1 20161016 (Linaro GCC 6.2-2016.11)
>
> I'm using gcc-7.0.1 here, which overall has better warnings for
> -Wmaybe-uninitialized
> than older versions, but sometimes also finds new false positives.
>
>> My next patch:
>>
>> https://patchwork.kernel.org/patch/9666921/ [1]
>>
>> which is under review, had the issue you mention (in v1)
>>
>> https://lkml.org/lkml/2017/4/5/246
>>
>> but I initialized best_freq_out with 0 to get rid of the problem.
>
> I try hard not to do that, because it can hide real problems in case the
> code gets modified further to actually have an uninitialized use that we
> would otherwise get a warning for.
>
>>> @@ -720,22 +718,14 @@ int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in,
>>> *sysclk_idx = i;
>>> *dac_idx = j;
>>> *bclk_idx = k;
>>> - best_freq_out = freq_out;
>>> - break;
>>> + return freq_out;
>>> }
>>> }
>>> - if (k != ARRAY_SIZE(bclk_divs))
>>> - break;
>>> }
>>> - if (j != ARRAY_SIZE(dac_divs))
>>> - break;
>>> }
>>> -
>>> - if (*bclk_idx != -1)
>>> - wm8960_set_pll(codec, freq_in, best_freq_out);
>>
>> I think the compiler is tricked into thinking that best_freq_out might
>> be uninitialized. Notice
>> that each time *bclk_idx is assigned a value (which is always >=0) we
>> also initialize best_freq_out.
>
> Right. This is probably the result of one of two things that
> prevent the compiler from figuring it out:
>
> a) bclk_idx being an indirect pointer might cause the compiler to
> decide that another operation might overwrite it, e.g. if it points
> to the same location as one of the other pointers.
>
> b) The flow analysis might just get too tricky, so when gcc gives
> up trying to work out whether the assignment has happened
> at least once for the two variables, it concludes that it doesn't
> know, without seeing that the answer is always the same for
> both of them.
>
>>> @@ -783,11 +773,12 @@ static int wm8960_configure_clocking(struct snd_soc_codec *codec)
>>> }
>>> }
>>>
>>> - ret = wm8960_configure_pll(codec, freq_in, &i, &j, &k);
>>> - if (ret < 0) {
>>> + freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k);
>>> + if (freq_out < 0) {
>>> dev_err(codec->dev, "failed to configure clock via PLL\n");
>>> - return -EINVAL;
>>> + return freq_out;
>>> }
>>> + wm8960_set_pll(codec, freq_in, freq_out);
>>>
>>> configure_clock:
>>> /* configure sysclk clock */
>>
>> Your idea looks good, will need to rework my follow up patch on it:
>>
>> https://patchwork.kernel.org/patch/9666921/
>
> Ok. Or feel free to fold my patch into yours if that makes it easier for you.

Great! I will fold your patch into my changes.

thanks Arnd!

Daniel.

\
 
 \ /
  Last update: 2017-04-20 10:22    [W:0.096 / U:5.372 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site