Message-ID: <113ACA888B71994BB56E5CF3704953486D8E9DFEAF@BGMAIL02.nvidia.com>
Date:	Tue, 8 Jul 2014 10:37:40 +0530
From:	Vidya Sagar <vidyas@...dia.com>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
CC:	"nagananda.chumbalkar@...com" <nagananda.chumbalkar@...com>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>,
	Stephen Warren <swarren@...dia.com>,
	Krishna Thota <kthota@...dia.com>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Matthew Garrett" <matthew.garrett@...ula.com>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"tianyu.lan@...el.com" <tianyu.lan@...el.com>
Subject: RE: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE mode

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@...gle.com]
> Sent: Tuesday, July 08, 2014 2:43 AM
> To: Vidya Sagar
> Cc: nagananda.chumbalkar@...com; thierry.reding@...il.com; Stephen
> Warren; Krishna Thota; linux-pci@...r.kernel.org; linux-
> kernel@...r.kernel.org; Matthew Garrett; Rafael J. Wysocki;
> tianyu.lan@...el.com
> Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE
> mode
> 
> On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <vidyas@...dia.com> wrote:
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:bhelgaas@...gle.com]
> >> Sent: Sunday, July 06, 2014 12:32 AM
> >> To: Vidya Sagar
> >> Cc: nagananda.chumbalkar@...com; thierry.reding@...il.com; Stephen
> >> Warren; Krishna Thota; linux-pci@...r.kernel.org; linux-
> >> kernel@...r.kernel.org; Matthew Garrett; Rafael J. Wysocki
> >> Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE
> >> POWERSAVE mode
> >>
> >> [+cc Rafael]
> >>
> >> On Sat, Jul 05, 2014 at 12:57:36PM -0600, Bjorn Helgaas wrote:
> >> > [+cc Matthew]
> >> >
> >> > On Tue, Jul 01, 2014 at 12:46:18PM +0530, Vidya Sagar wrote:
> >> > > commit 1a680b7c moved pcie_aspm_powersave_config_link() out of
> >> > > pci_raw_set_power_state() to pci_set_power_state() which would
> >> > > enable ASPM. But, with commit db288c9c, which re-introduced the
> >> > > following check
> >> > > ./drivers/pci/pci.c: pci_set_power_state()
> >> > > +   /* Check if we're already there */
> >> > > +   if (dev->current_state == state)
> >> > > +       return 0;
> >> > > in pci_set_power_state(), call to
> >> > > pcie_aspm_powersave_config_link() is never made leaving ASPM
> broken.
> >> > > Fix it by not returning from when the above condition is true,
> >> > > rather, jump to ASPM configuration code and exit from there
> eventually.
> >> >
> >> > Rafael, Matthew, any comments?  We have vacillated on this before
> >> > and the web is already pretty tangled.
> >> >
> >> > Vidya, can you give more details about the bug fixed by this change?
> >> > What's the scenario?  Are we resuming and the device is powered up
> >> > but ASPM isn't enabled?  Maybe you could collect more details in a
> >> > http://bugzilla.kernel.org report?
> >
> > I've raised a bugzilla bug
> https://bugzilla.kernel.org/show_bug.cgi?id=79621 for the same.
> > Scenario here is, when the driver of a particular PCIe device is loaded and
> when CONFIG_PCIEASPM_POWERSAVE=y, ASPM is expected to get
> configured by the sub system which is not happening as of today.
> > The reason for this behavior is the 'if' condition that checks for the device's
> power state and returns if it is already in D0. Ideally, it shouldn't return from
> there, rather proceed to configure ASPM.
> 
> Thanks, Vidya.  I'm not really looking for analysis of the code flow, since I can
> work that out myself.  What I *would* like is a complete dmesg log and "lspci
> -vv" output, because I'd like to know what's special about this particular
> device.  We boot with devices powered on (in D0) all the time, and most of
> the time, ASPM is configured correctly.  So I want to know what is different
> about this case.
> 
> Bjorn
> 

Please find the attached files (before_aspm_patch.txt & after_aspm_patch.txt) with 'dmesg' and 'lspci -vvv' info.
BTW, my understanding is that this issue (enabling ASPM while booting) exists for all the devices which are in D0 state.
Please correct my understanding if it's wrong.
Of course, enabling aspm through writing 'powersave' to '/sys/module/pcie_aspm/parameters' is working fine always.


> >> > > Signed-off-by: Vidya Sagar <vidyas@...dia.com>
> >> > > ---
> >> > >  drivers/pci/pci.c | 6 ++++--
> >> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> >> > >
> >> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index
> >> > > 1c8592b..ded24c4 100644
> >> > > --- a/drivers/pci/pci.c
> >> > > +++ b/drivers/pci/pci.c
> >> > > @@ -804,7 +804,7 @@
> >> EXPORT_SYMBOL_GPL(__pci_complete_power_transition);
> >> > >   */
> >> > >  int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
> >> > > {
> >> > > - int error;
> >> > > + int error = 0;
> >> > >
> >> > >   /* bound the state we're entering */
> >> > >   if (state > PCI_D3cold)
> >> > > @@ -821,7 +821,7 @@ int pci_set_power_state(struct pci_dev *dev,
> >> > > pci_power_t state)
> >> > >
> >> > >   /* Check if we're already there */
> >> > >   if (dev->current_state == state)
> >> > > -         return 0;
> >> > > +         goto config_aspm;
> >> > >
> >> > >   __pci_start_power_transition(dev, state);
> >> > >
> >> > > @@ -839,6 +839,8 @@ int pci_set_power_state(struct pci_dev *dev,
> >> > > pci_power_t state)
> >> > >
> >> > >   if (!__pci_complete_power_transition(dev, state))
> >> > >           error = 0;
> >> > > +
> >> > > +config_aspm:
> >> > >   /*
> >> > >    * When aspm_policy is "powersave" this call ensures
> >> > >    * that ASPM is configured.
> >> > > --
> >> > > 1.8.1.5
> >> > >

View attachment "before_aspm_patch.txt" of type "text/plain" (58889 bytes)

View attachment "after_aspm_patch.txt" of type "text/plain" (59036 bytes)

Powered by blists - more mailing lists