> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Saturday, May 19, 2012 9:20 AM > To: Xudong Hao > Cc: linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > kvm@xxxxxxxxxxxxxxx; avi@xxxxxxxxxx; alex.williamson@xxxxxxxxxx; Zhang, > Xiantao; Hao, Xudong > Subject: Re: [PATCH 1/1] Enable LTR/OBFF before device is used by driver > > On Sun, May 13, 2012 at 8:48 PM, Xudong Hao <xudong.hao@xxxxxxxxxxxxxxx> > wrote: > > Enable LTR(Latency tolerance reporting) and OBFF(optimized buffer flush/fill) > in > > pci_enable_device(), so that they are enabled before the device is used by > driver. > > Please split this into two patches (one for LTR and another for OBFF) > so they can be reverted individually if they cause trouble. OK. > It would > be nice if you bundled these together with your other "save/restore > max Latency Value" patch so they were all together on the mailing > list. > Sure, I'll modify the save/restore patch and bundle them together. > I read the LTR sections of the PCIe spec, but I can't figure out how > it's supposed to work. It says "power management policies ... can be > implemented to consider Endpoint service requirements." Does that > mean there's some OS software that might be involved, or is this just > a matter of software flipping the LTR-enable bit and the hardware > doing everything else? How confident can we be that enabling this is > safe? > Software only set the LTR-enable bit, then hardware/chipset/device do everything else. There are one thing that software can be involved: software can configure maximum latency tolerance. > For OBFF, is there some OS piece not included here that tells a Root > Complex that "now is a good time for Endpoints to do something," e.g., > the spec mentions an "operating system timer tick." Is there some > benefit to this patch without that piece? I don't understand the big > picture yet. > As like LTR, OBFF do not need OS do additional changes, just set obff-enable bit. > > Signed-off-by: Xudong Hao <xudong.hao@xxxxxxxxx> > > > > --- > > drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++ > > 1 files changed, 29 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 111569c..2369883 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -1134,6 +1134,31 @@ int pci_load_and_free_saved_state(struct > pci_dev *dev, > > } > > EXPORT_SYMBOL_GPL(pci_load_and_free_saved_state); > > > > +static void pci_enable_dev_caps(struct pci_dev *dev) > > +{ > > + /* set default value */ > > + unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS; > > There's only one use of this value, so skip the variable and just use > PCI_EXP_OBFF_SIGNAL_ALWAYS in the call. > Ok. > The comment at pci_enable_obff() says PCI_OBFF_SIGNAL_L0 is the > preferred type, so please explain why you're not using that. > Yes, here it's better to set PCI_OBFF_SIGNAL_L0 by default. > > + > > + /* LTR(Latency tolerance reporting) allows devices to send > > + * messages to the root complex indicating their latency > > + * tolerance for snooped & unsnooped memory transactions. > > + */ > > Follow Linux comment style, i.e., > > /* > * LTR ... > */ > Will modify, Thanks. > > + pci_enable_ltr(dev); > > + > > + /* OBFF (optimized buffer flush/fill), where supported, > > + * can help improve energy efficiency by giving devices > > + * information about when interrupts and other activity > > + * will have a reduced power impact. > > + */ > > + pci_enable_obff(dev, type); > > +} > > + > > +static void pci_disable_dev_caps(struct pci_dev *dev) > > +{ > > + pci_disable_obff(dev); > > + pci_disable_ltr(dev); > > +} > > + > > static int do_pci_enable_device(struct pci_dev *dev, int bars) > > { > > int err; > > @@ -1146,6 +1171,9 @@ static int do_pci_enable_device(struct pci_dev > *dev, int bars) > > return err; > > pci_fixup_device(pci_fixup_enable, dev); > > > > + /* Enable some device capibility before it's used by driver. */ > > + pci_enable_dev_caps(dev); > > Why is this here? It seems similar to what's already in > pci_init_capabilities(). Is there a reason to do this in the > pci_enable_device() path rather than in the pci_device_add() path? > pci_enable_device is called by any pci driver including kvm driver, Considering such a case in kvm, when device is assigned to guest(the device will be reset), we want not host lose those advanced PM feature, so add it in pci_enable_device so that kvm driver call it. > > + > > return 0; > > } > > > > @@ -1361,6 +1389,7 @@ static void do_pci_disable_device(struct pci_dev > *dev) > > } > > > > pcibios_disable_device(dev); > > + pci_disable_dev_caps(dev); > > } > > > > /** > > -- > > 1.6.0.rc1 > > -- 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
