> -----Original Message----- > From: Matthew Wilcox [mailto:matthew@xxxxxx] > Sent: Sunday, April 15, 2012 4:41 AM > To: Hao, Xudong > Cc: Bjorn Helgaas; linux-pci@xxxxxxxxxxxxxxx; 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@xxxxxxxxxxxxxxx> > > -- > 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." -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
