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

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

On Thu, Nov 20, 2014 at 12:31:48PM +0100, Jan Beulich wrote:
> >>> On 19.11.14 at 20:46, <donald.d.dugger@intel.com> 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@n0ano.com
Ph: 303/443-3786