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

From: Heiner Kallweit <hkallweit1@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Frederick Lawler <fred@fredlawl.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	David Miller <davem@davemloft.net>,
	Realtek linux nic maintainers <nic_swsd@realtek.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Rajat Jain <rajatja@google.com>
Subject: Re: [PATCH net] r8169: switch off ASPM by default and add sysfs attribute to control ASPM
Date: Fri, 5 Apr 2019 21:28:24 +0200	[thread overview]
Message-ID: <58f445a2-1eb3-7759-62d6-efacd87d52b6@gmail.com> (raw)
In-Reply-To: <20190405191024.GD26522@google.com>

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.

> 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