[PATCH] ath5k: disable ASPM

[PATCH] ath5k: disable ASPM

[Jussi Kivilinna]

Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
enabled. With ASPM ath5k will eventually stall on heavy traffic with often
'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
these problems.

Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
both ends (usually stalls within seconds).

Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
---
 drivers/net/wireless/ath/ath5k/base.c |   48 +++++++++++++++++++++++++++++++++
 1 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index cc6d41d..ce9c983 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -48,6 +48,7 @@
 #include <linux/netdevice.h>
 #include <linux/cache.h>
 #include <linux/pci.h>
+#include <linux/pci-aspm.h>
 #include <linux/ethtool.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
@@ -195,6 +196,8 @@ static const struct ieee80211_rate ath5k_rates[] = {
 static int __devinit	ath5k_pci_probe(struct pci_dev *pdev,
 				const struct pci_device_id *id);
 static void __devexit	ath5k_pci_remove(struct pci_dev *pdev);
+static void		__ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
+static void		ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
 #ifdef CONFIG_PM
 static int		ath5k_pci_suspend(struct device *dev);
 static int		ath5k_pci_resume(struct device *dev);
@@ -424,6 +427,47 @@ module_exit(exit_ath5k_pci);
 * PCI Initialization *
 \********************/
 
+#ifdef CONFIG_PCIEASPM
+static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
+{
+	pci_disable_link_state(pdev, state);
+}
+#else
+static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
+{
+	int pos;
+	u16 reg16;
+
+	/*
+	 * Both device and parent should have the same ASPM setting.
+	 * Disable ASPM in downstream component first and then upstream.
+	 */
+	pos = pci_pcie_cap(pdev);
+	pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, &reg16);
+	reg16 &= ~state;
+	pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
+
+	if (!pdev->bus->self)
+		return;
+
+	pos = pci_pcie_cap(pdev->bus->self);
+	pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, &reg16);
+	reg16 &= ~state;
+	pci_write_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, reg16);
+}
+#endif
+static void ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
+{
+	if (!pdev->is_pcie)
+		return;
+
+	dev_info(&pdev->dev, "Disabling ASPM %s%s\n",
+		 (state & PCIE_LINK_STATE_L0S) ? "L0s " : "",
+		 (state & PCIE_LINK_STATE_L1) ? "L1" : "");
+
+	__ath5k_disable_aspm(pdev, state);
+}
+
 static const char *
 ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
 {
@@ -473,6 +517,8 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	int ret;
 	u8 csz;
 
+	ath5k_disable_aspm(pdev, PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L0S);
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "can't enable device\n");
@@ -725,6 +771,8 @@ static int ath5k_pci_resume(struct device *dev)
 	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
 	struct ath5k_softc *sc = hw->priv;
 
+	ath5k_disable_aspm(pdev, PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L0S);
+
 	/*
 	 * Suspend/Resume resets the PCI configuration space, so we have to
 	 * re-disable the RETRY_TIMEOUT register (0x41) to keep

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Pavel Roskin]

On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote:
> +static void		__ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
> +static void		ath5k_disable_aspm(struct pci_dev *pdev, u16 state);

Forward declarations should not be needed unless the functions are
called before their implementations, which is not the case here.

> @@ -424,6 +427,47 @@ module_exit(exit_ath5k_pci);
>  * PCI Initialization *
>  \********************/
>  
> +#ifdef CONFIG_PCIEASPM
> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
> +{
> +	pci_disable_link_state(pdev, state);
> +}
> +#else
> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
> +{

It looks like a replacement for pci_disable_link_state() if
CONFIG_PCIEASPM is disabled.  I guess it should be in the PCI code, not
in ath5k.  At least the PCI developers should have a look at the
replacement code.

-- 
Regards,
Pavel Roskin

Re: [PATCH] ath5k: disable ASPM

[Luis R. Rodriguez]

On Fri, May 28, 2010 at 3:09 AM, Jussi Kivilinna
<jussi.kivilinna@mbnet.fi> wrote:
> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
> these problems.

So you disable ASPM all together on the entire driver?

  Luis

> Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> both ends (usually stalls within seconds).
>
> Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> ---
>  drivers/net/wireless/ath/ath5k/base.c |   48 +++++++++++++++++++++++++++++++++
>  1 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index cc6d41d..ce9c983 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -48,6 +48,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/cache.h>
>  #include <linux/pci.h>
> +#include <linux/pci-aspm.h>
>  #include <linux/ethtool.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> @@ -195,6 +196,8 @@ static const struct ieee80211_rate ath5k_rates[] = {
>  static int __devinit   ath5k_pci_probe(struct pci_dev *pdev,
>                                const struct pci_device_id *id);
>  static void __devexit  ath5k_pci_remove(struct pci_dev *pdev);
> +static void            __ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
> +static void            ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
>  #ifdef CONFIG_PM
>  static int             ath5k_pci_suspend(struct device *dev);
>  static int             ath5k_pci_resume(struct device *dev);
> @@ -424,6 +427,47 @@ module_exit(exit_ath5k_pci);
>  * PCI Initialization *
>  \********************/
>
> +#ifdef CONFIG_PCIEASPM
> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
> +{
> +       pci_disable_link_state(pdev, state);
> +}
> +#else
> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
> +{
> +       int pos;
> +       u16 reg16;
> +
> +       /*
> +        * Both device and parent should have the same ASPM setting.
> +        * Disable ASPM in downstream component first and then upstream.
> +        */
> +       pos = pci_pcie_cap(pdev);
> +       pci_read_config_word(pdev, pos + PCI_EXP_LNKCTL, &reg16);
> +       reg16 &= ~state;
> +       pci_write_config_word(pdev, pos + PCI_EXP_LNKCTL, reg16);
> +
> +       if (!pdev->bus->self)
> +               return;
> +
> +       pos = pci_pcie_cap(pdev->bus->self);
> +       pci_read_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, &reg16);
> +       reg16 &= ~state;
> +       pci_write_config_word(pdev->bus->self, pos + PCI_EXP_LNKCTL, reg16);
> +}
> +#endif
> +static void ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
> +{
> +       if (!pdev->is_pcie)
> +               return;
> +
> +       dev_info(&pdev->dev, "Disabling ASPM %s%s\n",
> +                (state & PCIE_LINK_STATE_L0S) ? "L0s " : "",
> +                (state & PCIE_LINK_STATE_L1) ? "L1" : "");
> +
> +       __ath5k_disable_aspm(pdev, state);
> +}
> +
>  static const char *
>  ath5k_chip_name(enum ath5k_srev_type type, u_int16_t val)
>  {
> @@ -473,6 +517,8 @@ ath5k_pci_probe(struct pci_dev *pdev,
>        int ret;
>        u8 csz;
>
> +       ath5k_disable_aspm(pdev, PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L0S);
> +
>        ret = pci_enable_device(pdev);
>        if (ret) {
>                dev_err(&pdev->dev, "can't enable device\n");
> @@ -725,6 +771,8 @@ static int ath5k_pci_resume(struct device *dev)
>        struct ieee80211_hw *hw = pci_get_drvdata(pdev);
>        struct ath5k_softc *sc = hw->priv;
>
> +       ath5k_disable_aspm(pdev, PCIE_LINK_STATE_L1 | PCIE_LINK_STATE_L0S);
> +
>        /*
>         * Suspend/Resume resets the PCI configuration space, so we have to
>         * re-disable the RETRY_TIMEOUT register (0x41) to keep
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Luis R. Rodriguez" <mcgrof@gmail.com>:

> On Fri, May 28, 2010 at 3:09 AM, Jussi Kivilinna
> <jussi.kivilinna@mbnet.fi> wrote:
>> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
>> these problems.
>
> So you disable ASPM all together on the entire driver?
>

Sorry, I meant to this to be RFC patch. I'm not familiar with PCI  
devices/drivers.. How should I pinpoint this patch? To 168c:001c? only  
rev 1? Is there other revs that don't mix with ASPM? Is this AOA150  
problem only? Use AOA150 specific quirk?

-Jussi

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Pavel Roskin" <proski@gnu.org>:

> On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote:
>> +static void		__ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
>> +static void		ath5k_disable_aspm(struct pci_dev *pdev, u16 state);
>
> Forward declarations should not be needed unless the functions are
> called before their implementations, which is not the case here.

Ok.

>
>> @@ -424,6 +427,47 @@ module_exit(exit_ath5k_pci);
>>  * PCI Initialization *
>>  \********************/
>>
>> +#ifdef CONFIG_PCIEASPM
>> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
>> +{
>> +	pci_disable_link_state(pdev, state);
>> +}
>> +#else
>> +static void __ath5k_disable_aspm(struct pci_dev *pdev, u16 state)
>> +{
>
> It looks like a replacement for pci_disable_link_state() if
> CONFIG_PCIEASPM is disabled.  I guess it should be in the PCI code, not
> in ath5k.  At least the PCI developers should have a look at the
> replacement code.

I used code from e1000e which does this same way, which now suddenly  
reminds me of that ath5k is dual lisenced, right? Can I even reuse  
code from GPL driver in ath5k?

-Jussi

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Pavel Roskin]

On Fri, 2010-05-28 at 21:25 +0300, Jussi Kivilinna wrote:
> I used code from e1000e which does this same way, which now suddenly  
> reminds me of that ath5k is dual lisenced, right? Can I even reuse  
> code from GPL driver in ath5k?

That's another reason why we don't want this code to be all over the
place.

If you could generalize the code sufficiently, it could be an inline
function in pci.h or some other GPL licensed header.

If we need to add GPL code to ath5k, it could go to a separate file.
But if that separation becomes inconvenient, we could drop BSD
compatibility from ath5k.  I don't see any benefit from dual licensing,
unless some existing ath5k contributors are BSD developers who would
stop working on the code in case of relicensing, but I don't think it's
the case.

-- 
Regards,
Pavel Roskin

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Bruno Randolf]

On Saturday 29 May 2010 05:27:56 Pavel Roskin wrote:
> If we need to add GPL code to ath5k, it could go to a separate file.
> But if that separation becomes inconvenient, we could drop BSD
> compatibility from ath5k.  I don't see any benefit from dual licensing,
> unless some existing ath5k contributors are BSD developers who would
> stop working on the code in case of relicensing, but I don't think it's
> the case.

afaik, one of the reasons for the dual license is that *BSDs (our ancestors 
from a ath5k point of view) should be able to benefit from improvements we 
make. this applies mostly to atheros chipset related things, and is irrelevant 
for linux-only implementation specifics.

also we can mix BSD and GPL in one file - see debug.c

bruno

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Luis R. Rodriguez]

Note: this e-mail thread is on a public mailing list.

Adding a few folks just for their information or in case they have
anything to add and It is also good to remind ourselves about best
practices for this stuff.

On Sun, May 30, 2010 at 6:06 PM, Bruno Randolf <br1@einfach.org> wrote:
> On Saturday 29 May 2010 05:27:56 Pavel Roskin wrote:
>> If we need to add GPL code to ath5k, it could go to a separate file.
>> But if that separation becomes inconvenient, we could drop BSD
>> compatibility from ath5k.  I don't see any benefit from dual licensing,
>> unless some existing ath5k contributors are BSD developers who would
>> stop working on the code in case of relicensing, but I don't think it's
>> the case.
>
> afaik, one of the reasons for the dual license is that *BSDs (our ancestors
> from a ath5k point of view) should be able to benefit from improvements we
> make.

Some clarifications on this.

Dual licensing GPL/BSD is typically done for the purpose of sharing
code between GPL codebases and permissive licensed codebases but I
should note that dual licensing GPL/BSD with this intention is legally
equivalent to just licensing under the BSD license. Also the
"Alternative" or "or" language used in dual licensing can sometimes be
left up to interpretation and confusion and for this same reason
SFLC's guidelines advise against this practice and instead recommend a
simple clear permissive license [1].

In ath9k's case all files are permissive licensed on purpose,
following SFLC's latest advice on this matter [1]. In ath5k only the
hardware files were *only* permissive licensed since those were the
files under review for sharing with OpenBSD. The other core driver
files have the dual license just because of historical purposes but
with the same intentions.

I should note though that when sharing permissive licensed files on a
larger GPL project it is also important to have in process a tag
mechanism to annotate when contributions made to a permissive licensed
file are indeed being kept under that same permissive licensed file.
In Linux this is accomplished by the Signed-off-by tag, which has as a
purpose stated under the 'Developer's Certificate of Origin 1.1':

------------------------------------------------------------------------------------------
        By making a contribution to this project, I certify that:

        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or
------------------------------------------------------------------------------------------

For ath5k initially we used to use a more implicit
Changes-licensed-under tag but after a while this practice faded off
in favor for educating more on the meaning behind the Singed-off-by
tag, as stated above. There are more subsystems where this code
sharing happens with permissive licensed code bases and sticking to
the simple SOB makes it easier.

> this applies mostly to atheros chipset related things, and is irrelevant
> for linux-only implementation specifics.

Actually the Linux kernel has other places where we care to share with
the other BSD families, another example I am aware of is ACPI code.
There may be others, but I am not too sure.

> also we can mix BSD and GPL in one file - see debug.c

No, once you have GPL a file the entire driver becomes GPL as a whole,
the final binary that is. The ath5k/debug.c file is GPL, and future
contributions would be made only as GPL then. If you want to separate
permissive licensed changes from GPL changes that technically is
possible but its just a pain in the ass and silly. For this reason I
recommend to keep GPL files separate from permissive files. But yes,
it is also possible to keep some GPL files and some permissive
licensed files for a driver and ath5k is good example of such a case:

  * GPL files (debug.c)
  * Dual licensing for historical purposes (core driver)
  * Clean simple permissive license (ISC license, on hardware code)

[1] http://www.softwarefreedom.org/resources/2007/gpl-non-gpl-collaboration.html

  Luis

Re: [PATCH] ath5k: disable ASPM

[Maxim Levitsky]

On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote: 
> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
> these problems.
> 
> Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> both ends (usually stalls within seconds).

This fixes the same nasty problem on my AR2425.

My AR2425 will stall if it transmits for about 1~2 minutes.

It sends storm of RXORN interrupts although it only transmits.

I see that now lspci calls it AR5001.

Jussi Kivilinna, million thanks to you. I would never think of going
this direction.

Luis, so I was right after all, wasn't I?
It is a hardware bug that is worked around in windows driver by
disabling PCIE ASPM L0S.

Jussi Kivilinna, thanks again, I really mean it.


Best regards,
Maxim Levitsky 

Re: [PATCH] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:

> On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote:
>> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
>> these problems.
>>
>> Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
>> both ends (usually stalls within seconds).
>
> This fixes the same nasty problem on my AR2425.
>
> My AR2425 will stall if it transmits for about 1~2 minutes.
>
> It sends storm of RXORN interrupts although it only transmits.
>
> I see that now lspci calls it AR5001.
>
> Jussi Kivilinna, million thanks to you. I would never think of going
> this direction.
>
> Luis, so I was right after all, wasn't I?
> It is a hardware bug that is worked around in windows driver by
> disabling PCIE ASPM L0S.

I noticed this L0s disabling in windows driver too. I cannot test this  
anymore, since I don't have ath5k hw installed anymore (I switched to  
b43).

(ok, I might switch back ath5k to work on this, but opening AAO is  
pain.. on the other hand, I'm just user in this case and pretty  
unwilling to work with dual-license)

I did test device with L0s+L1 enabled (aspm=force), on this setup  
device fails within seconds. I tested patch with disabling L1 but not  
L0s, still fails but after longer time. I did _not_ test with L0s off  
but L1 enabled. So maybe it would be worth to test this patch with  
just disabling L0s.

AAO150 seems to enable L0s from BIOS, so this happens without  
aspm=force or CONFIG_.._ASPM at all.

  -Jussi

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[RHS Linux User]

Hi All,

   Let's hear it for chips without available specs!

   Does anyone remember the days when paper specs arrived in plain
envelopes with no return address when you asked the manufacturer for some
real support?

   And lets hope they are REAL low in price!

   wiz


On Fri, 18 Jun 2010, Jussi Kivilinna wrote:

> Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:
> 
> > On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote:
> >> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> >> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> >> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> >> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
> >> these problems.
> >>
> >> Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> >> both ends (usually stalls within seconds).
> >
> > This fixes the same nasty problem on my AR2425.
> >
> > My AR2425 will stall if it transmits for about 1~2 minutes.
> >
> > It sends storm of RXORN interrupts although it only transmits.
> >
> > I see that now lspci calls it AR5001.
> >
> > Jussi Kivilinna, million thanks to you. I would never think of going
> > this direction.
> >
> > Luis, so I was right after all, wasn't I?
> > It is a hardware bug that is worked around in windows driver by
> > disabling PCIE ASPM L0S.
> 
> I noticed this L0s disabling in windows driver too. I cannot test this  
> anymore, since I don't have ath5k hw installed anymore (I switched to  
> b43).
> 
> (ok, I might switch back ath5k to work on this, but opening AAO is  
> pain.. on the other hand, I'm just user in this case and pretty  
> unwilling to work with dual-license)
> 
> I did test device with L0s+L1 enabled (aspm=force), on this setup  
> device fails within seconds. I tested patch with disabling L1 but not  
> L0s, still fails but after longer time. I did _not_ test with L0s off  
> but L1 enabled. So maybe it would be worth to test this patch with  
> just disabling L0s.
> 
> AAO150 seems to enable L0s from BIOS, so this happens without  
> aspm=force or CONFIG_.._ASPM at all.
> 
>   -Jussi
> 
> _______________________________________________
> ath5k-devel mailing list
> ath5k-devel@lists.ath5k.org
> https://lists.ath5k.org/mailman/listinfo/ath5k-devel
> 

Re: [PATCH] ath5k: disable ASPM

[Maxim Levitsky]

On Fri, 2010-06-18 at 11:20 +0300, Jussi Kivilinna wrote: 
> Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:
> 
> > On Fri, 2010-05-28 at 13:09 +0300, Jussi Kivilinna wrote:
> >> Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> >> Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> >> enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> >> 'unsupported jumbo' warnings appearing. Disabling ASPM L0s/L1 in ath5k fixes
> >> these problems.
> >>
> >> Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> >> both ends (usually stalls within seconds).
> >
> > This fixes the same nasty problem on my AR2425.
> >
> > My AR2425 will stall if it transmits for about 1~2 minutes.
> >
> > It sends storm of RXORN interrupts although it only transmits.
> >
> > I see that now lspci calls it AR5001.
> >
> > Jussi Kivilinna, million thanks to you. I would never think of going
> > this direction.
> >
> > Luis, so I was right after all, wasn't I?
> > It is a hardware bug that is worked around in windows driver by
> > disabling PCIE ASPM L0S.
> 
> I noticed this L0s disabling in windows driver too. I cannot test this  
> anymore, since I don't have ath5k hw installed anymore (I switched to  
> b43).
> 
> (ok, I might switch back ath5k to work on this, but opening AAO is  
> pain.. on the other hand, I'm just user in this case and pretty  
> unwilling to work with dual-license)
What do you mean?

> 
> I did test device with L0s+L1 enabled (aspm=force), on this setup  
> device fails within seconds. I tested patch with disabling L1 but not  
> L0s, still fails but after longer time. I did _not_ test with L0s off  
> but L1 enabled. So maybe it would be worth to test this patch with  
> just disabling L0s.
I did, and it works.

> 
> AAO150 seems to enable L0s from BIOS, so this happens without  
> aspm=force or CONFIG_.._ASPM at all.
Exactly.


Best regards,
Maxim Levitsky

Re: [PATCH] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:

>> (ok, I might switch back ath5k to work on this, but opening AAO is
>> pain.. on the other hand, I'm just user in this case and pretty
>> unwilling to work with dual-license)
> What do you mean?

Patch I made uses GPL code from e1000e, but since ath5k is  
dual-licensed so patch can't be accepted. So if I got it right, patch  
has to be remade from scratch by someone who really knows about pci  
registers etc. I don't, and learning this to fix something that is  
already fixed in my point of view is waste of (my) time.

>> I did test device with L0s+L1 enabled (aspm=force), on this setup
>> device fails within seconds. I tested patch with disabling L1 but not
>> L0s, still fails but after longer time. I did _not_ test with L0s off
>> but L1 enabled. So maybe it would be worth to test this patch with
>> just disabling L0s.
> I did, and it works.

Yeah, I noticed your reply at bugzilla (#13892).

  -Jussi

Re: [PATCH] ath5k: disable ASPM

[Maxim Levitsky]

On Fri, 2010-06-18 at 13:49 +0300, Jussi Kivilinna wrote: 
> Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:
> 
> >> (ok, I might switch back ath5k to work on this, but opening AAO is
> >> pain.. on the other hand, I'm just user in this case and pretty
> >> unwilling to work with dual-license)
> > What do you mean?
> 
> Patch I made uses GPL code from e1000e, but since ath5k is  
> dual-licensed so patch can't be accepted. So if I got it right, patch  
> has to be remade from scratch by someone who really knows about pci  
> registers etc. I don't, and learning this to fix something that is  
> already fixed in my point of view is waste of (my) time.
Sure, regardless of licensing, this patch has to be redone (and e1000
with it)

Best regards,
Maxim Levitsky

Re: [PATCH] ath5k: disable ASPM

[Bob Copeland]

On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> Patch I made uses GPL code from e1000e, but since ath5k is
>> dual-licensed so patch can't be accepted. So if I got it right, patch
>> has to be remade from scratch by someone who really knows about pci
>> registers etc. I don't, and learning this to fix something that is
>> already fixed in my point of view is waste of (my) time.
> Sure, regardless of licensing, this patch has to be redone (and e1000
> with it)

At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
beer, lots of bugs have been reported on these devices.

Maxim, this device was always broken in the same way, right?  Just
curious if anything made it worse recently.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: disable ASPM

[Maxim Levitsky]

On Fri, 2010-06-18 at 09:59 -0400, Bob Copeland wrote: 
> On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> >> Patch I made uses GPL code from e1000e, but since ath5k is
> >> dual-licensed so patch can't be accepted. So if I got it right, patch
> >> has to be remade from scratch by someone who really knows about pci
> >> registers etc. I don't, and learning this to fix something that is
> >> already fixed in my point of view is waste of (my) time.
> > Sure, regardless of licensing, this patch has to be redone (and e1000
> > with it)
> 
> At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
> beer, lots of bugs have been reported on these devices.
> 
> Maxim, this device was always broken in the same way, right?  Just
> curious if anything made it worse recently.

Always was broken, of course even with madwifi.

Recently I think driver stopped doing reset on RXORN, which sometimes
helped. This did made things a bit worse.

Anyway, just disable L0S, and card works perfectly.

Best regards,
Maxim Levitsky

[PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

On Fri, 2010-06-18 at 17:11 +0300, Maxim Levitsky wrote: 
> On Fri, 2010-06-18 at 09:59 -0400, Bob Copeland wrote: 
> > On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > >> Patch I made uses GPL code from e1000e, but since ath5k is
> > >> dual-licensed so patch can't be accepted. So if I got it right, patch
> > >> has to be remade from scratch by someone who really knows about pci
> > >> registers etc. I don't, and learning this to fix something that is
> > >> already fixed in my point of view is waste of (my) time.
> > > Sure, regardless of licensing, this patch has to be redone (and e1000
> > > with it)
> > 
> > At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
> > beer, lots of bugs have been reported on these devices.
> > 
> > Maxim, this device was always broken in the same way, right?  Just
> > curious if anything made it worse recently.
> 
> Always was broken, of course even with madwifi.
> 
> Recently I think driver stopped doing reset on RXORN, which sometimes
> helped. This did made things a bit worse.
> 
> Anyway, just disable L0S, and card works perfectly.

How this patch?
Its same patch but without open coded ASPM disabler.
Of course to work therefore you need CONFIG_PCIEASPM.
Without it, this reduces to noop.
However I asked at linux-pci, and they said that its not a bad idea to
just remove CONFIG_PCIEASPM and make it default.

I hope there won't be a silly GPL vs BSD debate over one line of code...


commit ac5de416f822917b927958b21186a82141550da7
Author: Maxim Levitsky <maximlevitsky@gmail.com>
Date:   Thu Jun 17 23:21:42 2010 +0300

    ath5k: disable ASPM
    
    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
    these problems.
    
    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
    both ends (usually stalls within seconds).
    
    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>


diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 3abbe75..e7a189a 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -48,6 +48,7 @@
 #include <linux/netdevice.h>
 #include <linux/cache.h>
 #include <linux/pci.h>
+#include <linux/pci-aspm.h>
 #include <linux/ethtool.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
@@ -469,6 +470,9 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	int ret;
 	u8 csz;
 
+	/* Disable PCIE ASPM L0S. It is never enabled by windows driver */
+	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "can't enable device\n");
@@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
 	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
 	struct ath5k_softc *sc = hw->priv;
 
+	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
+
 	/*
 	 * Suspend/Resume resets the PCI configuration space, so we have to
 	 * re-disable the RETRY_TIMEOUT register (0x41) to keep

Re: [PATCH v2] ath5k: disable ASPM

[Bob Copeland]

On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> How this patch?

Looks fine to me.  Some nitpicking below but feel free to add my

Acked-by: Bob Copeland <me@bobcopeland.com>

to this or a later version.

>     Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>

I believe maintaining the s-o-b is usually reserved for the case when the
patch only has minor edits; here your patch while same in spirit has
some 30 fewer lines.  Maybe should use "From:" or mention Jussi in the
commit log to maintain attribution instead?

> +	/* Disable PCIE ASPM L0S. It is never enabled by windows driver */
> +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> +

Would be good to expand this comment as to why (just copy another
sentence from the commit log or so).

>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "can't enable device\n");
> @@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
>  	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
>  	struct ath5k_softc *sc = hw->priv;
>  
> +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> +

Did you test that it did get lost over suspend/resume?  A glance at
pci_save_pcie_state seems to indicate it's saved.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > How this patch?
> 
> Looks fine to me.  Some nitpicking below but feel free to add my
> 
> Acked-by: Bob Copeland <me@bobcopeland.com>
> 
> to this or a later version.
> 
> >     Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> 
> I believe maintaining the s-o-b is usually reserved for the case when the
> patch only has minor edits; here your patch while same in spirit has
> some 30 fewer lines.  Maybe should use "From:" or mention Jussi in the
> commit log to maintain attribution instead?
OK


> 
> > +	/* Disable PCIE ASPM L0S. It is never enabled by windows driver */
> > +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > +
> 
> Would be good to expand this comment as to why (just copy another
> sentence from the commit log or so).
OK 
> 
> >  	ret = pci_enable_device(pdev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "can't enable device\n");
> > @@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
> >  	struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> >  	struct ath5k_softc *sc = hw->priv;
> >  
> > +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > +
> 
> Did you test that it did get lost over suspend/resume?  A glance at
> pci_save_pcie_state seems to indicate it's saved.
Will test.


Best regards,
Maxim Levitsky

[PATCH v3] ath5k: disable ASPM

[Maxim Levitsky]

On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > How this patch?
> > 
> > Looks fine to me.  Some nitpicking below but feel free to add my
> > 
> > Acked-by: Bob Copeland <me@bobcopeland.com>
> > 
Done.

Best regards,
Maxim Levitsky

--- 

commit 616afa397b3e843f2aba06be12a30e72dfff7740
Author: Maxim Levitsky <maximlevitsky@gmail.com>
Date:   Thu Jun 17 23:21:42 2010 +0300

    ath5k: disable ASPM
    
    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
    these problems.
    Also card sends a storm of RXORN interrupts even though medium is idle.
    
    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
    both ends (usually stalls within seconds).
    
    Unfortunately BIOS enables ASPM on this card by default on these machines
    This means that, problem shows up (less often) without pcie_aspm=force too. 
    Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
    

    All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    for finding and fixing this bug.
    
    Based on patch that is
    From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
    
    
    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
    Acked-by: Bob Copeland <me@bobcopeland.com>

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 3abbe75..4f6bd7c 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -48,6 +48,7 @@
 #include <linux/netdevice.h>
 #include <linux/cache.h>
 #include <linux/pci.h>
+#include <linux/pci-aspm.h>
 #include <linux/ethtool.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
@@ -469,6 +470,19 @@ ath5k_pci_probe(struct pci_dev *pdev,
 	int ret;
 	u8 csz;
 
+	/*
+	 * Disable PCIE ASPM L0S on the card.
+	 * ASPM triggers hardware bug, that makes card stall transmission
+	 * untill reset, and even that doesn't always help.
+	 * This happens on meduim to heavy transmit utilization.
+	 * In addition to stall, hardware usually gives a storm of
+	 * RXORN interrupts, despite idle channel, and otherwise doesn't work.
+	 * Windows driver also disables the L0s ASPM,
+	 * probably due to same reason
+	 * Note: to benefit from this fix, please _enable_ CONFIG_PCIEASPM
+	 */
+	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		dev_err(&pdev->dev, "can't enable device\n");

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

Note: this e-mail is on a public mailing list.

On Sat, Jun 19, 2010 at 12:49 AM, Maxim Levitsky
<maximlevitsky@gmail.com> wrote:
> On Fri, 2010-06-18 at 17:11 +0300, Maxim Levitsky wrote:
>> On Fri, 2010-06-18 at 09:59 -0400, Bob Copeland wrote:
>> > On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> > >> Patch I made uses GPL code from e1000e, but since ath5k is
>> > >> dual-licensed so patch can't be accepted. So if I got it right, patch
>> > >> has to be remade from scratch by someone who really knows about pci
>> > >> registers etc. I don't, and learning this to fix something that is
>> > >> already fixed in my point of view is waste of (my) time.
>> > > Sure, regardless of licensing, this patch has to be redone (and e1000
>> > > with it)
>> >
>> > At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
>> > beer, lots of bugs have been reported on these devices.
>> >
>> > Maxim, this device was always broken in the same way, right?  Just
>> > curious if anything made it worse recently.
>>
>> Always was broken, of course even with madwifi.
>>
>> Recently I think driver stopped doing reset on RXORN, which sometimes
>> helped. This did made things a bit worse.
>>
>> Anyway, just disable L0S, and card works perfectly.
>
> How this patch?
> Its same patch but without open coded ASPM disabler.
> Of course to work therefore you need CONFIG_PCIEASPM.
> Without it, this reduces to noop.
> However I asked at linux-pci, and they said that its not a bad idea to
> just remove CONFIG_PCIEASPM and make it default.
>
> I hope there won't be a silly GPL vs BSD debate over one line of code...

When you use the SOB you respect the license of the file you are
editing, please see Documentation/SubmittingPatches Developer's
Certificate of Origin 1.1.

> commit ac5de416f822917b927958b21186a82141550da7
> Author: Maxim Levitsky <maximlevitsky@gmail.com>
> Date:   Thu Jun 17 23:21:42 2010 +0300
>
>    ath5k: disable ASPM

You are not disabling ASPM, you are disabling L0s. ASPM can work with
L1, for example.

>    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
>    these problems.
>
>    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
>    both ends (usually stalls within seconds).

I *highly* discourage the use of pcie_aspm=force, in fact I'm inclined
to just remove this junk code from the kernel. What you should do to
test ASPM on a device is to use setpci on the config space. I have
documented how you can do this here:

http://wireless.kernel.org/en/users/Documentation/ASPM

Reason for discouraging this is when you use this you enable ASPM on
*all* root complexes and *all* devices which do support ASPM. If you
have another device which is capable of ASPM but has it disabled for
some reason you will run into other issues.

I should also note that loading a module already has an effect on
devices for ASPM. An example today is ath9k's ath9k_hw_init() which
runs simply during module load, this has some ASPM related code which
for example disables the PLL for ASPM for AR9003. I don't recall
exactly what we do with ath5k but just giving you an idea. To truly
test ASPM well I recommend to do something similar as with this script
or you can just give it a shot.

http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm

Not like I expect very different results but just wanted to clarify
the details on force aspm.

Why are you disabling L0s for all devices though? Why not just for the
reported device? Granted, L0s won't save you much more power but
still, why remove it completely, your commit log does not address that
in any way. It only states you have issues with L0s on one chipset but
what the patch really implies is you are disabling L0s completely for
all ath5k chipsets.

David, are you aware of recent L0s issues with some legacy cards?

>    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>
>
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 3abbe75..e7a189a 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -48,6 +48,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/cache.h>
>  #include <linux/pci.h>
> +#include <linux/pci-aspm.h>
>  #include <linux/ethtool.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> @@ -469,6 +470,9 @@ ath5k_pci_probe(struct pci_dev *pdev,
>        int ret;
>        u8 csz;
>
> +       /* Disable PCIE ASPM L0S. It is never enabled by windows driver */
> +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> +
>        ret = pci_enable_device(pdev);
>        if (ret) {
>                dev_err(&pdev->dev, "can't enable device\n");
> @@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
>        struct ieee80211_hw *hw = pci_get_drvdata(pdev);
>        struct ath5k_softc *sc = hw->priv;
>
> +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> +
>        /*
>         * Suspend/Resume resets the PCI configuration space, so we have to
>         * re-disable the RETRY_TIMEOUT register (0x41) to keep
>
>
>
>
>
>
> _______________________________________________
> ath5k-devel mailing list
> ath5k-devel@lists.ath5k.org
> https://lists.ath5k.org/mailman/listinfo/ath5k-devel
>

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

On Sun, 2010-06-20 at 01:13 -0700, Luis R. Rodriguez wrote: 
> Note: this e-mail is on a public mailing list.
> 
> On Sat, Jun 19, 2010 at 12:49 AM, Maxim Levitsky
> <maximlevitsky@gmail.com> wrote:
> > On Fri, 2010-06-18 at 17:11 +0300, Maxim Levitsky wrote:
> >> On Fri, 2010-06-18 at 09:59 -0400, Bob Copeland wrote:
> >> > On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> >> > >> Patch I made uses GPL code from e1000e, but since ath5k is
> >> > >> dual-licensed so patch can't be accepted. So if I got it right, patch
> >> > >> has to be remade from scratch by someone who really knows about pci
> >> > >> registers etc. I don't, and learning this to fix something that is
> >> > >> already fixed in my point of view is waste of (my) time.
> >> > > Sure, regardless of licensing, this patch has to be redone (and e1000
> >> > > with it)
> >> >
> >> > At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
> >> > beer, lots of bugs have been reported on these devices.
> >> >
> >> > Maxim, this device was always broken in the same way, right?  Just
> >> > curious if anything made it worse recently.
> >>
> >> Always was broken, of course even with madwifi.
> >>
> >> Recently I think driver stopped doing reset on RXORN, which sometimes
> >> helped. This did made things a bit worse.
> >>
> >> Anyway, just disable L0S, and card works perfectly.
> >
> > How this patch?
> > Its same patch but without open coded ASPM disabler.
> > Of course to work therefore you need CONFIG_PCIEASPM.
> > Without it, this reduces to noop.
> > However I asked at linux-pci, and they said that its not a bad idea to
> > just remove CONFIG_PCIEASPM and make it default.
> >
> > I hope there won't be a silly GPL vs BSD debate over one line of code...
> 
> When you use the SOB you respect the license of the file you are
> editing, please see Documentation/SubmittingPatches Developer's
> Certificate of Origin 1.1.
This is ok of course.
The debate was about Jussi Kivilinna copying ASPM disabler code from
e1000 which is GPL.

> 
> > commit ac5de416f822917b927958b21186a82141550da7
> > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > Date:   Thu Jun 17 23:21:42 2010 +0300
> >
> >    ath5k: disable ASPM
> 
> You are not disabling ASPM, you are disabling L0s. ASPM can work with
> L1, for example.
This is left over from original patch.
with open coded code I was able to disable just L0s and get stable
operation.
Note however that with this patch which implies CONFIG_PCIEASPM, pci
core disables both L0s and L1
(I still need to test and see if I need that patch at all. Maybe just
enabling CONFIG_PCIEASPM is enough...)


> 
> >    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> >    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> >    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> >    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> >    these problems.
> >
> >    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> >    both ends (usually stalls within seconds).
> 
> I *highly* discourage the use of pcie_aspm=force, in fact I'm inclined
> to just remove this junk code from the kernel. What you should do to
> test ASPM on a device is to use setpci on the config space. I have
> documented how you can do this here:

> 
> http://wireless.kernel.org/en/users/Documentation/ASPM
> 
> Reason for discouraging this is when you use this you enable ASPM on
> *all* root complexes and *all* devices which do support ASPM. If you
> have another device which is capable of ASPM but has it disabled for
> some reason you will run into other issues.
> 
> I should also note that loading a module already has an effect on
> devices for ASPM. An example today is ath9k's ath9k_hw_init() which
> runs simply during module load, this has some ASPM related code which
> for example disables the PLL for ASPM for AR9003. I don't recall
> exactly what we do with ath5k but just giving you an idea. To truly
> test ASPM well I recommend to do something similar as with this script
> or you can just give it a shot.
> 
> http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
> 
> Not like I expect very different results but just wanted to clarify
> the details on force aspm.
> 
> Why are you disabling L0s for all devices though? Why not just for the
> reported device? Granted, L0s won't save you much more power but
> still, why remove it completely, your commit log does not address that
> in any way. It only states you have issues with L0s on one chipset but
> what the patch really implies is you are disabling L0s completely for
> all ath5k chipsets.
First of all there aren't many PCIE ath5k based devices.
Two of them are known to be broken.
Also Jussi Kivilinna said that he found that in windows .inf file there
are some instructions to enable L1 but not L0s.


Note that I tested that again, and card works very stable.
I didn't see a single drop to 0 bytes/s. In fact throughput never drops
below 1 Mb/s. (usually about 2.4 Mb/s, with rare drops for few seconds
to ~1Mb/s)


Best regards,
Maxim Levitsky

> 
> David, are you aware of recent L0s issues with some legacy cards?
> 
> >    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> >
> >
> > diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> > index 3abbe75..e7a189a 100644
> > --- a/drivers/net/wireless/ath/ath5k/base.c
> > +++ b/drivers/net/wireless/ath/ath5k/base.c
> > @@ -48,6 +48,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/cache.h>
> >  #include <linux/pci.h>
> > +#include <linux/pci-aspm.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/slab.h>
> > @@ -469,6 +470,9 @@ ath5k_pci_probe(struct pci_dev *pdev,
> >        int ret;
> >        u8 csz;
> >
> > +       /* Disable PCIE ASPM L0S. It is never enabled by windows driver */
> > +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > +
> >        ret = pci_enable_device(pdev);
> >        if (ret) {
> >                dev_err(&pdev->dev, "can't enable device\n");
> > @@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
> >        struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> >        struct ath5k_softc *sc = hw->priv;
> >
> > +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > +
> >        /*
> >         * Suspend/Resume resets the PCI configuration space, so we have to
> >         * re-disable the RETRY_TIMEOUT register (0x41) to keep
> >
> >
> >
> >
> >
> >
> > _______________________________________________
> > ath5k-devel mailing list
> > ath5k-devel@lists.ath5k.org
> > https://lists.ath5k.org/mailman/listinfo/ath5k-devel
> >

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

On Sun, 2010-06-20 at 14:18 +0300, Maxim Levitsky wrote: 
> On Sun, 2010-06-20 at 01:13 -0700, Luis R. Rodriguez wrote: 
> > Note: this e-mail is on a public mailing list.
> > 
> > On Sat, Jun 19, 2010 at 12:49 AM, Maxim Levitsky
> > <maximlevitsky@gmail.com> wrote:
> > > On Fri, 2010-06-18 at 17:11 +0300, Maxim Levitsky wrote:
> > >> On Fri, 2010-06-18 at 09:59 -0400, Bob Copeland wrote:
> > >> > On Fri, Jun 18, 2010 at 7:05 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > >> > >> Patch I made uses GPL code from e1000e, but since ath5k is
> > >> > >> dual-licensed so patch can't be accepted. So if I got it right, patch
> > >> > >> has to be remade from scratch by someone who really knows about pci
> > >> > >> registers etc. I don't, and learning this to fix something that is
> > >> > >> already fixed in my point of view is waste of (my) time.
> > >> > > Sure, regardless of licensing, this patch has to be redone (and e1000
> > >> > > with it)
> > >> >
> > >> > At any rate, Jussi, thanks a bundle for tracking it down.  I owe you a
> > >> > beer, lots of bugs have been reported on these devices.
> > >> >
> > >> > Maxim, this device was always broken in the same way, right?  Just
> > >> > curious if anything made it worse recently.
> > >>
> > >> Always was broken, of course even with madwifi.
> > >>
> > >> Recently I think driver stopped doing reset on RXORN, which sometimes
> > >> helped. This did made things a bit worse.
> > >>
> > >> Anyway, just disable L0S, and card works perfectly.
> > >
> > > How this patch?
> > > Its same patch but without open coded ASPM disabler.
> > > Of course to work therefore you need CONFIG_PCIEASPM.
> > > Without it, this reduces to noop.
> > > However I asked at linux-pci, and they said that its not a bad idea to
> > > just remove CONFIG_PCIEASPM and make it default.
> > >
> > > I hope there won't be a silly GPL vs BSD debate over one line of code...
> > 
> > When you use the SOB you respect the license of the file you are
> > editing, please see Documentation/SubmittingPatches Developer's
> > Certificate of Origin 1.1.
> This is ok of course.
> The debate was about Jussi Kivilinna copying ASPM disabler code from
> e1000 which is GPL.
> 
> > 
> > > commit ac5de416f822917b927958b21186a82141550da7
> > > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > > Date:   Thu Jun 17 23:21:42 2010 +0300
> > >
> > >    ath5k: disable ASPM
> > 
> > You are not disabling ASPM, you are disabling L0s. ASPM can work with
> > L1, for example.
> This is left over from original patch.
> with open coded code I was able to disable just L0s and get stable
> operation.
> Note however that with this patch which implies CONFIG_PCIEASPM, pci
> core disables both L0s and L1
> (I still need to test and see if I need that patch at all. Maybe just
> enabling CONFIG_PCIEASPM is enough...)
Yep, the patch isn't needed, because of this:

pci 0000:03:00.0: disabling ASPM on pre-1.1 PCIe device.  You can enable it with 'pcie_aspm=force'


03:00.0 Ethernet controller: Atheros Communications Inc. AR5001 Wireless Network Adapter (rev 01)
	Subsystem: Foxconn International, Inc. Device e008
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0, Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 18
	Region 0: Memory at 55200000 (64-bit, non-prefetchable) [size=64K]
	Capabilities: [40] Power Management version 2
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=375mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] Message Signalled Interrupts: Mask- 64bit- Queue=0/0 Enable-
		Address: 00000000  Data: 0000
	Capabilities: [60] Express (v1) Legacy Endpoint, MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0, Latency L0s <512ns, L1 <64us
			ExtTag- AttnBtn- AttnInd- PwrInd- RBE- FLReset-
		DevCtl:	Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop-
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr+ UncorrErr+ FatalErr- UnsuppReq+ AuxPwr- TransPend-
		LnkCap:	Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <512ns, L1 <64us
			ClockPM- Suprise- LLActRep- BwNot-
		LnkCtl:	ASPM Disabled; RCB 128 bytes Disabled- Retrain- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-
	Capabilities: [90] MSI-X: Enable- Mask- TabSize=1
		Vector table: BAR=0 offset=00000000
		PBA: BAR=0 offset=00000000
	Capabilities: [100] Advanced Error Reporting <?>
	Capabilities: [140] Virtual Channel <?>
	Kernel driver in use: ath5k
	Kernel modules: ath5k


static int pcie_aspm_sanity_check(struct pci_dev *pdev)
{
	struct pci_dev *child;
	int pos;
	u32 reg32;
	/*
	 * Some functions in a slot might not all be PCIe functions,
	 * very strange. Disable ASPM for the whole slot
	 */
	list_for_each_entry(child, &pdev->subordinate->devices, bus_list) {
		pos = pci_pcie_cap(child);
		if (!pos)
			return -EINVAL;
		/*
		 * Disable ASPM for pre-1.1 PCIe device, we follow MS to use
		 * RBER bit to determine if a function is 1.1 version device
		 */
		pci_read_config_dword(child, pos + PCI_EXP_DEVCAP, &reg32);
		if (!(reg32 & PCI_EXP_DEVCAP_RBER) && !aspm_force) {
			dev_printk(KERN_INFO, &child->dev, "disabling ASPM"
				" on pre-1.1 PCIe device.  You can enable it"
				" with 'pcie_aspm=force'\n");
			return -EINVAL;
		}
	}
	return 0;
}


If MS=Microsoft, that explains it, so windows just disables ASPM on pre 1.1 PCIE devices....

It still is a hardware bug that Atheros device advertises ASPM support, but it is broken.

Best regards,
	Maxim Levitsky

> 
> 
> > 
> > >    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> > >    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> > >    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> > >    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> > >    these problems.
> > >
> > >    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> > >    both ends (usually stalls within seconds).
> > 
> > I *highly* discourage the use of pcie_aspm=force, in fact I'm inclined
> > to just remove this junk code from the kernel. What you should do to
> > test ASPM on a device is to use setpci on the config space. I have
> > documented how you can do this here:
> 
> > 
> > http://wireless.kernel.org/en/users/Documentation/ASPM
> > 
> > Reason for discouraging this is when you use this you enable ASPM on
> > *all* root complexes and *all* devices which do support ASPM. If you
> > have another device which is capable of ASPM but has it disabled for
> > some reason you will run into other issues.
> > 
> > I should also note that loading a module already has an effect on
> > devices for ASPM. An example today is ath9k's ath9k_hw_init() which
> > runs simply during module load, this has some ASPM related code which
> > for example disables the PLL for ASPM for AR9003. I don't recall
> > exactly what we do with ath5k but just giving you an idea. To truly
> > test ASPM well I recommend to do something similar as with this script
> > or you can just give it a shot.
> > 
> > http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
> > 
> > Not like I expect very different results but just wanted to clarify
> > the details on force aspm.
> > 
> > Why are you disabling L0s for all devices though? Why not just for the
> > reported device? Granted, L0s won't save you much more power but
> > still, why remove it completely, your commit log does not address that
> > in any way. It only states you have issues with L0s on one chipset but
> > what the patch really implies is you are disabling L0s completely for
> > all ath5k chipsets.
> First of all there aren't many PCIE ath5k based devices.
> Two of them are known to be broken.
> Also Jussi Kivilinna said that he found that in windows .inf file there
> are some instructions to enable L1 but not L0s.
> 
> 
> Note that I tested that again, and card works very stable.
> I didn't see a single drop to 0 bytes/s. In fact throughput never drops
> below 1 Mb/s. (usually about 2.4 Mb/s, with rare drops for few seconds
> to ~1Mb/s)
> 
> 
> Best regards,
> Maxim Levitsky
> 
> > 
> > David, are you aware of recent L0s issues with some legacy cards?
> > 
> > >    Signed-off-by: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> > >    Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > >
> > >
> > > diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> > > index 3abbe75..e7a189a 100644
> > > --- a/drivers/net/wireless/ath/ath5k/base.c
> > > +++ b/drivers/net/wireless/ath/ath5k/base.c
> > > @@ -48,6 +48,7 @@
> > >  #include <linux/netdevice.h>
> > >  #include <linux/cache.h>
> > >  #include <linux/pci.h>
> > > +#include <linux/pci-aspm.h>
> > >  #include <linux/ethtool.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/slab.h>
> > > @@ -469,6 +470,9 @@ ath5k_pci_probe(struct pci_dev *pdev,
> > >        int ret;
> > >        u8 csz;
> > >
> > > +       /* Disable PCIE ASPM L0S. It is never enabled by windows driver */
> > > +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > > +
> > >        ret = pci_enable_device(pdev);
> > >        if (ret) {
> > >                dev_err(&pdev->dev, "can't enable device\n");
> > > @@ -722,6 +726,8 @@ static int ath5k_pci_resume(struct device *dev)
> > >        struct ieee80211_hw *hw = pci_get_drvdata(pdev);
> > >        struct ath5k_softc *sc = hw->priv;
> > >
> > > +       pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > > +
> > >        /*
> > >         * Suspend/Resume resets the PCI configuration space, so we have to
> > >         * re-disable the RETRY_TIMEOUT register (0x41) to keep
> > >
> > >
> > >
> > >
> > >
> > >
> > > _______________________________________________
> > > ath5k-devel mailing list
> > > ath5k-devel@lists.ath5k.org
> > > https://lists.ath5k.org/mailman/listinfo/ath5k-devel
> > >
> 
> 

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Sun, Jun 20, 2010 at 4:18 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Sun, 2010-06-20 at 01:13 -0700, Luis R. Rodriguez wrote:
>> > commit ac5de416f822917b927958b21186a82141550da7
>> > Author: Maxim Levitsky <maximlevitsky@gmail.com>
>> > Date:   Thu Jun 17 23:21:42 2010 +0300
>> >
>> >    ath5k: disable ASPM
>>
>> You are not disabling ASPM, you are disabling L0s. ASPM can work with
>> L1, for example.
> This is left over from original patch.

Your latest patch still has all this old info, please adjust.

> with open coded code I was able to disable just L0s and get stable
> operation.
> Note however that with this patch which implies CONFIG_PCIEASPM, pci
> core disables both L0s and L1

You do not need CONFIG_PCIEASPM to use ASPM on Linux, that is another
thing I have been meaning to clarify upstream but haven't had time to
do so yet.

> (I still need to test and see if I need that patch at all. Maybe just
> enabling CONFIG_PCIEASPM is enough...)

You should be able to test ASPM by just enabling the driver.

>> >    Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>> >    Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>> >    enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>> >    'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
>> >    these problems.
>> >
>> >    Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
>> >    both ends (usually stalls within seconds).
>>
>> I *highly* discourage the use of pcie_aspm=force, in fact I'm inclined
>> to just remove this junk code from the kernel. What you should do to
>> test ASPM on a device is to use setpci on the config space. I have
>> documented how you can do this here:
>>
>> http://wireless.kernel.org/en/users/Documentation/ASPM
>>
>> Reason for discouraging this is when you use this you enable ASPM on
>> *all* root complexes and *all* devices which do support ASPM. If you
>> have another device which is capable of ASPM but has it disabled for
>> some reason you will run into other issues.
>>
>> I should also note that loading a module already has an effect on
>> devices for ASPM. An example today is ath9k's ath9k_hw_init() which
>> runs simply during module load, this has some ASPM related code which
>> for example disables the PLL for ASPM for AR9003. I don't recall
>> exactly what we do with ath5k but just giving you an idea. To truly
>> test ASPM well I recommend to do something similar as with this script
>> or you can just give it a shot.
>>
>> http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
>>
>> Not like I expect very different results but just wanted to clarify
>> the details on force aspm.
>>
>> Why are you disabling L0s for all devices though? Why not just for the
>> reported device? Granted, L0s won't save you much more power but
>> still, why remove it completely, your commit log does not address that
>> in any way. It only states you have issues with L0s on one chipset but
>> what the patch really implies is you are disabling L0s completely for
>> all ath5k chipsets.
>
> First of all there aren't many PCIE ath5k based devices.

Doesn't matter.

> Two of them are known to be broken.

Which ones?

> Also Jussi Kivilinna said that he found that in windows .inf file there
> are some instructions to enable L1 but not L0s.

For which chipsets?

> Note that I tested that again, and card works very stable.

Thanks for your work on this, I want to just check internally if there
is another way, that's all.

> I didn't see a single drop to 0 bytes/s. In fact throughput never drops
> below 1 Mb/s. (usually about 2.4 Mb/s, with rare drops for few seconds
> to ~1Mb/s)

Disabling L0s should not affect performance.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Luis R. Rodriguez" <mcgrof@gmail.com>:

>> Also Jussi Kivilinna said that he found that in windows .inf file there
>> are some instructions to enable L1 but not L0s.
>
> For which chipsets?

I uploaded windows driver I have to:
   
http://www.student.oulu.fi/~jukivili/ath5k/XP32_XP64_WHQL_Dri-7-6-1-149_ACU-7-0-2-48_LEAP_Acer_81024

In inf, all ASPM entries are either ASPM off or ASPM L0s:off/L1:on.  
There is also user option for changing ASPM setting with options  
L0s:off/L1:off and L0s:off/L1:on.

-Jussi

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

Luis, let me explain again, exactly the situation:

First of all AR5001 and AR5001X devices (former was usualy listed as
AR2425, and I have it, later I don't know about much), don't work well
with ASPM L0s enabled.

I told that many times, but I tell again.
As soon as card it put on medium to high transmit load
(for example even if transmission consists mostly of TCP ACK packets),
it dies.

Usualy it will stop transmitting, and then after few seconds it will
send RXORN intrrupt to the host, even though the channel was idle.
(Tested by sending a stream of UDP packets on channel that is neighbor
free).

I didn't see it, but some users reported seeing jumbo frames at this
time as well. 
Overall it doesn't matter because card just goes south.

A reset sometimes brings card to life, sometimes causes another storm of
RXORN and sometimes just results in quiet dead card.
A next reset might bring it to life, or not.

Card (at least mine) advertises its as a 'pre pci 1.1 device'.
Therefore if I enable CONFIG_PCIEASPM, the pci core will automaticly
disable ASPM (both L0s and L1) on this card.
I won't be surprised that windows does the same.


Therefore the patch I sent it useless because it only works when
CONFIG_PCIEASPM is enabled.

In addition to that using the original version of this patch from Jussi
Kivilinna contains code that disables ASPM (I modified it to disable
only L0s) with or without CONFIG_PCIEASPM.

But without CONFIG_PCIEASPM it uses open coded function that should
belong to pci core.
And it is also copied from e1000 which shouldn't do that too.

Best regards,
Maxim Levitsky

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Jussi Kivilinna]

Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:

> Card (at least mine) advertises its as a 'pre pci 1.1 device'.
> Therefore if I enable CONFIG_PCIEASPM, the pci core will automaticly
> disable ASPM (both L0s and L1) on this card.
> I won't be surprised that windows does the same.
>

Even if CONFIG_PCIEASPM compiled in, ASPM code can be disabled by user  
with pcie_aspm=off option, leaving BIOS-enabled L0s on.

-Jussi

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jun 21, 2010 at 1:16 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> Luis, let me explain again, exactly the situation:
>
> First of all AR5001 and AR5001X devices (former was usualy listed as
> AR2425, and I have it, later I don't know about much), don't work well
> with ASPM L0s enabled.

Thanks for the clarification. David, do you see this as well on your end?

> I told that many times, but I tell again.
> As soon as card it put on medium to high transmit load
> (for example even if transmission consists mostly of TCP ACK packets),
> it dies.
>
> Usualy it will stop transmitting, and then after few seconds it will
> send RXORN intrrupt to the host, even though the channel was idle.
> (Tested by sending a stream of UDP packets on channel that is neighbor
> free).
>
> I didn't see it, but some users reported seeing jumbo frames at this
> time as well.
> Overall it doesn't matter because card just goes south.
>
> A reset sometimes brings card to life, sometimes causes another storm of
> RXORN and sometimes just results in quiet dead card.
> A next reset might bring it to life, or not.
>
> Card (at least mine) advertises its as a 'pre pci 1.1 device'.
> Therefore if I enable CONFIG_PCIEASPM, the pci core will automaticly
> disable ASPM (both L0s and L1) on this card.
> I won't be surprised that windows does the same.

I am not sure when L0s was enabled as per the spec as mandatory, I
thought it was optional. Interesting to hear this behavior on Linux
with CONFIG_PCIEASPM. That Kconfig description and code really need
some spit shining.

> Therefore the patch I sent it useless because it only works when
> CONFIG_PCIEASPM is enabled.

How so ? Your patch disables L0s, I can use ASPM with L0s and L1 with
or without CONFIG_PCIEASPM. If I added your calls to ath9k I would
disable L0s.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jun 21, 2010 at 1:33 PM, Jussi Kivilinna
<jussi.kivilinna@mbnet.fi> wrote:
> Quoting "Maxim Levitsky" <maximlevitsky@gmail.com>:
>
>> Card (at least mine) advertises its as a 'pre pci 1.1 device'.
>> Therefore if I enable CONFIG_PCIEASPM, the pci core will automaticly
>> disable ASPM (both L0s and L1) on this card.
>> I won't be surprised that windows does the same.
>>
>
> Even if CONFIG_PCIEASPM compiled in, ASPM code can be disabled by user with
> pcie_aspm=off option, leaving BIOS-enabled L0s on.

Last I reviewed CONFIG_PCIEASPM won't buy you *anything* other than
debugging knobs. With it you can force all devices to enable ASPM
completely on or disable it. Both of which I think are not really
useful and instead should be done in userspace given that if you are
testing ASPM you likely want to test only one one device and its
respective root complex, not all at the same time.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Maxim Levitsky]

On Mon, 2010-06-21 at 13:37 -0700, Luis R. Rodriguez wrote: 
> On Mon, Jun 21, 2010 at 1:16 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > Luis, let me explain again, exactly the situation:
> >
> > First of all AR5001 and AR5001X devices (former was usualy listed as
> > AR2425, and I have it, later I don't know about much), don't work well
> > with ASPM L0s enabled.
> 
> Thanks for the clarification. David, do you see this as well on your end?
> 
> > I told that many times, but I tell again.
> > As soon as card it put on medium to high transmit load
> > (for example even if transmission consists mostly of TCP ACK packets),
> > it dies.
> >
> > Usualy it will stop transmitting, and then after few seconds it will
> > send RXORN intrrupt to the host, even though the channel was idle.
> > (Tested by sending a stream of UDP packets on channel that is neighbor
> > free).
> >
> > I didn't see it, but some users reported seeing jumbo frames at this
> > time as well.
> > Overall it doesn't matter because card just goes south.
> >
> > A reset sometimes brings card to life, sometimes causes another storm of
> > RXORN and sometimes just results in quiet dead card.
> > A next reset might bring it to life, or not.
> >
> > Card (at least mine) advertises its as a 'pre pci 1.1 device'.
> > Therefore if I enable CONFIG_PCIEASPM, the pci core will automaticly
> > disable ASPM (both L0s and L1) on this card.
> > I won't be surprised that windows does the same.
> 
> I am not sure when L0s was enabled as per the spec as mandatory, I
> thought it was optional. Interesting to hear this behavior on Linux
> with CONFIG_PCIEASPM. That Kconfig description and code really need
> some spit shining.
> 
> > Therefore the patch I sent it useless because it only works when
> > CONFIG_PCIEASPM is enabled.
> 
> How so ? Your patch disables L0s, I can use ASPM with L0s and L1 with
> or without CONFIG_PCIEASPM. If I added your calls to ath9k I would
> disable L0s.
Because due to card advertisement, CONFIG_PCIEASPM already disables L0s
and L1 on the card, therefore my patch does nothing.



Best regards,
Maxim Levitsky

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jun 21, 2010 at 01:39:07PM -0700, Luis R. Rodriguez wrote:
> Last I reviewed CONFIG_PCIEASPM won't buy you *anything* other than
> debugging knobs. With it you can force all devices to enable ASPM
> completely on or disable it. Both of which I think are not really
> useful and instead should be done in userspace given that if you are
> testing ASPM you likely want to test only one one device and its
> respective root complex, not all at the same time.

It buys you enabling of ASPM on devices that the BIOS hasn't configured, 
which is legitimate and useful.

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

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 9:31 AM, Matthew Garrett <mjg@redhat.com> wrote:
> On Mon, Jun 21, 2010 at 01:39:07PM -0700, Luis R. Rodriguez wrote:
>> Last I reviewed CONFIG_PCIEASPM won't buy you *anything* other than
>> debugging knobs. With it you can force all devices to enable ASPM
>> completely on or disable it. Both of which I think are not really
>> useful and instead should be done in userspace given that if you are
>> testing ASPM you likely want to test only one one device and its
>> respective root complex, not all at the same time.
>
> It buys you enabling of ASPM on devices that the BIOS hasn't configured,
> which is legitimate and useful.

Sure, I agree with that, but it also will enable ASPM for *all*
devices which have the capability which IMHO is a terrible idea for
users when all they want to do is enable ASPM for one device. Instead
I recommend users to enable ASPM for their devices selectively and
from userspace.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Matthew Garrett]

On Tue, Jun 22, 2010 at 09:48:40AM -0700, Luis R. Rodriguez wrote:

> Sure, I agree with that, but it also will enable ASPM for *all*
> devices which have the capability which IMHO is a terrible idea for
> users when all they want to do is enable ASPM for one device. Instead
> I recommend users to enable ASPM for their devices selectively and
> from userspace.

Why would you only want to enable ASPM for one device?

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

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 9:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jun 22, 2010 at 09:48:40AM -0700, Luis R. Rodriguez wrote:
>
>> Sure, I agree with that, but it also will enable ASPM for *all*
>> devices which have the capability which IMHO is a terrible idea for
>> users when all they want to do is enable ASPM for one device. Instead
>> I recommend users to enable ASPM for their devices selectively and
>> from userspace.
>
> Why would you only want to enable ASPM for one device?

ASPM doesn't always work for all devices even if they do advertise
ASPM capability so turning it on selectively by device is what I
recommend since otherwise you may get hangs and you will then have to
do the selective enabling. Furthermore laptops tend to disable ASPM
for cards not built-in to it, an example is Cardbus slots or internal
PCI-E slots. This is often done because to enable ASPM for some cards
you often need to tune the host controller in addition to enabling
ASPM for the endpoint, so this will vary depending on vendor, chipset,
and host controller combination. This is documentation that the OEM /
ODM typically end up getting, but not end users.

So given the complexity its best to be selective about it on each
platform until you verify ASPM works well for all devices present.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Matthew Garrett]

On Tue, Jun 22, 2010 at 10:17:11AM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 9:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Why would you only want to enable ASPM for one device?
> 
> ASPM doesn't always work for all devices even if they do advertise
> ASPM capability so turning it on selectively by device is what I
> recommend since otherwise you may get hangs and you will then have to
> do the selective enabling.

Right, which we have to deal with by having drivers disable ASPM on 
broken devices.

> Furthermore laptops tend to disable ASPM for cards not built-in to it, 
> an example is Cardbus slots or internal PCI-E slots. This is often 
> done because to enable ASPM for some cards you often need to tune the 
> host controller in addition to enabling ASPM for the endpoint, so this 
> will vary depending on vendor, chipset, and host controller 
> combination. This is documentation that the OEM / ODM typically end up 
> getting, but not end users.

Having looked into this, Windows will enable ASPM on external 
controllers unless there's some reason for it not to - where that may be 
either the appropriate bit in the FADT being set, the device not being 
PCIe 1.1 or later, there being no _OSC method on the appropriate root 
bridge or the _OSC method not giving it full control over PCIe, the 
driver disabling ASPM or the device not advertising it in the first 
place. Are you aware of any other cases where Windows will refuse to 
enable ASPM?
 
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 10:25 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jun 22, 2010 at 10:17:11AM -0700, Luis R. Rodriguez wrote:
>> On Tue, Jun 22, 2010 at 9:52 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > Why would you only want to enable ASPM for one device?
>>
>> ASPM doesn't always work for all devices even if they do advertise
>> ASPM capability so turning it on selectively by device is what I
>> recommend since otherwise you may get hangs and you will then have to
>> do the selective enabling.
>
> Right, which we have to deal with by having drivers disable ASPM on
> broken devices.

Agreed, but then the assumption would be drivers are ASPM bug free
which is expect to be false with Video and 802.11 given that only a
handful of vendors do actually get involved with their drivers
upstream. Safe thing of course is to just disable it, of course, but
if you are going to use pcie_aspm=force good luck!

>> Furthermore laptops tend to disable ASPM for cards not built-in to it,
>> an example is Cardbus slots or internal PCI-E slots. This is often
>> done because to enable ASPM for some cards you often need to tune the
>> host controller in addition to enabling ASPM for the endpoint, so this
>> will vary depending on vendor, chipset, and host controller
>> combination. This is documentation that the OEM / ODM typically end up
>> getting, but not end users.
>
> Having looked into this, Windows will enable ASPM on external
> controllers unless there's some reason for it not to - where that may be
> either the appropriate bit in the FADT being set, the device not being
> PCIe 1.1 or later, there being no _OSC method on the appropriate root
> bridge or the _OSC method not giving it full control over PCIe, the
> driver disabling ASPM or the device not advertising it in the first
> place.

I was unaware of all this root complex sanity checks on Windows,
thanks for sharing.

> Are you aware of any other cases where Windows will refuse to
> enable ASPM?

My point was not whether or not ASPM typically got enabled on Windows
Vs Linux, my point was more of the fact that for some endpoint devices
you may have to tweak the root complex to get ASPM properly working
and that these tweaks *are* implemented on the BIOS by the ODM / OEM
for those devices and that the documentation for such tweaks is not
typically public. So, if you are like me and cannot stand the internal
802.11 card on your laptop and want to replace it with something else
you are stuck to hoping such BIOS tweaks are either not required or
figuring out what the tweaks are yourself and doing them through
userspace for the root complex *prior* to enabling ASPM through
userspace as well for the endpoint.

I suspect these tweaks will go away as the industry produces cards
with both L1 and L0s enabled all the time (devices being produced
today), but for devices caught in that middle of time between whether
or not L0s would be *required*  (last 2 years) I suspect we'll run
into these issues.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Matthew Garrett]

On Tue, Jun 22, 2010 at 10:40:15AM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 10:25 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Right, which we have to deal with by having drivers disable ASPM on
> > broken devices.
> 
> Agreed, but then the assumption would be drivers are ASPM bug free
> which is expect to be false with Video and 802.11 given that only a
> handful of vendors do actually get involved with their drivers
> upstream. Safe thing of course is to just disable it, of course, but
> if you are going to use pcie_aspm=force good luck!

People who use "force" deserve whatever they get, but "powersave" really 
ought to work. Fedora's defaulted to that for a while now - we've hit 
issues with aacraid, but that's pretty much it in terms of cases where 
the heuristics don't work. Maxim's problems wouldn't be triggered 
because CONFIG_PCIE_ASPM disables it on pre-1.1 devices regardless of 
the BIOS setup.

> > Having looked into this, Windows will enable ASPM on external
> > controllers unless there's some reason for it not to - where that may be
> > either the appropriate bit in the FADT being set, the device not being
> > PCIe 1.1 or later, there being no _OSC method on the appropriate root
> > bridge or the _OSC method not giving it full control over PCIe, the
> > driver disabling ASPM or the device not advertising it in the first
> > place.
> 
> I was unaware of all this root complex sanity checks on Windows,
> thanks for sharing.

With the patch I've just sent, they should also all be used for Linux as 
well.

> I suspect these tweaks will go away as the industry produces cards
> with both L1 and L0s enabled all the time (devices being produced
> today), but for devices caught in that middle of time between whether
> or not L0s would be *required*  (last 2 years) I suspect we'll run
> into these issues.

If the same problems would appear under Windows then it's not a problem 
that I'm hugely concerned about as yet - we'll wait a bit longer and 
then change the ASPM defaults to be more aggressive under Linux, and if 
it turns out to be a significant problem in the real world we'll have to 
reconsider it. But I don't think we should be depending on userspace 
bashing hardware registers in order to be able to enable power 
management.

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

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 10:50 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jun 22, 2010 at 10:40:15AM -0700, Luis R. Rodriguez wrote:
>> On Tue, Jun 22, 2010 at 10:25 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > Right, which we have to deal with by having drivers disable ASPM on
>> > broken devices.
>>
>> Agreed, but then the assumption would be drivers are ASPM bug free
>> which is expect to be false with Video and 802.11 given that only a
>> handful of vendors do actually get involved with their drivers
>> upstream. Safe thing of course is to just disable it, of course, but
>> if you are going to use pcie_aspm=force good luck!
>
> People who use "force" deserve whatever they get,

Heh, this whole patch and thread was started because Jussi tested
ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
been trying to explain all along why this is a terrible idea to the
point we should probably just remove that code from the kernel. Hence
my side rants and explanations to justify my reasonings.

> but "powersave" really ought to work.

Interesting, as per Documentation/kernel-parameters.txt we have:

        pcie_aspm=      [PCIE] Forcibly enable or disable PCIe Active
State Power
                        Management.
                off     Disable ASPM.
                force   Enable ASPM even on devices that claim not to
support it.
                        WARNING: Forcing ASPM on may cause system lockups.

I was unaware of a "powersave" option to the pcie_aspm kernel
parameter. In fact:

static int __init pcie_aspm_disable(char *str)
{
        if (!strcmp(str, "off")) {
                aspm_disabled = 1;
                printk(KERN_INFO "PCIe ASPM is disabled\n");
        } else if (!strcmp(str, "force")) {
                aspm_force = 1;
                printk(KERN_INFO "PCIe ASPM is forcedly enabled\n");
        }
        return 1;
}

__setup("pcie_aspm=", pcie_aspm_disable);

Where is "powersave"?

I do see a "powersave" but its an ASPM policy string and it implies
you want to enable L1 and L0s when the device's LinkCap supports it,
see pcie_config_aspm_link() and its users. So in other words powersave
seems to imply you are using pcie_aspm=force, no?

> Fedora's defaulted to that for a while now - we've hit
> issues with aacraid, but that's pretty much it in terms of cases where
> the heuristics don't work. Maxim's problems wouldn't be triggered
> because CONFIG_PCIE_ASPM disables it on pre-1.1 devices regardless of
> the BIOS setup.

I don't expect all distributions to have CONFIG_PCIE_ASPM enabled, in
fact I was unaware of this sanity check being included as part of
CONFIG_PCIE_ASPM, I recommend we consider just enabling the sanity
check all the time. The fact that CONFIG_PCIE_ASPM is even an option
seems confusing to me given that apart from this sanity check the only
other thing that I see useful in it is the forcing of ASPM settings
and as I noted I think pcie_aspm=force is pretty dangerous.

>> > Having looked into this, Windows will enable ASPM on external
>> > controllers unless there's some reason for it not to - where that may be
>> > either the appropriate bit in the FADT being set, the device not being
>> > PCIe 1.1 or later, there being no _OSC method on the appropriate root
>> > bridge or the _OSC method not giving it full control over PCIe, the
>> > driver disabling ASPM or the device not advertising it in the first
>> > place.
>>
>> I was unaware of all this root complex sanity checks on Windows,
>> thanks for sharing.
>
> With the patch I've just sent, they should also all be used for Linux as
> well.

Oh nice! It'll be part of 2.6.36?

>> I suspect these tweaks will go away as the industry produces cards
>> with both L1 and L0s enabled all the time (devices being produced
>> today), but for devices caught in that middle of time between whether
>> or not L0s would be *required*  (last 2 years) I suspect we'll run
>> into these issues.
>
> If the same problems would appear under Windows then it's not a problem
> that I'm hugely concerned about as yet

Yes, these issues would also be part of Windows. But should also note
this also means for those people working on their own BIOSes it means
you also have to take these things into more serious consideration.

> - we'll wait a bit longer and
> then change the ASPM defaults to be more aggressive under Linux, and if
> it turns out to be a significant problem in the real world we'll have to
> reconsider it.

The problem is the tweaks in question are device specific. I can see
if I can get you concrete examples.

> But I don't think we should be depending on userspace
> bashing hardware registers in order to be able to enable power
> management.

Me neither, ASPM should just work with default settings, which is why
I also do not like that the sanity check on the PCIE 1.1 spec is done
through CONFIG_PCIE_ASPM, it makes no sense given that ASPM *will*
work even if you do not have CONFIG_PCIE_ASPM but the device has
functional ASPM.

I do think we should be depending on userspace to do development
testing and forcing ASPM on, because the only other alternative is
pcie_aspm=force and as noted this is just not a good idea unless you
*seriously* know what you are doing.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Matthew Garrett]

On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 10:50 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > People who use "force" deserve whatever they get,
> 
> Heh, this whole patch and thread was started because Jussi tested
> ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
> been trying to explain all along why this is a terrible idea to the
> point we should probably just remove that code from the kernel. Hence
> my side rants and explanations to justify my reasonings.

Well, there's two things here. If you use force then you might get 
inappropriate ASPM. But if your BIOS enables ASPM on an old device, then 
booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting 
*with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is 
confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows 
the kernel to modify the BIOS default, and disabling it makes the 
assumption that your BIOS did something sensible.

> Where is "powersave"?
> 
> I do see a "powersave" but its an ASPM policy string and it implies
> you want to enable L1 and L0s when the device's LinkCap supports it,
> see pcie_config_aspm_link() and its users. So in other words powersave
> seems to imply you are using pcie_aspm=force, no?

No. pcie_aspm=force will enable ASPM even if (a) the device is pre-1.1, 
(b) the firmware has the FADT flag set to tell you not to and (c) the 
firmware doesn't grant control via _OSC. The powersave policy will 
enable ASPM even if the BIOS didn't, but only if something else doesn't 
tell us not to.

> > Fedora's defaulted to that for a while now - we've hit
> > issues with aacraid, but that's pretty much it in terms of cases where
> > the heuristics don't work. Maxim's problems wouldn't be triggered
> > because CONFIG_PCIE_ASPM disables it on pre-1.1 devices regardless of
> > the BIOS setup.
> 
> I don't expect all distributions to have CONFIG_PCIE_ASPM enabled, in
> fact I was unaware of this sanity check being included as part of
> CONFIG_PCIE_ASPM, I recommend we consider just enabling the sanity
> check all the time. The fact that CONFIG_PCIE_ASPM is even an option
> seems confusing to me given that apart from this sanity check the only
> other thing that I see useful in it is the forcing of ASPM settings
> and as I noted I think pcie_aspm=force is pretty dangerous.

You're right, it shouldn't be an option. It's vital for making sure that 
ASPM is disabled when it should be. I'd be happy with pcie_aspm=force 
tainting the kernel.

> > With the patch I've just sent, they should also all be used for Linux as
> > well.
> 
> Oh nice! It'll be part of 2.6.36?

As long as Jesse picks it up.

> > If the same problems would appear under Windows then it's not a problem
> > that I'm hugely concerned about as yet
> 
> Yes, these issues would also be part of Windows. But should also note
> this also means for those people working on their own BIOSes it means
> you also have to take these things into more serious consideration.

There's a standardised mechanism for BIOS authors to tell us not to 
touch their ASPM configuration, and people that ignore that really do 
deserve to have things break.

> Me neither, ASPM should just work with default settings, which is why
> I also do not like that the sanity check on the PCIE 1.1 spec is done
> through CONFIG_PCIE_ASPM, it makes no sense given that ASPM *will*
> work even if you do not have CONFIG_PCIE_ASPM but the device has
> functional ASPM.

I agree. I'll send a patch that moves it under CONFIG_EMBEDDED and 
defaults to on.

> I do think we should be depending on userspace to do development
> testing and forcing ASPM on, because the only other alternative is
> pcie_aspm=force and as noted this is just not a good idea unless you
> *seriously* know what you are doing.

If you set the powersave policy and ASPM doesn't get enabled, then 
that's because we've got a really strong belief that ASPM shouldn't be 
enabled. Is your concern just that pcie_aspm=force is too easy for users 
to get at?

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

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 11:44:26AM -0700, Matthew Garrett wrote:
> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
> > On Tue, Jun 22, 2010 at 10:50 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > People who use "force" deserve whatever they get,
> > 
> > Heh, this whole patch and thread was started because Jussi tested
> > ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
> > been trying to explain all along why this is a terrible idea to the
> > point we should probably just remove that code from the kernel. Hence
> > my side rants and explanations to justify my reasonings.
> 
> Well, there's two things here. If you use force then you might get 
> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then 
> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting 
> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is 
> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows 
> the kernel to modify the BIOS default, and disabling it makes the 
> assumption that your BIOS did something sensible.

Agreed, given that you also mentioned you would put it under embeeded
how about something like this:

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b8b494b..9c76b70 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -31,14 +31,29 @@ source "drivers/pci/pcie/aer/Kconfig"
 # PCI Express ASPM
 #
 config PCIEASPM
-	bool "PCI Express ASPM support(Experimental)"
-	depends on PCI && EXPERIMENTAL && PCIEPORTBUS
-	default n
+	bool "PCI Express ASPM sanity check support" if EMBEDDED
+	depends on PCI && PCIEPORTBUS
+	default y
 	help
-	  This enables PCI Express ASPM (Active State Power Management) and
-	  Clock Power Management. ASPM supports state L0/L0s/L1.
+	  This enables some sanity checks for PCI Express ASPM.
+	  ASPM supports the states L0/L0s/L1. The sanity checks are to
+	  disable ASPM if:
+
+	  a) the device is pre-1.1
+	  b) the firmware has the FADT flag set to tell you not to
+	  c) the firmware doesn't grant control via _OSC
+
+	  Without this option your BIOS's defaults will be respected
+	  and while although this should always be correct it typically
+	  is not. If your ASPM settings are incorrect you may experience
+	  odd hangs which are hard to debug. These sanity checks should
+	  help avoid these odd hangs by only enabling ASPM if we are
+	  sure we can enable it.
+
+	  For more information you can refer to this documentation:
+
+	  http://wireless.kernel.org/en/users/Documentation/ASPM
 
-	  When in doubt, say N.
 config PCIEASPM_DEBUG
 	bool "Debug PCI Express ASPM"
 	depends on PCIEASPM

> > Where is "powersave"?
> > 
> > I do see a "powersave" but its an ASPM policy string and it implies
> > you want to enable L1 and L0s when the device's LinkCap supports it,
> > see pcie_config_aspm_link() and its users. So in other words powersave
> > seems to imply you are using pcie_aspm=force, no?
> 
> No. pcie_aspm=force will enable ASPM even if (a) the device is pre-1.1, 
> (b) the firmware has the FADT flag set to tell you not to and (c) the 
> firmware doesn't grant control via _OSC. The powersave policy will 
> enable ASPM even if the BIOS didn't, but only if something else doesn't 
> tell us not to.

So if any of the above (a) (b) or (c) are true powersave will keep
it disabled? Is pcie_aspm=forcepowersave this a new option with your
patches?

> > > Fedora's defaulted to that for a while now - we've hit
> > > issues with aacraid, but that's pretty much it in terms of cases where
> > > the heuristics don't work. Maxim's problems wouldn't be triggered
> > > because CONFIG_PCIE_ASPM disables it on pre-1.1 devices regardless of
> > > the BIOS setup.
> > 
> > I don't expect all distributions to have CONFIG_PCIE_ASPM enabled, in
> > fact I was unaware of this sanity check being included as part of
> > CONFIG_PCIE_ASPM, I recommend we consider just enabling the sanity
> > check all the time. The fact that CONFIG_PCIE_ASPM is even an option
> > seems confusing to me given that apart from this sanity check the only
> > other thing that I see useful in it is the forcing of ASPM settings
> > and as I noted I think pcie_aspm=force is pretty dangerous.
> 
> You're right, it shouldn't be an option. It's vital for making sure that 
> ASPM is disabled when it should be. I'd be happy with pcie_aspm=force 
> tainting the kernel.

:) !

> > > With the patch I've just sent, they should also all be used for Linux as
> > > well.
> > 
> > Oh nice! It'll be part of 2.6.36?
> 
> As long as Jesse picks it up.

Nice.

> > > If the same problems would appear under Windows then it's not a problem
> > > that I'm hugely concerned about as yet
> > 
> > Yes, these issues would also be part of Windows. But should also note
> > this also means for those people working on their own BIOSes it means
> > you also have to take these things into more serious consideration.
> 
> There's a standardised mechanism for BIOS authors to tell us not to 
> touch their ASPM configuration, and people that ignore that really do 
> deserve to have things break.

Oh, I was more concerned about open bios hackers. If a device requires
PCI host controller tweaks should *we* be doing those tweaks and sanity
checks oursevles upstream or should we rely on the open-bios guys to
do it accordingly in their code?

> > Me neither, ASPM should just work with default settings, which is why
> > I also do not like that the sanity check on the PCIE 1.1 spec is done
> > through CONFIG_PCIE_ASPM, it makes no sense given that ASPM *will*
> > work even if you do not have CONFIG_PCIE_ASPM but the device has
> > functional ASPM.
> 
> I agree. I'll send a patch that moves it under CONFIG_EMBEDDED and 
> defaults to on.

:D

> > I do think we should be depending on userspace to do development
> > testing and forcing ASPM on, because the only other alternative is
> > pcie_aspm=force and as noted this is just not a good idea unless you
> > *seriously* know what you are doing.
> 
> If you set the powersave policy and ASPM doesn't get enabled, then 
> that's because we've got a really strong belief that ASPM shouldn't be 
> enabled. Is your concern just that pcie_aspm=force is too easy for users 
> to get at?

Yes! I think people are starting to use it like to magically save
more power without taking into consideration the implications. This is
why I was suggesting maybe we nuke that option all together. Does it
*really* give us any benefit? The only benefit I see is if we *are*
100% sure our system should work with all our root complexes and
endpoints having ASPM enabled. That I do not see happening until
a few years from now.

Maybe we should have another type of module parameter type, a
I_know_what_Im_doing_module_parameter and taint whenever any of
those are on, not just pcie_aspm=force ? :)

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Johannes Stezenbach]

On Tue, Jun 22, 2010 at 07:44:26PM +0100, Matthew Garrett wrote:
> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
> > 
> > Heh, this whole patch and thread was started because Jussi tested
> > ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
> > been trying to explain all along why this is a terrible idea to the
> > point we should probably just remove that code from the kernel. Hence
> > my side rants and explanations to justify my reasonings.
> 
> Well, there's two things here. If you use force then you might get 
> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then 
> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting 
> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is 
> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows 
> the kernel to modify the BIOS default, and disabling it makes the 
> assumption that your BIOS did something sensible.

Does CONFIG_PCIEASPM provide a way for the user to modifiy
the settings at runtime?

I have a Samsung N130 netbook which has a BIOS setting
called "CPU Power Saving Mode".  When enabled it activates
ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
and the PCIE bridge (with the BIOS setting off it's just L1).
The result is that the ethernet througput is reduced to 25Mbit/s.
(The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)

99,9% of the time I want to enjoy the power savings,
but occationally I have to transfer some bulk data and would
like to switch the setting for a few minutes.

Or, well, ideally I'd like to have power savings _and_ performance
at the same time without any manual intervention.  I'm not sure
if this is a quirk of the N130 or if ASPM L0s always causes
performance degradation?


Thanks
Johannes

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js@sig21.net> wrote:
> On Tue, Jun 22, 2010 at 07:44:26PM +0100, Matthew Garrett wrote:
>> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
>> >
>> > Heh, this whole patch and thread was started because Jussi tested
>> > ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
>> > been trying to explain all along why this is a terrible idea to the
>> > point we should probably just remove that code from the kernel. Hence
>> > my side rants and explanations to justify my reasonings.
>>
>> Well, there's two things here. If you use force then you might get
>> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then
>> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting
>> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is
>> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows
>> the kernel to modify the BIOS default, and disabling it makes the
>> assumption that your BIOS did something sensible.
>
> Does CONFIG_PCIEASPM provide a way for the user to modifiy
> the settings at runtime?

You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:

http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
http://wireless.kernel.org/en/users/Documentation/ASPM

> I have a Samsung N130 netbook which has a BIOS setting
> called "CPU Power Saving Mode".  When enabled it activates
> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> and the PCIE bridge (with the BIOS setting off it's just L1).
> The result is that the ethernet througput is reduced to 25Mbit/s.
> (The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)
>
> 99,9% of the time I want to enjoy the power savings,
> but occationally I have to transfer some bulk data and would
> like to switch the setting for a few minutes.
>
> Or, well, ideally I'd like to have power savings _and_ performance
> at the same time without any manual intervention.  I'm not sure
> if this is a quirk of the N130 or if ASPM L0s always causes
> performance degradation?

L0s is not going to buy you much gains, getting at least L1 will
however. L0s is just a further enhancement. I recommend you test by
enabling L1 and L0s, check how longer your battery lasts and then test
again with just L1. Then test without both L1 and L0s.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jun 22, 2010 at 12:37 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js@sig21.net> wrote:
>> On Tue, Jun 22, 2010 at 07:44:26PM +0100, Matthew Garrett wrote:
>>> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
>>> >
>>> > Heh, this whole patch and thread was started because Jussi tested
>>> > ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
>>> > been trying to explain all along why this is a terrible idea to the
>>> > point we should probably just remove that code from the kernel. Hence
>>> > my side rants and explanations to justify my reasonings.
>>>
>>> Well, there's two things here. If you use force then you might get
>>> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then
>>> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting
>>> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is
>>> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows
>>> the kernel to modify the BIOS default, and disabling it makes the
>>> assumption that your BIOS did something sensible.
>>
>> Does CONFIG_PCIEASPM provide a way for the user to modifiy
>> the settings at runtime?
>
> You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:
>
> http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
> http://wireless.kernel.org/en/users/Documentation/ASPM
>
>> I have a Samsung N130 netbook which has a BIOS setting
>> called "CPU Power Saving Mode".  When enabled it activates
>> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
>> and the PCIE bridge (with the BIOS setting off it's just L1).
>> The result is that the ethernet througput is reduced to 25Mbit/s.
>> (The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)
>>
>> 99,9% of the time I want to enjoy the power savings,
>> but occationally I have to transfer some bulk data and would
>> like to switch the setting for a few minutes.
>>
>> Or, well, ideally I'd like to have power savings _and_ performance
>> at the same time without any manual intervention.  I'm not sure
>> if this is a quirk of the N130 or if ASPM L0s always causes
>> performance degradation?
>
> L0s is not going to buy you much gains, getting at least L1 will
> however. L0s is just a further enhancement. I recommend you test by
> enabling L1 and L0s, check how longer your battery lasts and then test
> again with just L1. Then test without both L1 and L0s.

So defaults should always be sane and you should not have to play with
this stuff, unless you're a hacker, or are testing something for
development purposes. Tweaking ASPM settings is not something a user
should have to worry about. Period.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Johannes Stezenbach]

On Tue, Jun 22, 2010 at 12:38:03PM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 12:37 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> > On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js@sig21.net> wrote:
> >>
> >> Does CONFIG_PCIEASPM provide a way for the user to modifiy
> >> the settings at runtime?
> >
> > You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:
> >
> > http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
> > http://wireless.kernel.org/en/users/Documentation/ASPM
> >
> >> I have a Samsung N130 netbook which has a BIOS setting
> >> called "CPU Power Saving Mode".  When enabled it activates
> >> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> >> and the PCIE bridge (with the BIOS setting off it's just L1).
> >> The result is that the ethernet througput is reduced to 25Mbit/s.
> >
> > L0s is not going to buy you much gains, getting at least L1 will
> > however. L0s is just a further enhancement. I recommend you test by
> > enabling L1 and L0s, check how longer your battery lasts and then test
> > again with just L1. Then test without both L1 and L0s.
> 
> So defaults should always be sane and you should not have to play with
> this stuff, unless you're a hacker, or are testing something for
> development purposes. Tweaking ASPM settings is not something a user
> should have to worry about. Period.

OK, let me put the question another way:

If enabling ASPM comes with a performance penalty (which is not unexpected,
there is usually a tradeoff between performance and power consumption),
do you think a boot time option (pcie_aspm=) or compile time option
(CONFIG_PCIEASPM) is the right user interface?


But meanwhile I found that CONFIG_PCIEASPM has a runtime
interface, /sys/module/pcie_aspm/parameters/policy.
http://lwn.net/Articles/266585/

I have not tested it on my N130 yet.


Johannes

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Luis R. Rodriguez]

On Wed, Jun 23, 2010 at 7:39 AM, Johannes Stezenbach <js@sig21.net> wrote:
> On Tue, Jun 22, 2010 at 12:38:03PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Jun 22, 2010 at 12:37 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> > On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js@sig21.net> wrote:
>> >>
>> >> Does CONFIG_PCIEASPM provide a way for the user to modifiy
>> >> the settings at runtime?
>> >
>> > You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:
>> >
>> > http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
>> > http://wireless.kernel.org/en/users/Documentation/ASPM
>> >
>> >> I have a Samsung N130 netbook which has a BIOS setting
>> >> called "CPU Power Saving Mode".  When enabled it activates
>> >> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
>> >> and the PCIE bridge (with the BIOS setting off it's just L1).
>> >> The result is that the ethernet througput is reduced to 25Mbit/s.
>> >
>> > L0s is not going to buy you much gains, getting at least L1 will
>> > however. L0s is just a further enhancement. I recommend you test by
>> > enabling L1 and L0s, check how longer your battery lasts and then test
>> > again with just L1. Then test without both L1 and L0s.
>>
>> So defaults should always be sane and you should not have to play with
>> this stuff, unless you're a hacker, or are testing something for
>> development purposes. Tweaking ASPM settings is not something a user
>> should have to worry about. Period.
>
> OK, let me put the question another way:
>
> If enabling ASPM comes with a performance penalty (which is not unexpected,
> there is usually a tradeoff between performance and power consumption),
> do you think a boot time option (pcie_aspm=) or compile time option
> (CONFIG_PCIEASPM) is the right user interface?
>
>
> But meanwhile I found that CONFIG_PCIEASPM has a runtime
> interface, /sys/module/pcie_aspm/parameters/policy.
> http://lwn.net/Articles/266585/
>
> I have not tested it on my N130 yet.

Same thing, its to be used by developers not users, damn it we should
just remove this crap.

  Luis

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Johannes Stezenbach]

On Wed, Jun 23, 2010 at 09:28:57AM -0700, Luis R. Rodriguez wrote:
> On Wed, Jun 23, 2010 at 7:39 AM, Johannes Stezenbach <js@sig21.net> wrote:
> >
> > If enabling ASPM comes with a performance penalty (which is not unexpected,
> > there is usually a tradeoff between performance and power consumption),
> > do you think a boot time option (pcie_aspm=) or compile time option
> > (CONFIG_PCIEASPM) is the right user interface?
> >
> >
> > But meanwhile I found that CONFIG_PCIEASPM has a runtime
> > interface, /sys/module/pcie_aspm/parameters/policy.
> > http://lwn.net/Articles/266585/
> 
> Same thing, its to be used by developers not users, damn it we should
> just remove this crap.

Hopefully I got at least the message across that there is
a good reason to have an interface where the user can
select the ASPM policy?

If your answer is "reboot and change the BIOS setting" then
you didn't get what I was talking about _at all_.

Also, having now briefly looked at pcie/aspm.c, I do not share your
opinion that it is crap, except maybe for the force enable.
If I read it correctly it will by default keep the BIOS settings,
but offers a per-device sysfs attribute to change the ASPM link
state, and it seems to do sanity checking, and takes care that
both ends of the link agree.

(disclaimer: I haven't tested any of it, only briefly looked at the code)


Johannes

Re: [ath5k-devel] [PATCH v2] ath5k: disable ASPM

[Johannes Stezenbach]

On Tue, Jun 22, 2010 at 12:37:01PM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js@sig21.net> wrote:
> > I have a Samsung N130 netbook which has a BIOS setting
> > called "CPU Power Saving Mode".  When enabled it activates
> > ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> > and the PCIE bridge (with the BIOS setting off it's just L1).
> > The result is that the ethernet througput is reduced to 25Mbit/s.
> > (The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)
> 
> L0s is not going to buy you much gains, getting at least L1 will
> however. L0s is just a further enhancement. I recommend you test by
> enabling L1 and L0s, check how longer your battery lasts and then test
> again with just L1. Then test without both L1 and L0s.

What I did was to dump lspci output to a file, for both settings
of the BIOS option, and then diff.

00:1c.2 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 3 (rev 02) (prog-if 00 [Normal decode])
		...
                LnkCap: Port #3, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <256ns, L1 <4us
                        ClockPM- Surprise- LLActRep+ BwNot-
-               LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
+               LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
                        ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-

03:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8101E/RTL8102E PCI Express Fast Ethernet controller (rev 02)
		...
                LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <512ns, L1 <64us
                        ClockPM+ Surprise- LLActRep- BwNot-
-               LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
+               LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
                        ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
                LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-

I suspect the performance penalty is because L0s is entered very often
even during ethernet bulk transfer, whereas L1 is only entered
when the link is idle for longer time.

The difference in battery run time due to the BIOS option is
significant (IIRC, it's been a while back), but I don't know
what else the BIOS option changes.


Johannes

Re: [PATCH] ath5k: disable ASPM

[Maxim Levitsky]

Hi,

Just want to summarize and finally put that problem to rest.

Was the patch that removes & sets on CONFIG_PCIEASPM? accepted?

Is it possible to check that all ath5k pcie devices that must not use
L0s actually have the 'PCI_EXP_DEVCAP_RBER' disabled (this bit causes
pcie device be marked as legacy, and ASPM is disabled on it)

Best regards,
Maxim Levitsky

Re: [PATCH] ath5k: disable ASPM

[John W. Linville]

On Mon, Jul 26, 2010 at 07:34:04PM +0300, Maxim Levitsky wrote:

> Just want to summarize and finally put that problem to rest.
> 
> Was the patch that removes & sets on CONFIG_PCIEASPM? accepted?
> 
> Is it possible to check that all ath5k pcie devices that must not use
> L0s actually have the 'PCI_EXP_DEVCAP_RBER' disabled (this bit causes
> pcie device be marked as legacy, and ASPM is disabled on it)

http://marc.info/?l=linux-wireless&m=127507083106814&w=2

"Sorry, I meant to this to be RFC patch."

So, I dropped it.

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [ath5k-devel] [PATCH] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 9:34 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> Hi,
>
> Just want to summarize and finally put that problem to rest.
>
> Was the patch that removes & sets on CONFIG_PCIEASPM? accepted?
>
> Is it possible to check that all ath5k pcie devices that must not use
> L0s actually have the 'PCI_EXP_DEVCAP_RBER' disabled (this bit causes
> pcie device be marked as legacy, and ASPM is disabled on it)

ASPM should be kept enabled for ath5k but L0s should be disabled, so
that is, only L1 for ASPM should be kept enabled for ath5k for all
ath5k chipsets. Can someone resubmit the simpler patch again on a new
independent thread?

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
> On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > > How this patch?
> > > 
> > > Looks fine to me.  Some nitpicking below but feel free to add my
> > > 
> > > Acked-by: Bob Copeland <me@bobcopeland.com>
> > > 
> Done.
> 
> Best regards,
> Maxim Levitsky
> 
> --- 
> 
> commit 616afa397b3e843f2aba06be12a30e72dfff7740
> Author: Maxim Levitsky <maximlevitsky@gmail.com>
> Date:   Thu Jun 17 23:21:42 2010 +0300
> 
>     ath5k: disable ASPM
>     
>     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
>     these problems.
>     Also card sends a storm of RXORN interrupts even though medium is idle.
>     
>     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
>     both ends (usually stalls within seconds).
>     
>     Unfortunately BIOS enables ASPM on this card by default on these machines
>     This means that, problem shows up (less often) without pcie_aspm=force too. 
>     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
>     
> 
>     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>     for finding and fixing this bug.
>     
>     Based on patch that is
>     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>     
>     
>     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>     Acked-by: Bob Copeland <me@bobcopeland.com>

Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>

But please resubmit and completley modify the commit log to indicate
ath5k cards support ASPM but L0s must be disabled, only L1
works correctly.

The comments about ASPM force should be removed as it would
lead others to try to use the same and the fact of the matter is
that ASPM force should never be used. As was clarified out of
some of these discussions worth noting also is that in newer
kernels CONFIG_PCIEASPM=y will always become the default, for
older kernels this was never the default and some distributions
(Ubunutu) do not have this enabled, the benefit of having it
enabled is it will disable ASPM for these cases:

(a) the PCIE device is pre PCIE 1.1
(b) the firmware has the FADT flag set to tell you not to and
(c) the firmware doesn't grant control via _OSC. The powersave policy will
enable ASPM even if the BIOS didn't, but only if something else doesn't
tell us not to.

The last two checks were only recently added by Mathew and forcing
CONFIG_PCIEASPM=y was also only recently made default.

In short, Linux distributions should also start enabling
CONFIG_PCIEASPM=y on older kernels.

  Luis

> 
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 3abbe75..4f6bd7c 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -48,6 +48,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/cache.h>
>  #include <linux/pci.h>
> +#include <linux/pci-aspm.h>
>  #include <linux/ethtool.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> @@ -469,6 +470,19 @@ ath5k_pci_probe(struct pci_dev *pdev,
>  	int ret;
>  	u8 csz;
>  
> +	/*
> +	 * Disable PCIE ASPM L0S on the card.
> +	 * ASPM triggers hardware bug, that makes card stall transmission
> +	 * untill reset, and even that doesn't always help.
> +	 * This happens on meduim to heavy transmit utilization.
> +	 * In addition to stall, hardware usually gives a storm of
> +	 * RXORN interrupts, despite idle channel, and otherwise doesn't work.
> +	 * Windows driver also disables the L0s ASPM,
> +	 * probably due to same reason
> +	 * Note: to benefit from this fix, please _enable_ CONFIG_PCIEASPM
> +	 */
> +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> +
>  	ret = pci_enable_device(pdev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "can't enable device\n");
> 
> 
> _______________________________________________
> ath5k-devel mailing list
> ath5k-devel@lists.ath5k.org
> https://lists.ath5k.org/mailman/listinfo/ath5k-devel

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Maxim Levitsky]

On Mon, 2010-07-26 at 13:13 -0700, Luis R. Rodriguez wrote: 
> On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
> > On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> > > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > > > How this patch?
> > > > 
> > > > Looks fine to me.  Some nitpicking below but feel free to add my
> > > > 
> > > > Acked-by: Bob Copeland <me@bobcopeland.com>
> > > > 
> > Done.
> > 
> > Best regards,
> > Maxim Levitsky
> > 
> > --- 
> > 
> > commit 616afa397b3e843f2aba06be12a30e72dfff7740
> > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > Date:   Thu Jun 17 23:21:42 2010 +0300
> > 
> >     ath5k: disable ASPM
> >     
> >     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> >     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> >     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> >     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> >     these problems.
> >     Also card sends a storm of RXORN interrupts even though medium is idle.
> >     
> >     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> >     both ends (usually stalls within seconds).
> >     
> >     Unfortunately BIOS enables ASPM on this card by default on these machines
> >     This means that, problem shows up (less often) without pcie_aspm=force too. 
> >     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
> >     
> > 
> >     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >     for finding and fixing this bug.
> >     
> >     Based on patch that is
> >     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >     
> >     
> >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> >     Acked-by: Bob Copeland <me@bobcopeland.com>
> 
> Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> 
> But please resubmit and completley modify the commit log to indicate
> ath5k cards support ASPM but L0s must be disabled, only L1
> works correctly.
> 
> The comments about ASPM force should be removed as it would
> lead others to try to use the same and the fact of the matter is
> that ASPM force should never be used. As was clarified out of
> some of these discussions worth noting also is that in newer
> kernels CONFIG_PCIEASPM=y will always become the default, for
> older kernels this was never the default and some distributions
> (Ubunutu) do not have this enabled, the benefit of having it
> enabled is it will disable ASPM for these cases:
> 
> (a) the PCIE device is pre PCIE 1.1
> (b) the firmware has the FADT flag set to tell you not to and
> (c) the firmware doesn't grant control via _OSC. The powersave policy will
> enable ASPM even if the BIOS didn't, but only if something else doesn't
> tell us not to.
> 
> The last two checks were only recently added by Mathew and forcing
> CONFIG_PCIEASPM=y was also only recently made default.
> 
> In short, Linux distributions should also start enabling
> CONFIG_PCIEASPM=y on older kernels.


Just one note that since at least my ath5k device is pre 1.1, and you
say that L1 can be enabled, and should, I probably need to enable L1
explicitly in the driver.

OK?

Best regards,
Maxim Levitsky

> 
>   Luis
> 
> > 
> > diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> > index 3abbe75..4f6bd7c 100644
> > --- a/drivers/net/wireless/ath/ath5k/base.c
> > +++ b/drivers/net/wireless/ath/ath5k/base.c
> > @@ -48,6 +48,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/cache.h>
> >  #include <linux/pci.h>
> > +#include <linux/pci-aspm.h>
> >  #include <linux/ethtool.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/slab.h>
> > @@ -469,6 +470,19 @@ ath5k_pci_probe(struct pci_dev *pdev,
> >  	int ret;
> >  	u8 csz;
> >  
> > +	/*
> > +	 * Disable PCIE ASPM L0S on the card.
> > +	 * ASPM triggers hardware bug, that makes card stall transmission
> > +	 * untill reset, and even that doesn't always help.
> > +	 * This happens on meduim to heavy transmit utilization.
> > +	 * In addition to stall, hardware usually gives a storm of
> > +	 * RXORN interrupts, despite idle channel, and otherwise doesn't work.
> > +	 * Windows driver also disables the L0s ASPM,
> > +	 * probably due to same reason
> > +	 * Note: to benefit from this fix, please _enable_ CONFIG_PCIEASPM
> > +	 */
> > +	pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S);
> > +
> >  	ret = pci_enable_device(pdev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "can't enable device\n");
> > 
> > 
> > _______________________________________________
> > ath5k-devel mailing list
> > ath5k-devel@lists.ath5k.org
> > https://lists.ath5k.org/mailman/listinfo/ath5k-devel

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 01:49:22PM -0700, Maxim Levitsky wrote:
> On Mon, 2010-07-26 at 13:13 -0700, Luis R. Rodriguez wrote: 
> > On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
> > > On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> > > > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > > > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > > > > How this patch?
> > > > > 
> > > > > Looks fine to me.  Some nitpicking below but feel free to add my
> > > > > 
> > > > > Acked-by: Bob Copeland <me@bobcopeland.com>
> > > > > 
> > > Done.
> > > 
> > > Best regards,
> > > Maxim Levitsky
> > > 
> > > --- 
> > > 
> > > commit 616afa397b3e843f2aba06be12a30e72dfff7740
> > > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > > Date:   Thu Jun 17 23:21:42 2010 +0300
> > > 
> > >     ath5k: disable ASPM
> > >     
> > >     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> > >     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> > >     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> > >     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> > >     these problems.
> > >     Also card sends a storm of RXORN interrupts even though medium is idle.
> > >     
> > >     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> > >     both ends (usually stalls within seconds).
> > >     
> > >     Unfortunately BIOS enables ASPM on this card by default on these machines
> > >     This means that, problem shows up (less often) without pcie_aspm=force too. 
> > >     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
> > >     
> > > 
> > >     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> > >     for finding and fixing this bug.
> > >     
> > >     Based on patch that is
> > >     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> > >     
> > >     
> > >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > >     Acked-by: Bob Copeland <me@bobcopeland.com>
> > 
> > Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> > 
> > But please resubmit and completley modify the commit log to indicate
> > ath5k cards support ASPM but L0s must be disabled, only L1
> > works correctly.
> > 
> > The comments about ASPM force should be removed as it would
> > lead others to try to use the same and the fact of the matter is
> > that ASPM force should never be used. As was clarified out of
> > some of these discussions worth noting also is that in newer
> > kernels CONFIG_PCIEASPM=y will always become the default, for
> > older kernels this was never the default and some distributions
> > (Ubunutu) do not have this enabled, the benefit of having it
> > enabled is it will disable ASPM for these cases:
> > 
> > (a) the PCIE device is pre PCIE 1.1
> > (b) the firmware has the FADT flag set to tell you not to and
> > (c) the firmware doesn't grant control via _OSC. The powersave policy will
> > enable ASPM even if the BIOS didn't, but only if something else doesn't
> > tell us not to.
> > 
> > The last two checks were only recently added by Mathew and forcing
> > CONFIG_PCIEASPM=y was also only recently made default.
> > 
> > In short, Linux distributions should also start enabling
> > CONFIG_PCIEASPM=y on older kernels.
> 
> 
> Just one note that since at least my ath5k device is pre 1.1, and you
> say that L1 can be enabled, and should, I probably need to enable L1
> explicitly in the driver.
> 
> OK?

No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
other settings that have to be taken care of like modifying some PCI entrance and
exit latency timers, the number of FTS packets we send to exit L0s, amongst
other things. If a user selectively enables L1 but the BIOS had it disabled on
the device it may not work correctly.

In other words leave the settings as-is on your card unless you developing
for a company to enable ASPM yourself or you are willing to take the risks.
Tweaks like force enabling a device to use ASPM should only then be done in
userspace and by an end user. This is not something Linux distributions should
let users tweak. Its a hacker task.

However, it is reasonable to force disable L0s, for example, completley on
the driver if it is known that L0s does not work for all chisets supported
on that driver.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 02:06:51PM -0700, Luis R. Rodriguez wrote:

> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
> other settings that have to be taken care of like modifying some PCI entrance and
> exit latency timers, the number of FTS packets we send to exit L0s, amongst
> other things. If a user selectively enables L1 but the BIOS had it disabled on
> the device it may not work correctly.

That's really the job of the driver. If the ASPM policy is set to 
powersave, the fadt doesn't indicate that ASPM should be disabled and 
the bus's _OSC method grants full control then the kernel will enable 
whatever combination of L states meet the latency constraints. If the 
hardware has additional constraints then the hardware-specific driver 
needs to handle them.

We don't rely on the BIOS to set up ASPM states. Nor does Windows.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Maxim Levitsky]

On Mon, 2010-07-26 at 14:06 -0700, Luis R. Rodriguez wrote: 
> On Mon, Jul 26, 2010 at 01:49:22PM -0700, Maxim Levitsky wrote:
> > On Mon, 2010-07-26 at 13:13 -0700, Luis R. Rodriguez wrote: 
> > > On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
> > > > On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> > > > > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > > > > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > > > > > How this patch?
> > > > > > 
> > > > > > Looks fine to me.  Some nitpicking below but feel free to add my
> > > > > > 
> > > > > > Acked-by: Bob Copeland <me@bobcopeland.com>
> > > > > > 
> > > > Done.
> > > > 
> > > > Best regards,
> > > > Maxim Levitsky
> > > > 
> > > > --- 
> > > > 
> > > > commit 616afa397b3e843f2aba06be12a30e72dfff7740
> > > > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > > > Date:   Thu Jun 17 23:21:42 2010 +0300
> > > > 
> > > >     ath5k: disable ASPM
> > > >     
> > > >     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> > > >     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> > > >     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> > > >     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> > > >     these problems.
> > > >     Also card sends a storm of RXORN interrupts even though medium is idle.
> > > >     
> > > >     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> > > >     both ends (usually stalls within seconds).
> > > >     
> > > >     Unfortunately BIOS enables ASPM on this card by default on these machines
> > > >     This means that, problem shows up (less often) without pcie_aspm=force too. 
> > > >     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
> > > >     
> > > > 
> > > >     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> > > >     for finding and fixing this bug.
> > > >     
> > > >     Based on patch that is
> > > >     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> > > >     
> > > >     
> > > >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> > > >     Acked-by: Bob Copeland <me@bobcopeland.com>
> > > 
> > > Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> > > 
> > > But please resubmit and completley modify the commit log to indicate
> > > ath5k cards support ASPM but L0s must be disabled, only L1
> > > works correctly.
> > > 
> > > The comments about ASPM force should be removed as it would
> > > lead others to try to use the same and the fact of the matter is
> > > that ASPM force should never be used. As was clarified out of
> > > some of these discussions worth noting also is that in newer
> > > kernels CONFIG_PCIEASPM=y will always become the default, for
> > > older kernels this was never the default and some distributions
> > > (Ubunutu) do not have this enabled, the benefit of having it
> > > enabled is it will disable ASPM for these cases:
> > > 
> > > (a) the PCIE device is pre PCIE 1.1
> > > (b) the firmware has the FADT flag set to tell you not to and
> > > (c) the firmware doesn't grant control via _OSC. The powersave policy will
> > > enable ASPM even if the BIOS didn't, but only if something else doesn't
> > > tell us not to.
> > > 
> > > The last two checks were only recently added by Mathew and forcing
> > > CONFIG_PCIEASPM=y was also only recently made default.
> > > 
> > > In short, Linux distributions should also start enabling
> > > CONFIG_PCIEASPM=y on older kernels.
> > 
> > 
> > Just one note that since at least my ath5k device is pre 1.1, and you
> > say that L1 can be enabled, and should, I probably need to enable L1
> > explicitly in the driver.
> > 
> > OK?
> 
> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
> other settings that have to be taken care of like modifying some PCI entrance and
> exit latency timers, the number of FTS packets we send to exit L0s, amongst
> other things. If a user selectively enables L1 but the BIOS had it disabled on
> the device it may not work correctly.
> 
> In other words leave the settings as-is on your card unless you developing
> for a company to enable ASPM yourself or you are willing to take the risks.
> Tweaks like force enabling a device to use ASPM should only then be done in
> userspace and by an end user. This is not something Linux distributions should
> let users tweak. Its a hacker task.
> 
> However, it is reasonable to force disable L0s, for example, completley on
> the driver if it is known that L0s does not work for all chisets supported
> on that driver.

You didn't understand me.


On my notebook, the AR5001 device is marked as pre 1.1 PCIE device.

ASPM *is* enabled for both L0s and L1 by BIOS.

Linux disables ASPM because the device is pre 1.1, and things work fine
with no more patching (assuming that CONFIG_PCIEASPM is set)


However, it is possible, (and that what I asked you) that some ath5k
devices aren't 'pre 1.1 pcie devices' so linux won't disable ASPM L0s
for them.
So indeed for 'good feeling' it is ok to disable L0s from ath5k
explicitly, but most of the time (or always) it will be no-op.

In *addition* to that, since you said that ASPM L1 *does* work, and is
enabled by BIOS, but linux disables it, that it might be worthy to
enable it again from ath5k driver explicitly.
As long as wireless works I don't really care if this done or not.


Best regards,
Maxim Levitsky

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Tue, Jul 27, 2010 at 12:17:13AM +0300, Maxim Levitsky wrote:

> However, it is possible, (and that what I asked you) that some ath5k
> devices aren't 'pre 1.1 pcie devices' so linux won't disable ASPM L0s
> for them.
> So indeed for 'good feeling' it is ok to disable L0s from ath5k
> explicitly, but most of the time (or always) it will be no-op.
> 
> In *addition* to that, since you said that ASPM L1 *does* work, and is
> enabled by BIOS, but linux disables it, that it might be worthy to
> enable it again from ath5k driver explicitly.
> As long as wireless works I don't really care if this done or not.

This may need to be done on a chip by chip basis. Take a look at 
http://www.atheros.cz/inffile.php?inf=68&bit=32&atheros=AR5002G&system=4 
and some of the other inf files on that site to see which devices 
provide the PciASPMOptIn flag - those should support ASPM states even if 
they're pre-1.1 devices.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 2:17 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Mon, 2010-07-26 at 14:06 -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 26, 2010 at 01:49:22PM -0700, Maxim Levitsky wrote:
>> > On Mon, 2010-07-26 at 13:13 -0700, Luis R. Rodriguez wrote:
>> > > On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
>> > > > On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote:
>> > > > > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote:
>> > > > > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
>> > > > > > > How this patch?
>> > > > > >
>> > > > > > Looks fine to me.  Some nitpicking below but feel free to add my
>> > > > > >
>> > > > > > Acked-by: Bob Copeland <me@bobcopeland.com>
>> > > > > >
>> > > > Done.
>> > > >
>> > > > Best regards,
>> > > > Maxim Levitsky
>> > > >
>> > > > ---
>> > > >
>> > > > commit 616afa397b3e843f2aba06be12a30e72dfff7740
>> > > > Author: Maxim Levitsky <maximlevitsky@gmail.com>
>> > > > Date:   Thu Jun 17 23:21:42 2010 +0300
>> > > >
>> > > >     ath5k: disable ASPM
>> > > >
>> > > >     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
>> > > >     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
>> > > >     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
>> > > >     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
>> > > >     these problems.
>> > > >     Also card sends a storm of RXORN interrupts even though medium is idle.
>> > > >
>> > > >     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
>> > > >     both ends (usually stalls within seconds).
>> > > >
>> > > >     Unfortunately BIOS enables ASPM on this card by default on these machines
>> > > >     This means that, problem shows up (less often) without pcie_aspm=force too.
>> > > >     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
>> > > >
>> > > >
>> > > >     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>> > > >     for finding and fixing this bug.
>> > > >
>> > > >     Based on patch that is
>> > > >     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
>> > > >
>> > > >
>> > > >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
>> > > >     Acked-by: Bob Copeland <me@bobcopeland.com>
>> > >
>> > > Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> > >
>> > > But please resubmit and completley modify the commit log to indicate
>> > > ath5k cards support ASPM but L0s must be disabled, only L1
>> > > works correctly.
>> > >
>> > > The comments about ASPM force should be removed as it would
>> > > lead others to try to use the same and the fact of the matter is
>> > > that ASPM force should never be used. As was clarified out of
>> > > some of these discussions worth noting also is that in newer
>> > > kernels CONFIG_PCIEASPM=y will always become the default, for
>> > > older kernels this was never the default and some distributions
>> > > (Ubunutu) do not have this enabled, the benefit of having it
>> > > enabled is it will disable ASPM for these cases:
>> > >
>> > > (a) the PCIE device is pre PCIE 1.1
>> > > (b) the firmware has the FADT flag set to tell you not to and
>> > > (c) the firmware doesn't grant control via _OSC. The powersave policy will
>> > > enable ASPM even if the BIOS didn't, but only if something else doesn't
>> > > tell us not to.
>> > >
>> > > The last two checks were only recently added by Mathew and forcing
>> > > CONFIG_PCIEASPM=y was also only recently made default.
>> > >
>> > > In short, Linux distributions should also start enabling
>> > > CONFIG_PCIEASPM=y on older kernels.
>> >
>> >
>> > Just one note that since at least my ath5k device is pre 1.1, and you
>> > say that L1 can be enabled, and should, I probably need to enable L1
>> > explicitly in the driver.
>> >
>> > OK?
>>
>> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
>> other settings that have to be taken care of like modifying some PCI entrance and
>> exit latency timers, the number of FTS packets we send to exit L0s, amongst
>> other things. If a user selectively enables L1 but the BIOS had it disabled on
>> the device it may not work correctly.
>>
>> In other words leave the settings as-is on your card unless you developing
>> for a company to enable ASPM yourself or you are willing to take the risks.
>> Tweaks like force enabling a device to use ASPM should only then be done in
>> userspace and by an end user. This is not something Linux distributions should
>> let users tweak. Its a hacker task.
>>
>> However, it is reasonable to force disable L0s, for example, completley on
>> the driver if it is known that L0s does not work for all chisets supported
>> on that driver.
>
> You didn't understand me.
>
>
> On my notebook, the AR5001 device is marked as pre 1.1 PCIE device.
>
> ASPM *is* enabled for both L0s and L1 by BIOS.
>
> Linux disables ASPM because the device is pre 1.1, and things work fine
> with no more patching (assuming that CONFIG_PCIEASPM is set)
>
>
> However, it is possible, (and that what I asked you) that some ath5k
> devices aren't 'pre 1.1 pcie devices' so linux won't disable ASPM L0s
> for them.
> So indeed for 'good feeling' it is ok to disable L0s from ath5k
> explicitly, but most of the time (or always) it will be no-op.

Sure, a no-op is fine given that it won't affect those devices. That
is why I ACKed the contents of the patch but not the commit log entry
description.

> In *addition* to that, since you said that ASPM L1 *does* work, and is
> enabled by BIOS, but linux disables it, that it might be worthy to
> enable it again from ath5k driver explicitly.
> As long as wireless works I don't really care if this done or not.

No, L1 isn't enabled on all devices, so best is to just keep what the
device has set and if the kernel knows better through the rules
mentioned before, let it disable it.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 2:25 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Tue, Jul 27, 2010 at 12:17:13AM +0300, Maxim Levitsky wrote:
>
>> However, it is possible, (and that what I asked you) that some ath5k
>> devices aren't 'pre 1.1 pcie devices' so linux won't disable ASPM L0s
>> for them.
>> So indeed for 'good feeling' it is ok to disable L0s from ath5k
>> explicitly, but most of the time (or always) it will be no-op.
>>
>> In *addition* to that, since you said that ASPM L1 *does* work, and is
>> enabled by BIOS, but linux disables it, that it might be worthy to
>> enable it again from ath5k driver explicitly.
>> As long as wireless works I don't really care if this done or not.
>
> This may need to be done on a chip by chip basis. Take a look at
> http://www.atheros.cz/inffile.php?inf=68&bit=32&atheros=AR5002G&system=4
> and some of the other inf files on that site to see which devices
> provide the PciASPMOptIn flag - those should support ASPM states even if
> they're pre-1.1 devices.

I rather we not bother with these, lets simply follow the kernel's
lead here for its rule matching.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 2:14 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jul 26, 2010 at 02:06:51PM -0700, Luis R. Rodriguez wrote:
>
>> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
>> other settings that have to be taken care of like modifying some PCI entrance and
>> exit latency timers, the number of FTS packets we send to exit L0s, amongst
>> other things. If a user selectively enables L1 but the BIOS had it disabled on
>> the device it may not work correctly.
>
> That's really the job of the driver.

The problem is that sometimes tweaks need to be done on the PCI
controller/root complex, not the PCIE device/endpoint. Today these
sort of changes *are* handled by the respective  systems
integrator/BIOS team and varies depending on the root complex used.
Atheros does not handle these at all in the driver.

> If the ASPM policy is set to
> powersave, the fadt doesn't indicate that ASPM should be disabled and
> the bus's _OSC method grants full control then the kernel will enable
> whatever combination of L states meet the latency constraints. If the
> hardware has additional constraints then the hardware-specific driver
> needs to handle them.

This makes sense but Is there an API for this?

> We don't rely on the BIOS to set up ASPM states. Nor does Windows.

Understood, but today some tweaks seem to be done on the BIOS
depending on the endpoint / root complex.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 03:15:32PM -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 26, 2010 at 2:25 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > This may need to be done on a chip by chip basis. Take a look at
> > http://www.atheros.cz/inffile.php?inf=68&bit=32&atheros=AR5002G&system=4
> > and some of the other inf files on that site to see which devices
> > provide the PciASPMOptIn flag - those should support ASPM states even if
> > they're pre-1.1 devices.
> 
> I rather we not bother with these, lets simply follow the kernel's
> lead here for its rule matching.

Sorry? The idea is to indicate which chips support ASPM even though 
they're pre-PCIe 1.1. If all Atheros parts work fine with L1 then that 
makes things much easier, but it would be good to know the correct set 
of chips that are broken with L0s.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 03:20:23PM -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 26, 2010 at 2:14 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Mon, Jul 26, 2010 at 02:06:51PM -0700, Luis R. Rodriguez wrote:
> >
> >> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
> >> other settings that have to be taken care of like modifying some PCI entrance and
> >> exit latency timers, the number of FTS packets we send to exit L0s, amongst
> >> other things. If a user selectively enables L1 but the BIOS had it disabled on
> >> the device it may not work correctly.
> >
> > That's really the job of the driver.
> 
> The problem is that sometimes tweaks need to be done on the PCI
> controller/root complex, not the PCIE device/endpoint. Today these
> sort of changes *are* handled by the respective  systems
> integrator/BIOS team and varies depending on the root complex used.
> Atheros does not handle these at all in the driver.

Really? Your Windows drivers declare that they support ASPM for some 
parts, which will trigger Vista and later to program L0s and L1 
(depending on system config) without further reference to the driver. If 
we need hooks in the kernel for drivers to provide further feedback to 
make sure this works correctly then we can certainly provide them...

> > If the ASPM policy is set to
> > powersave, the fadt doesn't indicate that ASPM should be disabled and
> > the bus's _OSC method grants full control then the kernel will enable
> > whatever combination of L states meet the latency constraints. If the
> > hardware has additional constraints then the hardware-specific driver
> > needs to handle them.
> 
> This makes sense but Is there an API for this?

Right now, no. What do you need?

> > We don't rely on the BIOS to set up ASPM states. Nor does Windows.
> 
> Understood, but today some tweaks seem to be done on the BIOS
> depending on the endpoint / root complex.

The current situation is that we won't modify anything that the BIOS has 
done, other than the link and clock pm bits. So if the BIOS has done 
something for us, we won't (currently) step on it.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 3:21 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jul 26, 2010 at 03:15:32PM -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 26, 2010 at 2:25 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > This may need to be done on a chip by chip basis. Take a look at
>> > http://www.atheros.cz/inffile.php?inf=68&bit=32&atheros=AR5002G&system=4
>> > and some of the other inf files on that site to see which devices
>> > provide the PciASPMOptIn flag - those should support ASPM states even if
>> > they're pre-1.1 devices.
>>
>> I rather we not bother with these, lets simply follow the kernel's
>> lead here for its rule matching.
>
> Sorry? The idea is to indicate which chips support ASPM even though
> they're pre-PCIe 1.1. If all Atheros parts work fine with L1 then that
> makes things much easier, but it would be good to know the correct set
> of chips that are broken with L0s.

What I meant was that the PCI config space would already have L1
enabled if L1 worked, so I don't see why we would need to nitpick out
specifics here. All Atheros PCIE chips should work with L1. The advise
given is to disable L0s though. I believe AR2425 would be one which
likely had L0s enabled but requires it to be disabled. Not sure of
others. But this is why I am saying this can be done globally for all
ath5k chipsets.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 03:26:37PM -0700, Luis R. Rodriguez wrote:

> What I meant was that the PCI config space would already have L1
> enabled if L1 worked, so I don't see why we would need to nitpick out
> specifics here. All Atheros PCIE chips should work with L1. The advise
> given is to disable L0s though. I believe AR2425 would be one which
> likely had L0s enabled but requires it to be disabled. Not sure of
> others. But this is why I am saying this can be done globally for all
> ath5k chipsets.

If L1 is set but the chip is pre-PCIe 1.1 then we'll disable L1 unless 
the driver tells us that it's functional. The .inf from the Windows 
driver seemed to suggest that only a subset of the chips re-enabled L1 
there, but if it's ok in general then that's a straightforward one-line 
patch.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 3:24 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jul 26, 2010 at 03:20:23PM -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 26, 2010 at 2:14 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Mon, Jul 26, 2010 at 02:06:51PM -0700, Luis R. Rodriguez wrote:
>> >
>> >> No, ASPM must be enabled by the Systems Integrator through the BIOS, there are
>> >> other settings that have to be taken care of like modifying some PCI entrance and
>> >> exit latency timers, the number of FTS packets we send to exit L0s, amongst
>> >> other things. If a user selectively enables L1 but the BIOS had it disabled on
>> >> the device it may not work correctly.
>> >
>> > That's really the job of the driver.
>>
>> The problem is that sometimes tweaks need to be done on the PCI
>> controller/root complex, not the PCIE device/endpoint. Today these
>> sort of changes *are* handled by the respective  systems
>> integrator/BIOS team and varies depending on the root complex used.
>> Atheros does not handle these at all in the driver.
>
> Really? Your Windows drivers declare that they support ASPM for some
> parts, which will trigger Vista and later to program L0s and L1
> (depending on system config) without further reference to the driver. If
> we need hooks in the kernel for drivers to provide further feedback to
> make sure this works correctly then we can certainly provide them...

Sure, agreed, I'm just pointing out today there are other changes made
and even Microsoft does not get involved with them, so it is the same
for us. However I do agree if the kernel can expose API to tweak
things then great.

>> > If the ASPM policy is set to
>> > powersave, the fadt doesn't indicate that ASPM should be disabled and
>> > the bus's _OSC method grants full control then the kernel will enable
>> > whatever combination of L states meet the latency constraints. If the
>> > hardware has additional constraints then the hardware-specific driver
>> > needs to handle them.
>>
>> This makes sense but Is there an API for this?
>
> Right now, no. What do you need?

Not sure, let me check internally and see if I can get the exact details.

>> > We don't rely on the BIOS to set up ASPM states. Nor does Windows.
>>
>> Understood, but today some tweaks seem to be done on the BIOS
>> depending on the endpoint / root complex.
>
> The current situation is that we won't modify anything that the BIOS has
> done, other than the link and clock pm bits. So if the BIOS has done
> something for us, we won't (currently) step on it.

Sure, understood.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 3:29 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jul 26, 2010 at 03:26:37PM -0700, Luis R. Rodriguez wrote:
>
>> What I meant was that the PCI config space would already have L1
>> enabled if L1 worked, so I don't see why we would need to nitpick out
>> specifics here. All Atheros PCIE chips should work with L1. The advise
>> given is to disable L0s though. I believe AR2425 would be one which
>> likely had L0s enabled but requires it to be disabled. Not sure of
>> others. But this is why I am saying this can be done globally for all
>> ath5k chipsets.
>
> If L1 is set but the chip is pre-PCIe 1.1 then we'll disable L1 unless
> the driver tells us that it's functional. The .inf from the Windows
> driver seemed to suggest that only a subset of the chips re-enabled L1
> there, but if it's ok in general then that's a straightforward one-line
> patch.

But why can't we just rely on what the device already has on its PCI
config space and only ensure to disable L0s?

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 03:31:28PM -0700, Luis R. Rodriguez wrote:
> On Mon, Jul 26, 2010 at 3:29 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Mon, Jul 26, 2010 at 03:26:37PM -0700, Luis R. Rodriguez wrote:
> >
> >> What I meant was that the PCI config space would already have L1
> >> enabled if L1 worked, so I don't see why we would need to nitpick out
> >> specifics here. All Atheros PCIE chips should work with L1. The advise
> >> given is to disable L0s though. I believe AR2425 would be one which
> >> likely had L0s enabled but requires it to be disabled. Not sure of
> >> others. But this is why I am saying this can be done globally for all
> >> ath5k chipsets.
> >
> > If L1 is set but the chip is pre-PCIe 1.1 then we'll disable L1 unless
> > the driver tells us that it's functional. The .inf from the Windows
> > driver seemed to suggest that only a subset of the chips re-enabled L1
> > there, but if it's ok in general then that's a straightforward one-line
> > patch.
> 
> But why can't we just rely on what the device already has on its PCI
> config space and only ensure to disable L0s?

Because we globally disable ASPM on pre-1.1 devices, because that's what 
Windows does. It makes it easier for us to figure out what level of 
support we can expect from different hardware revisions.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 3:33 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Jul 26, 2010 at 03:31:28PM -0700, Luis R. Rodriguez wrote:
>> On Mon, Jul 26, 2010 at 3:29 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> > On Mon, Jul 26, 2010 at 03:26:37PM -0700, Luis R. Rodriguez wrote:
>> >
>> >> What I meant was that the PCI config space would already have L1
>> >> enabled if L1 worked, so I don't see why we would need to nitpick out
>> >> specifics here. All Atheros PCIE chips should work with L1. The advise
>> >> given is to disable L0s though. I believe AR2425 would be one which
>> >> likely had L0s enabled but requires it to be disabled. Not sure of
>> >> others. But this is why I am saying this can be done globally for all
>> >> ath5k chipsets.
>> >
>> > If L1 is set but the chip is pre-PCIe 1.1 then we'll disable L1 unless
>> > the driver tells us that it's functional. The .inf from the Windows
>> > driver seemed to suggest that only a subset of the chips re-enabled L1
>> > there, but if it's ok in general then that's a straightforward one-line
>> > patch.
>>
>> But why can't we just rely on what the device already has on its PCI
>> config space and only ensure to disable L0s?
>
> Because we globally disable ASPM on pre-1.1 devices, because that's what
> Windows does. It makes it easier for us to figure out what level of
> support we can expect from different hardware revisions.

I see.. thanks Mathew... in that case since L1 works on all devices we
could just force enable L1 for all PCIE devices. What do you think?

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Matthew Garrett]

On Mon, Jul 26, 2010 at 03:43:04PM -0700, Luis R. Rodriguez wrote:

> I see.. thanks Mathew... in that case since L1 works on all devices we
> could just force enable L1 for all PCIE devices. What do you think?

Works for me.

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

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Mon, Jul 26, 2010 at 01:13:22PM -0700, Luis R. Rodriguez wrote:
> On Sat, Jun 19, 2010 at 08:32:44AM -0700, Maxim Levitsky wrote:
> > On Sat, 2010-06-19 at 16:02 +0300, Maxim Levitsky wrote: 
> > > On Sat, 2010-06-19 at 08:38 -0400, Bob Copeland wrote: 
> > > > On Sat, Jun 19, 2010 at 10:49:34AM +0300, Maxim Levitsky wrote:
> > > > > How this patch?
> > > > 
> > > > Looks fine to me.  Some nitpicking below but feel free to add my
> > > > 
> > > > Acked-by: Bob Copeland <me@bobcopeland.com>
> > > > 
> > Done.
> > 
> > Best regards,
> > Maxim Levitsky
> > 
> > --- 
> > 
> > commit 616afa397b3e843f2aba06be12a30e72dfff7740
> > Author: Maxim Levitsky <maximlevitsky@gmail.com>
> > Date:   Thu Jun 17 23:21:42 2010 +0300
> > 
> >     ath5k: disable ASPM
> >     
> >     Atheros card on Acer Aspire One (AOA150, Atheros Communications Inc. AR5001
> >     Wireless Network Adapter [168c:001c] (rev 01)) doesn't work well with ASPM
> >     enabled. With ASPM ath5k will eventually stall on heavy traffic with often
> >     'unsupported jumbo' warnings appearing. Disabling ASPM L0s in ath5k fixes
> >     these problems.
> >     Also card sends a storm of RXORN interrupts even though medium is idle.
> >     
> >     Reproduced with pcie_aspm=force and by using 'nc < /dev/zero > /dev/null' at
> >     both ends (usually stalls within seconds).
> >     
> >     Unfortunately BIOS enables ASPM on this card by default on these machines
> >     This means that, problem shows up (less often) without pcie_aspm=force too. 
> >     Therefore to benefit from this fix you need to _enable_ CONFIG_PCIEASPM
> >     
> > 
> >     All credit for this patch goes to Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >     for finding and fixing this bug.
> >     
> >     Based on patch that is
> >     From: Jussi Kivilinna <jussi.kivilinna@mbnet.fi>
> >     
> >     
> >     Signed-off-by: Maxim Levitsky <maximlevitsky@gmail.com>
> >     Acked-by: Bob Copeland <me@bobcopeland.com>
> 
> Acked-by: Luis R. Rodriguez <lrodriguez@atheros.com>

So NACK now, to really fix this correctly its best to instead disable L0s but
to force enable also L1 if the device is a PCI Express device. All Atheros legacy
devices (ath5k) should work correctly with L1. Kernels with CONFIG_PCIEASPM
would end up disabling even L1 for pre PCI 1.1 devices.

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Maxim Levitsky]

On Mon, 2010-07-26 at 23:50 +0100, Matthew Garrett wrote: 
> On Mon, Jul 26, 2010 at 03:43:04PM -0700, Luis R. Rodriguez wrote:
> 
> > I see.. thanks Mathew... in that case since L1 works on all devices we
> > could just force enable L1 for all PCIE devices. What do you think?
> 
> Works for me.
> 

On the second thought, there is no 'pci_enable_link_state' :-)
I afraid that if I add it, I might not do that right for all cases, thus
do more harm that good...

Best regards,
Maxim Levitsky

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Tue, Jul 27, 2010 at 2:35 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Mon, 2010-07-26 at 23:50 +0100, Matthew Garrett wrote:
>> On Mon, Jul 26, 2010 at 03:43:04PM -0700, Luis R. Rodriguez wrote:
>>
>> > I see.. thanks Mathew... in that case since L1 works on all devices we
>> > could just force enable L1 for all PCIE devices. What do you think?
>>
>> Works for me.
>>
>
> On the second thought, there is no 'pci_enable_link_state' :-)
> I afraid that if I add it, I might not do that right for all cases, thus
> do more harm that good...

I'm sorry, can you elaborate?

  Luis

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Maxim Levitsky]

On Tue, 2010-07-27 at 08:57 -0700, Luis R. Rodriguez wrote: 
> On Tue, Jul 27, 2010 at 2:35 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > On Mon, 2010-07-26 at 23:50 +0100, Matthew Garrett wrote:
> >> On Mon, Jul 26, 2010 at 03:43:04PM -0700, Luis R. Rodriguez wrote:
> >>
> >> > I see.. thanks Mathew... in that case since L1 works on all devices we
> >> > could just force enable L1 for all PCIE devices. What do you think?
> >>
> >> Works for me.
> >>
> >
> > On the second thought, there is no 'pci_enable_link_state' :-)
> > I afraid that if I add it, I might not do that right for all cases, thus
> > do more harm that good...
> 
> I'm sorry, can you elaborate?

I mean ASPM code doesn't have a function to undo effects of the
blacklist (due to pre 1.1 pcie device).

Its not that simple to write such function.

Best regards,
Maxim Levitsky

Re: [ath5k-devel] [PATCH v3] ath5k: disable ASPM

[Luis R. Rodriguez]

On Wed, Jul 28, 2010 at 4:48 PM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> On Tue, 2010-07-27 at 08:57 -0700, Luis R. Rodriguez wrote:
>> On Tue, Jul 27, 2010 at 2:35 AM, Maxim Levitsky <maximlevitsky@gmail.com> wrote:
>> > On Mon, 2010-07-26 at 23:50 +0100, Matthew Garrett wrote:
>> >> On Mon, Jul 26, 2010 at 03:43:04PM -0700, Luis R. Rodriguez wrote:
>> >>
>> >> > I see.. thanks Mathew... in that case since L1 works on all devices we
>> >> > could just force enable L1 for all PCIE devices. What do you think?
>> >>
>> >> Works for me.
>> >>
>> >
>> > On the second thought, there is no 'pci_enable_link_state' :-)
>> > I afraid that if I add it, I might not do that right for all cases, thus
>> > do more harm that good...
>>
>> I'm sorry, can you elaborate?
>
> I mean ASPM code doesn't have a function to undo effects of the
> blacklist (due to pre 1.1 pcie device).
>
> Its not that simple to write such function.

Ah good catch... pcie_aspm_sanity_check() will actually be used to
adjust the device link capability... So even if we do try to insist it
wouldn't work. I'm happy if we deal with this separately, its a
reasonable compromise to fix issues with existing devices out there.

  Luis