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.
Signed-off-by: Vidya Sagar <[email protected]>
---
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
On 07/01/2014 01:16 AM, 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.
Out of curiosity, was this patch tested by running an umodified mainline
kernel on a Tegra device, or is this simply a port from our downstream
kernel, without any additional upstream testing?
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Tuesday, July 01, 2014 8:35 PM
> To: Vidya Sagar; [email protected]; [email protected];
> [email protected]; [email protected]; Stephen
> Warren; Krishna Thota; [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE
> mode
>
> On 07/01/2014 01:16 AM, 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.
>
> Out of curiosity, was this patch tested by running an umodified mainline
> kernel on a Tegra device, or is this simply a port from our downstream kernel,
> without any additional upstream testing?
I've tested port of this patch on downstream kernel and it works.
I've also tested this on thierry's code base (git://gitorious.org/thierryreding/linux.git) which has
upstream code for T124-Jetson-tk1.
Because of the L0s and L1 exit latencies exhibited by jetson-tk1 on-board Realtek Ethernet controller,
subsystem can't enable ASPM states on the end point,
but as such I've verified the flow with the help of debug prints and they indicate that the patch is doing its job correctly.
[+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?
> Signed-off-by: Vidya Sagar <[email protected]>
> ---
> 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
>
[+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?
>
> > Signed-off-by: Vidya Sagar <[email protected]>
> > ---
> > 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
> >
On Sat, 2014-07-05 at 13:02 -0600, Bjorn Helgaas wrote:
> > 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 believe that's the case. The proposed change would fix it but isn't
really terribly clean - I think it would be more readable to split this
into multiple functions.
--
Matthew Garrett <[email protected]>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Sunday, July 06, 2014 12:32 AM
> To: Vidya Sagar
> Cc: [email protected]; [email protected]; Stephen
> Warren; Krishna Thota; [email protected]; linux-
> [email protected]; 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.
> >
> > > Signed-off-by: Vidya Sagar <[email protected]>
> > > ---
> > > 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
> > >
On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <[email protected]> wrote:
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:[email protected]]
>> Sent: Sunday, July 06, 2014 12:32 AM
>> To: Vidya Sagar
>> Cc: [email protected]; [email protected]; Stephen
>> Warren; Krishna Thota; [email protected]; linux-
>> [email protected]; 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
>> > > Signed-off-by: Vidya Sagar <[email protected]>
>> > > ---
>> > > 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
>> > >
> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Tuesday, July 08, 2014 2:43 AM
> To: Vidya Sagar
> Cc: [email protected]; [email protected]; Stephen
> Warren; Krishna Thota; [email protected]; linux-
> [email protected]; Matthew Garrett; Rafael J. Wysocki;
> [email protected]
> Subject: Re: [PATCH v1] PCI: enable ASPM configuration in PCIE POWERSAVE
> mode
>
> On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <[email protected]> wrote:
> >> -----Original Message-----
> >> From: Bjorn Helgaas [mailto:[email protected]]
> >> Sent: Sunday, July 06, 2014 12:32 AM
> >> To: Vidya Sagar
> >> Cc: [email protected]; [email protected]; Stephen
> >> Warren; Krishna Thota; [email protected]; linux-
> >> [email protected]; 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 <[email protected]>
> >> > > ---
> >> > > 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
> >> > >
On Tue, Jul 8, 2014 at 2:42 AM, Bjorn Helgaas <[email protected]> wrote:
> On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <[email protected]> wrote:
>>> -----Original Message-----
>>> From: Bjorn Helgaas [mailto:[email protected]]
>>> Sent: Sunday, July 06, 2014 12:32 AM
>>> To: Vidya Sagar
>>> Cc: [email protected]; [email protected]; Stephen
>>> Warren; Krishna Thota; [email protected]; linux-
>>> [email protected]; 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.
Though, enabling aspm through writing 'powersave' to
'/sys/module/pcie_aspm/parameters' is working fine always.
>
>>> > > Signed-off-by: Vidya Sagar <[email protected]>
>>> > > ---
>>> > > 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
>>> > >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[updated Naga's email address]
On Wed, Jul 09, 2014 at 11:50:01PM +0530, vidya sagar wrote:
> On Tue, Jul 8, 2014 at 2:42 AM, Bjorn Helgaas <[email protected]> wrote:
> > On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <[email protected]> wrote:
> >>> -----Original Message-----
> >>> From: Bjorn Helgaas [mailto:[email protected]]
> >>> Sent: Sunday, July 06, 2014 12:32 AM
> >>> To: Vidya Sagar
> >>> Cc: [email protected]; [email protected]; Stephen
> >>> Warren; Krishna Thota; [email protected]; linux-
> >>> [email protected]; 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.
> >>> >
> >> 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.
> >
> 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.
> Though, enabling aspm through writing 'powersave' to
> '/sys/module/pcie_aspm/parameters' is working fine always.
I think your observation is correct: it looks like if we boot with all
devices in D0 (which I would expect to be a very common case), we don't
touch ASPM configuration at all. That would mean we would just leave
the BIOS settings alone.
It seems pretty surprising that it could have been this way since
v3.6, when db288c9c5f9d was merged, and we haven't noticed until now,
but if most BIOSes configure ASPM themselves, maybe it's possible.
But I wonder if we can untangle ASPM from pci_set_power_state(). I
don't think they're really related. There are a zillion .suspend()
and .resume() methods that call pci_set_power_state(). I doubt that
we need to do ASPM configuration during suspend, and I'm dubious about
resume, too. What about a patch like the following?
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 63a54a340863..75fabd1f72bc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -839,12 +839,6 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
if (!__pci_complete_power_transition(dev, state))
error = 0;
- /*
- * When aspm_policy is "powersave" this call ensures
- * that ASPM is configured.
- */
- if (!error && dev->bus->self)
- pcie_aspm_powersave_config_link(dev->bus->self);
return error;
}
@@ -1195,12 +1189,18 @@ int __weak pcibios_enable_device(struct pci_dev *dev, int bars)
static int do_pci_enable_device(struct pci_dev *dev, int bars)
{
int err;
+ struct pci_dev *bridge;
u16 cmd;
u8 pin;
err = pci_set_power_state(dev, PCI_D0);
if (err < 0 && err != -EIO)
return err;
+
+ bridge = pci_upstream_bridge(dev);
+ if (bridge)
+ pcie_aspm_powersave_config_link(bridge);
+
err = pcibios_enable_device(dev, bars);
if (err < 0)
return err;
On Sat, Jul 12, 2014 at 4:14 AM, Bjorn Helgaas <[email protected]> wrote:
> [updated Naga's email address]
>
> On Wed, Jul 09, 2014 at 11:50:01PM +0530, vidya sagar wrote:
>> On Tue, Jul 8, 2014 at 2:42 AM, Bjorn Helgaas <[email protected]> wrote:
>> > On Mon, Jul 7, 2014 at 12:00 PM, Vidya Sagar <[email protected]> wrote:
>> >>> -----Original Message-----
>> >>> From: Bjorn Helgaas [mailto:[email protected]]
>> >>> Sent: Sunday, July 06, 2014 12:32 AM
>> >>> To: Vidya Sagar
>> >>> Cc: [email protected]; [email protected]; Stephen
>> >>> Warren; Krishna Thota; [email protected]; linux-
>> >>> [email protected]; 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.
>> >>> >
>> >> 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.
>> >
>> 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.
>> Though, enabling aspm through writing 'powersave' to
>> '/sys/module/pcie_aspm/parameters' is working fine always.
>
> I think your observation is correct: it looks like if we boot with all
> devices in D0 (which I would expect to be a very common case), we don't
> touch ASPM configuration at all. That would mean we would just leave
> the BIOS settings alone.
>
> It seems pretty surprising that it could have been this way since
> v3.6, when db288c9c5f9d was merged, and we haven't noticed until now,
> but if most BIOSes configure ASPM themselves, maybe it's possible.
>
> But I wonder if we can untangle ASPM from pci_set_power_state(). I
> don't think they're really related. There are a zillion .suspend()
> and .resume() methods that call pci_set_power_state(). I doubt that
> we need to do ASPM configuration during suspend, and I'm dubious about
> resume, too. What about a patch like the following?
>
yes. this looks better. I've tested this and is working fine.
I'll push a new patch with this.
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 63a54a340863..75fabd1f72bc 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -839,12 +839,6 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>
> if (!__pci_complete_power_transition(dev, state))
> error = 0;
> - /*
> - * When aspm_policy is "powersave" this call ensures
> - * that ASPM is configured.
> - */
> - if (!error && dev->bus->self)
> - pcie_aspm_powersave_config_link(dev->bus->self);
>
> return error;
> }
> @@ -1195,12 +1189,18 @@ int __weak pcibios_enable_device(struct pci_dev *dev, int bars)
> static int do_pci_enable_device(struct pci_dev *dev, int bars)
> {
> int err;
> + struct pci_dev *bridge;
> u16 cmd;
> u8 pin;
>
> err = pci_set_power_state(dev, PCI_D0);
> if (err < 0 && err != -EIO)
> return err;
> +
> + bridge = pci_upstream_bridge(dev);
> + if (bridge)
> + pcie_aspm_powersave_config_link(bridge);
> +
> err = pcibios_enable_device(dev, bars);
> if (err < 0)
> return err;