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?
186 }
regards,
dan carpenter
