Re: [Xen-devel] [PATCH V3] Decouple SnadyBridge quirk form VTd timeout

["Donald D. Dugger" <donald.d.dugger@xxxxxxxxx>]

On Thu, Nov 20, 2014 at 12:31:48PM +0100, Jan Beulich wrote:
> >>> On 19.11.14 at 20:46, <donald.d.dugger@xxxxxxxxx> wrote:
> > @@ -237,6 +248,42 @@
> >      }
> >  }
> >  
> > +static void __init parse_snb_timeout(const char *s)
> > +{
> > +   int not;
> > +
> > +   switch (*s) {
> > +
> > +   case '\0':
> > +           snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> > +           break;
> > +
> > +   case '0':       case '1':       case '2':
> > +   case '3':       case '4':       case '5':
> > +   case '6':       case '7':       case '8':
> > +   case '9':
> > +           snb_igd_timeout = MILLISECS(simple_strtoul(s, &s, 0));
> > +           if ( snb_igd_timeout == MILLISECS(1) )
> > +                   snb_igd_timeout = SNB_IGD_TIMEOUT_LEGACY;
> > +           break;
> 
> Overly complicated. Just parse_bool() first, if that returns negative
> check for "default" or "", and (if not matched) invoke strtoul(). No
> need for this switch statement.

Personally, I prefer switch statements whenever possible, they're linear
(if statements are evil) and the compiler optimizes them well.  Anyway,
NP, I'll follow your suggestion.

> 
> > +
> > +   default:
> > +           if ( strncmp("default", s, 7) == 0 ) {
> > +                   snb_igd_timeout = SNB_IGD_TIMEOUT;
> > +                   break;
> > +           }
> > +           not = !strncmp("no-", s, 3);
> 
> This makes no sense - you're looking for e.g. "snb_igd_quirk=no-no"
> here. If the use specified "no-snb_igd_quirk", you'll end up seeing
> "=no" when this function gets entered.

I was confused about the `no-' prefix, I thought it applied to the
value when it is really applied to the key (which makes a lot more
sense).  Fixed in next version.

> 
> Also the whole function is white space damaged (using hard tabs)
> and has misplaced opening braces.

Forgot Xen doesn't like tabs, my bad.

> 
> Jan
> 
> 

And, as Ian pointed out, dyslexia in the subject line, also fixed in the
next version.

-- 
Don Dugger
"Censeo Toto nos in Kansa esse decisse." - D. Gale
n0ano@xxxxxxxxx
Ph: 303/443-3786

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel