[1/1] Enable LTR/OBFF before device is used by driver

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.

Signed-off-by: Xudong Hao <xudong.hao@intel.com>

---
 drivers/pci/pci.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

--
1.6.0.rc1

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, May 13, 2012 at 8:48 PM, Xudong Hao <xudong.hao@linux.intel.com> 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.  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.

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?

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.

> Signed-off-by: Xudong Hao <xudong.hao@intel.com>
>
> ---
>  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.

The comment at pci_enable_obff() says PCI_OBFF_SIGNAL_L0 is the
preferred type, so please explain why you're not using that.

> +
> +       /* 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 ...
     */

> +       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?

> +
>        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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Saturday, May 19, 2012 9:20 AM
> To: Xudong Hao
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> kvm@vger.kernel.org; avi@redhat.com; alex.williamson@redhat.com; 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@linux.intel.com>
> 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@intel.com>
> >
> > ---
> >  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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

While you are making the other recommended fixes, could
you add/create a pci_obff_supported() function, like the pci_ltr_supported()
function, and more importantly, add it to the pci_disable_obff() function?
The latter does not check that DEVCAP2 is supported, and thus, could be
writing to non-existent (potentially private) cap space (i.e., a V1 CAP device,
which do exist in the marketplace).

The [enable,disable]_ltr functions do a good job of doing pci_ltr_supported()
checks, but the obff enable,disable functions should have similar calls.


On 05/13/2012 10:48 PM, Xudong Hao 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.
>
> Signed-off-by: Xudong Hao<xudong.hao@intel.com>
>
> ---
>   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;
> +
> +       /* LTR(Latency tolerance reporting) allows devices to send
> +        * messages to the root complex indicating their latency
> +        * tolerance for snooped&  unsnooped memory transactions.
> +        */
> +       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);
> +
>          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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> -----Original Message-----
> From: Don Dutile [mailto:ddutile@redhat.com]
> Sent: Thursday, May 31, 2012 12:13 AM
> To: Xudong Hao
> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org;
> linux-kernel@vger.kernel.org; kvm@vger.kernel.org; avi@redhat.com;
> alex.williamson@redhat.com; Zhang, Xiantao; Hao, Xudong
> Subject: Re: [PATCH 1/1] Enable LTR/OBFF before device is used by driver
> 
> While you are making the other recommended fixes, could
> you add/create a pci_obff_supported() function, like the pci_ltr_supported()
> function, and more importantly, add it to the pci_disable_obff() function?
> The latter does not check that DEVCAP2 is supported, and thus, could be
> writing to non-existent (potentially private) cap space (i.e., a V1 CAP device,
> which do exist in the marketplace).
> 
> The [enable,disable]_ltr functions do a good job of doing pci_ltr_supported()
> checks, but the obff enable,disable functions should have similar calls.
> 

Yeah, it's a good suggestion, I'll add pci_obff_supported() function.

> 
> On 05/13/2012 10:48 PM, Xudong Hao 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.
> >
> > Signed-off-by: Xudong Hao<xudong.hao@intel.com>
> >
> > ---
> >   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;
> > +
> > +       /* LTR(Latency tolerance reporting) allows devices to send
> > +        * messages to the root complex indicating their latency
> > +        * tolerance for snooped&  unsnooped memory transactions.
> > +        */
> > +       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);
> > +
> >          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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html