NACK/cmnt: [SRU] [A] [PATCH 0/7] Fix Runtime PM for r8169

[Kai-Heng Feng]

Stefan Bader <stefan.bader at canonical.com> wrote:

> On 21.03.2018 14:08, Kai-Heng Feng wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1757422
>>
>> [Impact]
>> r8169 stays in D0 even when no ethernet cable is plugged in. This drains
>> lots of power (~3W). The tested laptop uses 5.5W when r8169 is in D0,
>> 1.8W when r8169 is in D3. The power saved is substantial.
>>
>> [Fix]
>> Improved rumtime PM logic to let the device gets suspended (D3) when the
>> port is not in used and the link is down.
>>
>> [Test Case]
>> The chip version is 8168h/8111h.
>> Test when no ethernet gets plugged.
>>
>> Powertop shows power consumption is roughly 5.5W.
>> lspci shows the device is in D0.
>>
>> With the patch,
>> The power consumption is reduced to 1.8W.
>> lspci shows the device is in D3, PME# is correctly enabled.
>> Plug ethernet cable can corretly wake up the device.
>> Unplug the cable, the device gets suspended to D3 correctly.
>>
>> [Regression Potential]
>> Medium.
>> - r8169 is so ubiquitous, with lots of different chip versions. It's
>>   hard to test all of them.
>> - PCI D3 needs system firmware (ACPI) support, this might hit some
>>   plaform bugs.
>> - the code is still in v4.16-rc*, so it's not well tested by end users.
>>
>> If it's too risky to merge this series into Artful, it should still get
>> merged for Bionic, the power consumption reduction is significant.
>>
>> Daniel Drake (1):
>>   r8169: only enable PCI wakeups when WOL is active
>>
>> Heiner Kallweit (6):
>>   PCI: Add pcim_set_mwi(), a device-managed pci_set_mwi()
>>   r8169: switch to device-managed functions in probe
>>   r8169: remove netif_napi_del in probe error path
>>   r8169: remove unneeded rpm ops in rtl_shutdown
>>   r8169: improve runtime pm in rtl8169_check_link_status
>>   r8169: improve runtime pm in general and suspend unused ports
>>
>>  drivers/net/ethernet/realtek/r8169.c | 131 ++++++++++-------------------------
>>  drivers/pci/pci.c                    |  25 +++++++
>>  include/linux/pci.h                  |   1 +
>>  3 files changed, 62 insertions(+), 95 deletions(-)
> At least for now until this would have settled within Bionic. Just feels  
> like
> there could be too many possible problems.

Yea I guess it's too risky to include for Bionic.
That said, we still need this for Bionic based linux-oem.

When the time 18.10 is release, the patch should be well-tested.

>
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team