Re: [bug report] x86, sched: Bail out of frequency invariance if turbo frequency is unknown

[Dan Carpenter <dan.carpenter@oracle.com>]

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