[PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Matthew Garrett]

The aspm code will currently set the configured aspm policy before drivers
have had an opportunity to indicate that their hardware doesn't support it.
Unfortunately, putting some hardware in L0 or L1 can result in the hardware
no longer responding to any requests, even after aspm is disabled. It makes
more sense to leave aspm policy at the BIOS defaults at initial setup time,
reconfiguring it after pci_enable_device() is called. This allows the
driver to blacklist individual devices beforehand.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---

Cleaned up slightly to remove the hacky aspm_policy changing.

 drivers/pci/pcie/aspm.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index be53d98..7122281 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -588,11 +588,23 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
 	 * update through pcie_aspm_cap_init().
 	 */
 	pcie_aspm_cap_init(link, blacklist);
-	pcie_config_aspm_path(link);
 
 	/* Setup initial Clock PM state */
 	pcie_clkpm_cap_init(link, blacklist);
-	pcie_set_clkpm(link, policy_to_clkpm_state(link));
+
+	/*
+	 * At this stage drivers haven't had an opportunity to change the
+	 * link policy setting. Enabling ASPM on broken hardware can cripple
+	 * it even before the driver has had a chance to disable ASPM, so
+	 * default to a safe level right now. If we're enabling ASPM beyond
+	 * the BIOS's expectation, we'll do so once pci_enable_device() is
+	 * called.
+	 */
+	if (aspm_policy != POLICY_POWERSAVE) {
+		pcie_config_aspm_path(link);
+		pcie_set_clkpm(link, policy_to_clkpm_state(link));
+	}
+
 unlock:
 	mutex_unlock(&aspm_lock);
 out:
-- 
1.7.0.1

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Kenji Kaneshige]

Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>


(2010/06/10 5:05), Matthew Garrett wrote:
> The aspm code will currently set the configured aspm policy before drivers
> have had an opportunity to indicate that their hardware doesn't support it.
> Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> no longer responding to any requests, even after aspm is disabled. It makes
> more sense to leave aspm policy at the BIOS defaults at initial setup time,
> reconfiguring it after pci_enable_device() is called. This allows the
> driver to blacklist individual devices beforehand.
>
> Signed-off-by: Matthew Garrett<mjg@redhat.com>
> ---
>
> Cleaned up slightly to remove the hacky aspm_policy changing.
>
>   drivers/pci/pcie/aspm.c |   16 ++++++++++++++--
>   1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index be53d98..7122281 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -588,11 +588,23 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>   	 * update through pcie_aspm_cap_init().
>   	 */
>   	pcie_aspm_cap_init(link, blacklist);
> -	pcie_config_aspm_path(link);
>
>   	/* Setup initial Clock PM state */
>   	pcie_clkpm_cap_init(link, blacklist);
> -	pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +
> +	/*
> +	 * At this stage drivers haven't had an opportunity to change the
> +	 * link policy setting. Enabling ASPM on broken hardware can cripple
> +	 * it even before the driver has had a chance to disable ASPM, so
> +	 * default to a safe level right now. If we're enabling ASPM beyond
> +	 * the BIOS's expectation, we'll do so once pci_enable_device() is
> +	 * called.
> +	 */
> +	if (aspm_policy != POLICY_POWERSAVE) {
> +		pcie_config_aspm_path(link);
> +		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	}
> +
>   unlock:
>   	mutex_unlock(&aspm_lock);
>   out:

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Tomas Henzl]

On 06/09/2010 10:05 PM, Matthew Garrett wrote:
> The aspm code will currently set the configured aspm policy before drivers
> have had an opportunity to indicate that their hardware doesn't support it.
> Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> no longer responding to any requests, even after aspm is disabled. It makes
> more sense to leave aspm policy at the BIOS defaults at initial setup time,
> reconfiguring it after pci_enable_device() is called. This allows the
> driver to blacklist individual devices beforehand.
>
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>
> Cleaned up slightly to remove the hacky aspm_policy changing.
>
>  drivers/pci/pcie/aspm.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index be53d98..7122281 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -588,11 +588,23 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>  	 * update through pcie_aspm_cap_init().
>  	 */
>  	pcie_aspm_cap_init(link, blacklist);
> -	pcie_config_aspm_path(link);
>  
>  	/* Setup initial Clock PM state */
>  	pcie_clkpm_cap_init(link, blacklist);
> -	pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +
> +	/*
> +	 * At this stage drivers haven't had an opportunity to change the
> +	 * link policy setting. Enabling ASPM on broken hardware can cripple
> +	 * it even before the driver has had a chance to disable ASPM, so
> +	 * default to a safe level right now. If we're enabling ASPM beyond
> +	 * the BIOS's expectation, we'll do so once pci_enable_device() is
> +	 * called.
> +	 */
> +	if (aspm_policy != POLICY_POWERSAVE) {
> +		pcie_config_aspm_path(link);
> +		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	}
>   
Matthew, isn't it so that the POLICY_DEFAULT will pass the above test
and possibly switch the ASPM on?

tomas

> +
>  unlock:
>  	mutex_unlock(&aspm_lock);
>  out:
>   

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Matthew Garrett]

On Tue, Jun 15, 2010 at 05:11:59PM +0200, Tomas Henzl wrote:

> Matthew, isn't it so that the POLICY_DEFAULT will pass the above test
> and possibly switch the ASPM on?

Yes. We assume that the BIOS authors aren't actively malicious.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Tomas Henzl]

On 06/15/2010 05:18 PM, Matthew Garrett wrote:
> On Tue, Jun 15, 2010 at 05:11:59PM +0200, Tomas Henzl wrote:
>
>   
>> Matthew, isn't it so that the POLICY_DEFAULT will pass the above test
>> and possibly switch the ASPM on?
>>     
> Yes. We assume that the BIOS authors aren't actively malicious.
>   
I originally thought your patch is about not enabling ASPM on broken 
hardware (bios included) before the driver has a chance to change it.

Sorry for the confusion, I was wrong with the bios.

tomas

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Matthew Garrett]

On Tue, Jun 15, 2010 at 05:29:02PM +0200, Tomas Henzl wrote:
> On 06/15/2010 05:18 PM, Matthew Garrett wrote:
> > On Tue, Jun 15, 2010 at 05:11:59PM +0200, Tomas Henzl wrote:
> >
> >   
> >> Matthew, isn't it so that the POLICY_DEFAULT will pass the above test
> >> and possibly switch the ASPM on?
> >>     
> > Yes. We assume that the BIOS authors aren't actively malicious.
> >   
> I originally thought your patch is about not enabling ASPM on broken 
> hardware (bios included) before the driver has a chance to change it.

Heh. No, if the BIOS set up ASPM on the controller, it seems like it's 
safe to do the same. That's been the default behaviour of upstream for 
some time now.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Maxim Levitsky]

On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> The aspm code will currently set the configured aspm policy before drivers
> have had an opportunity to indicate that their hardware doesn't support it.
> Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> no longer responding to any requests, even after aspm is disabled. It makes
> more sense to leave aspm policy at the BIOS defaults at initial setup time,
> reconfiguring it after pci_enable_device() is called. This allows the
> driver to blacklist individual devices beforehand.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---

Hi,

I recently discovered that my aspire one wireless troubles (card just
dies after a while) are caused by ASPM L0S state.
The device (AR5001) seems to have a hardware bug, and it also disables
L0S in windows driver.

Unfortenuly BIOS (news at 11) enables L0S.

Its easy to disable ASPM from driver. It just a matter of calling
pci_disable_link_state.

However, that depends on CONFIG_PCIEASPM.

How about making pci_disable_link_state always available or even better,
just make CONFIG_PCIEASPM unconditional?


Btw, this is my last linux problem that had slim chances to be solved.
The other one was s2ram hang on my primary notebook.

This just shows that in linux everyone helps each other.

I fixed almost impossible s2ram problem on my notebook.
Jussi Kivilinna fixed almost impossible to solve problem on my aspire
one.


Best regards,
Maxim Levitsky

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Jesse Barnes]

On Fri, 18 Jun 2010 15:06:51 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> > The aspm code will currently set the configured aspm policy before drivers
> > have had an opportunity to indicate that their hardware doesn't support it.
> > Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> > no longer responding to any requests, even after aspm is disabled. It makes
> > more sense to leave aspm policy at the BIOS defaults at initial setup time,
> > reconfiguring it after pci_enable_device() is called. This allows the
> > driver to blacklist individual devices beforehand.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > ---
> 
> Hi,
> 
> I recently discovered that my aspire one wireless troubles (card just
> dies after a while) are caused by ASPM L0S state.
> The device (AR5001) seems to have a hardware bug, and it also disables
> L0S in windows driver.
> 
> Unfortenuly BIOS (news at 11) enables L0S.
> 
> Its easy to disable ASPM from driver. It just a matter of calling
> pci_disable_link_state.
> 
> However, that depends on CONFIG_PCIEASPM.
> 
> How about making pci_disable_link_state always available or even better,
> just make CONFIG_PCIEASPM unconditional?

The former is ok with me.  Care to post a patch?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Maxim Levitsky]

On Fri, 2010-06-18 at 09:12 -0700, Jesse Barnes wrote: 
> On Fri, 18 Jun 2010 15:06:51 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> > > The aspm code will currently set the configured aspm policy before drivers
> > > have had an opportunity to indicate that their hardware doesn't support it.
> > > Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> > > no longer responding to any requests, even after aspm is disabled. It makes
> > > more sense to leave aspm policy at the BIOS defaults at initial setup time,
> > > reconfiguring it after pci_enable_device() is called. This allows the
> > > driver to blacklist individual devices beforehand.
> > > 
> > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > ---
> > 
> > Hi,
> > 
> > I recently discovered that my aspire one wireless troubles (card just
> > dies after a while) are caused by ASPM L0S state.
> > The device (AR5001) seems to have a hardware bug, and it also disables
> > L0S in windows driver.
> > 
> > Unfortenuly BIOS (news at 11) enables L0S.
> > 
> > Its easy to disable ASPM from driver. It just a matter of calling
> > pci_disable_link_state.
> > 
> > However, that depends on CONFIG_PCIEASPM.
> > 
> > How about making pci_disable_link_state always available or even better,
> > just make CONFIG_PCIEASPM unconditional?
> 
> The former is ok with me.  Care to post a patch?
It not that simple at first glance.
This functions uses plenty of code from the aspm.c, therefore care
should be taken to do that properly.

Of course the easy solution is to compile all code in always, and just
disable it by runtime switch (it even exists, aspm_disabled)
Or, another easy solution is to make ath5k depend on CONFIG_PCIEASPM

What do you think?

Best regards,
Maxim Levitsky

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Jesse Barnes]

On Fri, 18 Jun 2010 20:05:42 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Fri, 2010-06-18 at 09:12 -0700, Jesse Barnes wrote: 
> > On Fri, 18 Jun 2010 15:06:51 +0300
> > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > 
> > > On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> > > > The aspm code will currently set the configured aspm policy before drivers
> > > > have had an opportunity to indicate that their hardware doesn't support it.
> > > > Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> > > > no longer responding to any requests, even after aspm is disabled. It makes
> > > > more sense to leave aspm policy at the BIOS defaults at initial setup time,
> > > > reconfiguring it after pci_enable_device() is called. This allows the
> > > > driver to blacklist individual devices beforehand.
> > > > 
> > > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > > ---
> > > 
> > > Hi,
> > > 
> > > I recently discovered that my aspire one wireless troubles (card just
> > > dies after a while) are caused by ASPM L0S state.
> > > The device (AR5001) seems to have a hardware bug, and it also disables
> > > L0S in windows driver.
> > > 
> > > Unfortenuly BIOS (news at 11) enables L0S.
> > > 
> > > Its easy to disable ASPM from driver. It just a matter of calling
> > > pci_disable_link_state.
> > > 
> > > However, that depends on CONFIG_PCIEASPM.
> > > 
> > > How about making pci_disable_link_state always available or even better,
> > > just make CONFIG_PCIEASPM unconditional?
> > 
> > The former is ok with me.  Care to post a patch?
> It not that simple at first glance.
> This functions uses plenty of code from the aspm.c, therefore care
> should be taken to do that properly.
> 
> Of course the easy solution is to compile all code in always, and just
> disable it by runtime switch (it even exists, aspm_disabled)
> Or, another easy solution is to make ath5k depend on CONFIG_PCIEASPM
> 
> What do you think?

I do like the idea of getting rid of config options.  I just want to
make sure we don't regress people, so I need to double check the
default behavior, especially for non-x86 and see whether it'll
generally be a no-op or not.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Jesse Barnes]

On Fri, 18 Jun 2010 20:05:42 +0300
Maxim Levitsky <maximlevitsky@gmail.com> wrote:

> On Fri, 2010-06-18 at 09:12 -0700, Jesse Barnes wrote: 
> > On Fri, 18 Jun 2010 15:06:51 +0300
> > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > 
> > > On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> > > > The aspm code will currently set the configured aspm policy before drivers
> > > > have had an opportunity to indicate that their hardware doesn't support it.
> > > > Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> > > > no longer responding to any requests, even after aspm is disabled. It makes
> > > > more sense to leave aspm policy at the BIOS defaults at initial setup time,
> > > > reconfiguring it after pci_enable_device() is called. This allows the
> > > > driver to blacklist individual devices beforehand.
> > > > 
> > > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > > ---
> > > 
> > > Hi,
> > > 
> > > I recently discovered that my aspire one wireless troubles (card just
> > > dies after a while) are caused by ASPM L0S state.
> > > The device (AR5001) seems to have a hardware bug, and it also disables
> > > L0S in windows driver.
> > > 
> > > Unfortenuly BIOS (news at 11) enables L0S.
> > > 
> > > Its easy to disable ASPM from driver. It just a matter of calling
> > > pci_disable_link_state.
> > > 
> > > However, that depends on CONFIG_PCIEASPM.
> > > 
> > > How about making pci_disable_link_state always available or even better,
> > > just make CONFIG_PCIEASPM unconditional?
> > 
> > The former is ok with me.  Care to post a patch?
> It not that simple at first glance.
> This functions uses plenty of code from the aspm.c, therefore care
> should be taken to do that properly.
> 
> Of course the easy solution is to compile all code in always, and just
> disable it by runtime switch (it even exists, aspm_disabled)
> Or, another easy solution is to make ath5k depend on CONFIG_PCIEASPM
> 
> What do you think?

So unless a device is *really* broken, it seems like the default policy
should be ok across the board.  So let's get a CONFIG_PCIEASPM removal
patch into the 2.6.36 merge window early to flush out any potential
problems.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Maxim Levitsky]

On Fri, 2010-06-18 at 10:15 -0700, Jesse Barnes wrote: 
> On Fri, 18 Jun 2010 20:05:42 +0300
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > On Fri, 2010-06-18 at 09:12 -0700, Jesse Barnes wrote: 
> > > On Fri, 18 Jun 2010 15:06:51 +0300
> > > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > > 
> > > > On Wed, 2010-06-09 at 16:05 -0400, Matthew Garrett wrote: 
> > > > > The aspm code will currently set the configured aspm policy before drivers
> > > > > have had an opportunity to indicate that their hardware doesn't support it.
> > > > > Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> > > > > no longer responding to any requests, even after aspm is disabled. It makes
> > > > > more sense to leave aspm policy at the BIOS defaults at initial setup time,
> > > > > reconfiguring it after pci_enable_device() is called. This allows the
> > > > > driver to blacklist individual devices beforehand.
> > > > > 
> > > > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > I recently discovered that my aspire one wireless troubles (card just
> > > > dies after a while) are caused by ASPM L0S state.
> > > > The device (AR5001) seems to have a hardware bug, and it also disables
> > > > L0S in windows driver.
> > > > 
> > > > Unfortenuly BIOS (news at 11) enables L0S.
> > > > 
> > > > Its easy to disable ASPM from driver. It just a matter of calling
> > > > pci_disable_link_state.
> > > > 
> > > > However, that depends on CONFIG_PCIEASPM.
> > > > 
> > > > How about making pci_disable_link_state always available or even better,
> > > > just make CONFIG_PCIEASPM unconditional?
> > > 
> > > The former is ok with me.  Care to post a patch?
> > It not that simple at first glance.
> > This functions uses plenty of code from the aspm.c, therefore care
> > should be taken to do that properly.
> > 
> > Of course the easy solution is to compile all code in always, and just
> > disable it by runtime switch (it even exists, aspm_disabled)
> > Or, another easy solution is to make ath5k depend on CONFIG_PCIEASPM
> > 
> > What do you think?
> 
> So unless a device is *really* broken, it seems like the default policy
> should be ok across the board.  So let's get a CONFIG_PCIEASPM removal
> patch into the 2.6.36 merge window early to flush out any potential
> problems.
It would be perfect for me.

Best regards,
Maxim Levitsky

Re: [PATCH] pci: Don't enable aspm before drivers have had a chance to veto it

[Jesse Barnes]

On Wed,  9 Jun 2010 16:05:07 -0400
Matthew Garrett <mjg@redhat.com> wrote:

> The aspm code will currently set the configured aspm policy before drivers
> have had an opportunity to indicate that their hardware doesn't support it.
> Unfortunately, putting some hardware in L0 or L1 can result in the hardware
> no longer responding to any requests, even after aspm is disabled. It makes
> more sense to leave aspm policy at the BIOS defaults at initial setup time,
> reconfiguring it after pci_enable_device() is called. This allows the
> driver to blacklist individual devices beforehand.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
> 
> Cleaned up slightly to remove the hacky aspm_policy changing.
> 
>  drivers/pci/pcie/aspm.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index be53d98..7122281 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -588,11 +588,23 @@ void pcie_aspm_init_link_state(struct pci_dev *pdev)
>  	 * update through pcie_aspm_cap_init().
>  	 */
>  	pcie_aspm_cap_init(link, blacklist);
> -	pcie_config_aspm_path(link);
>  
>  	/* Setup initial Clock PM state */
>  	pcie_clkpm_cap_init(link, blacklist);
> -	pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +
> +	/*
> +	 * At this stage drivers haven't had an opportunity to change the
> +	 * link policy setting. Enabling ASPM on broken hardware can cripple
> +	 * it even before the driver has had a chance to disable ASPM, so
> +	 * default to a safe level right now. If we're enabling ASPM beyond
> +	 * the BIOS's expectation, we'll do so once pci_enable_device() is
> +	 * called.
> +	 */
> +	if (aspm_policy != POLICY_POWERSAVE) {
> +		pcie_config_aspm_path(link);
> +		pcie_set_clkpm(link, policy_to_clkpm_state(link));
> +	}
> +
>  unlock:
>  	mutex_unlock(&aspm_lock);
>  out:

Applied to linux-next, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center