RE: [PATCH v5] Quirk for IVB graphics FLR errata

["Hao, Xudong" <xudong.hao@intel.com>]

> -----Original Message-----
> From: Matthew Wilcox [mailto:matthew@wil.cx]
> Sent: Sunday, April 15, 2012 4:41 AM
> To: Hao, Xudong
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org; Don Dutile
> Subject: Re: [PATCH v5] Quirk for IVB graphics FLR errata
> 
> On Fri, Apr 13, 2012 at 08:22:18AM +0000, Hao, Xudong wrote:
> > Changes from v4: (based on Matthew and Don's comments)
> > - using jiffies to set timeout instead of tsc.
> > - correct type mmio_base variable
> > - using readl() and writel() to access io.
> > - correct code style, use spaces around the multiply operator.
> 
> Much better!  Just a couple of minor nits ...
> 
> > +	val = readl(mmio_base + PCH_PP_CONTROL) & 0xfffffffe;
> > +	writel(val, mmio_base + PCH_PP_CONTROL);
> > +	do {
> > +		timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
> > +		while (1) {
> 
> Personally, I'd write this as:
> 
> 	while (time_before(jiffies, timeout)) {
> 		...
> 	}
> 
> > +			val = readl(mmio_base + PCH_PP_STATUS);
> > +			if (((val & 0x80000000) == 0)
> > +				&& ((val & 0x30000000) == 0))
> 
> Is there a reason why this shouldn't be:
> 
> 		if ((val & 0xB0000000) == 0)
> 
> > +				break;
> > +			if (time_after(jiffies, timeout))
> > +				break;
> > +			cpu_relax();
> > +		}
> > +	} while (0);
> 
> Why do we need this to be in a do { } while (0) loop?
> 
> Putting those three suggestions together, I think it should look like this:
> 
> 	writel(val, mmio_base + PCH_PP_CONTROL);
> 	timeout = jiffies + msecs_to_jiffies(IGD_OPERATION_TIMEOUT);
> 	while (time_before(jiffies, timeout)) {
> 		val = readl(mmio_base + PCH_PP_STATUS);
> 		if ((val & 0xB0000000) == 0)
> 			break;
> 		cpu_relax();
> 	}
> 	writel(0x00000002, mmio_base + 0xd0100);
> 

The code looks simple and reasonable, I'll modify with changes and add your name in Signed-off-by.

> With that change, please add:
> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> --
> Matthew Wilcox				Intel Open Source Technology Centre
> "Bill, look, we understand that you're interested in selling us this operating
> system, but compare it to ours.  We can't possibly take such a retrograde
> step."