On Fri, 2022-05-13 at 17:05 +0300, Dan Carpenter wrote:
> On Fri, May 13, 2022 at 03:17:24PM +0200, Giovanni Gherdovich wrote:
> > On Thu, 2022-05-05 at 10:05 +0300, Dan Carpenter wrote:
> > > Hello Giovanni Gherdovich,
> > >
> > > The patch 51beea8862a3: "x86, sched: Bail out of frequency invariance
> > > if turbo frequency is unknown" from May 31, 2020, leads to the
> > > following Smatch static checker warning:
> > >
> > > arch/x86/kernel/cpu/aperfmperf.c:274 intel_set_max_freq_ratio()
> > > error: uninitialized symbol 'turbo_freq'.
> > >
> > > arch/x86/kernel/cpu/aperfmperf.c
> > > 242 static bool __init intel_set_max_freq_ratio(void)
> > > 243 {
> > > 244 u64 base_freq, turbo_freq;
> > > 245 u64 turbo_ratio;
> > > 246
> > > 247 if (slv_set_max_freq_ratio(&base_freq, &turbo_freq))
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > Imagine this fails.
> > >
> > > 248 goto out;
> >
> > I think this is a false positive of the static checker; I'll send the patch
> > to initialize the variables anyway as it looks better.
> >
> > When slv_set_max_freq_ratio() fails, it returns false, and we move on to the
> > next "if" statement. "goto out" is executed only if slv_set_max_freq_ratio()
> > succeeds (returns true), and writes data in its input parameters.
> >
> > I give you that the code looks wrong, because apparently when people (and
> > static analyzers) read that, they think "if (problem) goto error;" but this
> > function is written like "if (it_works) goto the_end;".
>
> I read this wrong. But looking at it closer, I think the static analyzer is
> actually correct (definitely correct-ish for sure). Smatch is complaining about
> knl_set_max_freq_ratio().
>
> 149 static bool __init knl_set_max_freq_ratio(u64 *base_freq, u64 *turbo_freq,
> 150 int num_delta_fratio)
> 151 {
> 152 int fratio, delta_fratio, found;
> 153 int err, i;
> 154 u64 msr;
> 155
> 156 err = rdmsrl_safe(MSR_PLATFORM_INFO, base_freq);
> 157 if (err)
> 158 return false;
> 159
> 160 *base_freq = (*base_freq >> 8) & 0xFF; /* max P state */
> 161
> 162 err = rdmsrl_safe(MSR_TURBO_RATIO_LIMIT, &msr);
> 163 if (err)
> 164 return false;
> 165
> 166 fratio = (msr >> 8) & 0xFF;
> 167 i = 16;
> 168 found = 0;
> 169 do {
> 170 if (found >= num_delta_fratio) {
> 171 *turbo_freq = fratio;
> 172 return true;
> 173 }
> 174
> 175 delta_fratio = (msr >> (i + 5)) & 0x7;
> 176
> 177 if (delta_fratio) {
> 178 found += 1;
> 179 fratio -= delta_fratio;
> 180 }
> 181
> 182 i += 8;
> 183 } while (i < 64);
> 184
> 185 return true;
>
> If we reach this "return true" then turbo_freq is not set. This may not be
> reachable in real life. Should it be return false?
>
That's right, it's a bug. It should return false at the end.
I'll send a fix, thanks.
Giovanni
