netdev - Re: [PATCH net] r8169: switch off ASPM by default and add sysfs attribute to control ASPM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2698cdee-91b9-8955-5b29-b5ba4e656526@gmail.com>
Date:   Tue, 9 Apr 2019 19:32:15 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Bjorn Helgaas <helgaas@...nel.org>,
        Frederick Lawler <fred@...dlawl.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        Rajat Jain <rajatja@...gle.com>
Subject: Re: [PATCH net] r8169: switch off ASPM by default and add sysfs
 attribute to control ASPM

On 05.04.2019 21:28, Heiner Kallweit wrote:
> On 05.04.2019 21:10, Bjorn Helgaas wrote:
>> On Wed, Apr 03, 2019 at 07:45:29PM +0200, Heiner Kallweit wrote:
>>> On 03.04.2019 15:14, Bjorn Helgaas wrote:
>>>> On Wed, Apr 03, 2019 at 07:53:40AM +0200, Heiner Kallweit wrote:
>>>>> On 02.04.2019 23:57, Bjorn Helgaas wrote:
>>>>>> On Tue, Apr 02, 2019 at 10:41:20PM +0200, Heiner Kallweit wrote:
>>>>>>> On 02.04.2019 22:16, Florian Fainelli wrote:
>>>>>>>> On 4/2/19 12:55 PM, Heiner Kallweit wrote:
>>>>>>>>> There are numerous reports about different problems caused by
>>>>>>>>> ASPM incompatibilities between certain network chip versions
>>>>>>>>> and board chipsets. On the other hand on (especially mobile)
>>>>>>>>> systems where ASPM works properly it can significantly
>>>>>>>>> contribute to power-saving and increased battery runtime.
>>>>>>>>> One problem so far to make ASPM configurable was to find an
>>>>>>>>> acceptable way of configuration (e.g. module parameters are
>>>>>>>>> discouraged for that purpose).
>>
>>>>>>>>> +Certain combinations of network chip versions and board
>>>>>>>>> +chipsets result in increased packet latency, PCIe errors, or
>>>>>>>>> +significantly reduced network performance. Therefore ASPM is
>>>>>>>>> +off by default. On the other hand ASPM can significantly
>>>>>>>>> +contribute to power-saving and thus increased battery runtime
>>>>>>>>> +on notebooks.
>>
>>>> That said, I think Frederick has already started working on a plan
>>>> for the PCI core to expose sysfs files to manage ASPM.  This is
>>>> similar to the link_state files enabled by CONFIG_PCIEASPM_DEBUG,
>>>> but it will be always enabled and probably structured slightly
>>>> differently.  The idea is that this would be generic and would not
>>>> require any driver support.
>>
>>> Frederick, is there anything you could share already? Or any timeline?
>>> Based on Bjorns info what seems to be best to me:
>>> 1. Disable ASPM for r8169 on stable (back to 4.19).
>>> 2. Once the generic ASPM sysfs attributes are available, reenable ASPM
>>>    for r8169 in net-next.
>>
>> This is out of my wheelhouse, but even with a generic sysfs knob, it
>> doesn't sound like a good idea to me to enable ASPM by default for
>> r8169 if we think it's unreliable on any significant fraction of
>> machines.
>>
> I was a little bit imprecise. With the second statement I wanted to say:
> Keep ASPM disabled per default, but make it possible that setting the
> new sysfs attribute enables ASPM. After digging deeper in the ASPM core
> code it seems however that we don't even have to touch the driver later.
> 

ASPM has been disabled again for r8169: b75bb8a5b755 ("r8169: disable ASPM
again"). So, coming back to controlling ASPM via sysfs:
My first thought would be to extend pci_disable_link_state with support
for disabling L1.1/L1.2, and then basically expose pci_disable_link_state
via sysfs (attribute reading being handled with a direct read from
pcie_link_state->aspm_disable).

Is this what you were planning or do you have some other approach in mind?

>> Users of those unreliable machines will see poor performance, PCIe
>> errors, etc, and they won't have a clue about how to fix them.
>>
>> To me it sounds better to leave ASPM disabled by default for r8169,
>> then incrementally whitelist systems that are known to work.  Users
>> will have poor battery life, but at least things will work reliably.
>>
>> Bjorn
>>
> Heiner
> 
Heiner