[SRU,Bionic,PULL,00/62] Fix Runtime PM for r8169

Message ID 20180625065619.17426-1-kai.heng.feng@canonical.com
State New
Headers show

Pull-request

git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm

Message

Kai-Heng Feng June 25, 2018, 6:56 a.m.
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.

Sync r8169 driver with mainline, with one fix from net-next.
Everything is cleanly cherry-picked.
This means the fix for LP: #1752772 is dropped and re-cherry-picked.

[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]
Low.
The code was in mainline for a while, it should be well-tested by now.

The following changes since commit 123dad8e7f35b815fdf6d0647b056c096f14d052:

  ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device (2018-06-18 14:57:01 -0400)

are available in the Git repository at:

  git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm

for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:

  r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)

----------------------------------------------------------------
Andy Shevchenko (2):
      r8169: Dereference MMIO address immediately before use
      r8169: switch to device-managed functions in probe (part 2)

Heiner Kallweit (55):
      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
      r8169: remove some WOL-related dead code
      r8169: remove not needed PHY soft reset in rtl8168e_2_hw_phy_config
      r8169: disable WOL per default
      r8169: simplify and improve check for dash
      r8169: improve interrupt handling
      r8169: convert remaining feature flag and remove enum features
      r8169: fix interrupt number after adding support for MSI-X interrupts
      r8169: simplify rtl_set_mac_address
      r8169: change type of first argument in rtl_tx_performance_tweak
      r8169: change type of argument in rtl_disable/enable_clock_request
      r8169: add helper tp_to_dev
      PCI: Add two more values for PCIe Max_Read_Request_Size
      r8169: replace magic numbers with PCI MRRS constant
      r8169: remove unused member features from struct
      r8169: remove member align from struct rtl_cfg_info
      r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
      r8169: use constant NAPI_POLL_WAIT
      r8169: switch to napi_schedule_irqoff
      r8169: simplify rtl8169_alloc_rx_data
      r8169: improve rtl8169_init_ring
      r8169: remove unneeded check in rtl8169_rx_fill
      r8169: replace rx_buf_sz with a constant
      r8169: remove rtl8169_map_to_asic
      r8169: change hw_start argument type
      r8169: change argument type of counters handling functions
      r8169: change interrupt handler argument type
      r8169: drop member opts1_mask from struct rtl8169_private
      r8169: don't display tp->mmio_addr address
      r8169: improve rtl8169_get_mac_version
      r8169: drop member txd_version from struct rtl8169_private
      r8169: improve pci region handling
      r8169: remove jumbo_tx_csum from chip config struct
      r8169: don't use netif_info et al before net_device has been registered
      r8169: remove unneeded call to __rtl8169_set_features in rtl_open
      r8169: improve rtl8169_set_features
      r8169: replace magic number for INTT mask with a constant
      r8169: improve CPlusCmd handling
      r8169: improve handling of CPCMD quirk mask
      r8169: simplify rtl_hw_start_8169
      r8169: remove calls to rtl_set_rx_mode
      r8169: move common initializations to tp->hw_start
      r8169: remove unneeded check in r8168_pll_power_down
      r8169: remove 810x_phy_power_up/down
      r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
      r8169: drop member pll_power_ops from struct rtl8169_private
      r8169: simplify code by using ranges in switch clauses
      r8169: replace longer if statements with switch statements
      r8169: drop rtl_generic_op
      r8169: improve PCI config space access
      r8169: avoid potentially misaligned access when getting mac address
      r8169: replace get_protocol with vlan_get_protocol
      r8169: fix network error on resume from suspend

Kai-Heng Feng (4):
      Revert "r8169: fix interrupt number after adding support for MSI-X interrupts"
      Revert "r8169: improve interrupt handling"
      Revert "r8169: disable WOL per default"
      Revert "r8169: remove some WOL-related dead code"

Ville Syrjälä (1):
      r8169: Fix netpoll oops

 drivers/net/ethernet/realtek/r8169.c | 2101 +++++++++++-----------------------
 include/uapi/linux/pci_regs.h        |    2 +
 2 files changed, 649 insertions(+), 1454 deletions(-)

Comments

Stefan Bader June 25, 2018, 7:24 a.m. | #1
On 25.06.2018 08:56, 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.
> 
> Sync r8169 driver with mainline, with one fix from net-next.
> Everything is cleanly cherry-picked.
> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
> 
> [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]
> Low.
> The code was in mainline for a while, it should be well-tested by now.
> 
> The following changes since commit 123dad8e7f35b815fdf6d0647b056c096f14d052:
> 
>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device (2018-06-18 14:57:01 -0400)
> 
> are available in the Git repository at:
> 
>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
> 
> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
> 
>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
> 
> ----------------------------------------------------------------
> Andy Shevchenko (2):
>       r8169: Dereference MMIO address immediately before use
>       r8169: switch to device-managed functions in probe (part 2)
> 
> Heiner Kallweit (55):
>       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
>       r8169: remove some WOL-related dead code
>       r8169: remove not needed PHY soft reset in rtl8168e_2_hw_phy_config
>       r8169: disable WOL per default
>       r8169: simplify and improve check for dash
>       r8169: improve interrupt handling
>       r8169: convert remaining feature flag and remove enum features
>       r8169: fix interrupt number after adding support for MSI-X interrupts
>       r8169: simplify rtl_set_mac_address
>       r8169: change type of first argument in rtl_tx_performance_tweak
>       r8169: change type of argument in rtl_disable/enable_clock_request
>       r8169: add helper tp_to_dev
>       PCI: Add two more values for PCIe Max_Read_Request_Size
>       r8169: replace magic numbers with PCI MRRS constant
>       r8169: remove unused member features from struct
>       r8169: remove member align from struct rtl_cfg_info
>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>       r8169: use constant NAPI_POLL_WAIT
>       r8169: switch to napi_schedule_irqoff
>       r8169: simplify rtl8169_alloc_rx_data
>       r8169: improve rtl8169_init_ring
>       r8169: remove unneeded check in rtl8169_rx_fill
>       r8169: replace rx_buf_sz with a constant
>       r8169: remove rtl8169_map_to_asic
>       r8169: change hw_start argument type
>       r8169: change argument type of counters handling functions
>       r8169: change interrupt handler argument type
>       r8169: drop member opts1_mask from struct rtl8169_private
>       r8169: don't display tp->mmio_addr address
>       r8169: improve rtl8169_get_mac_version
>       r8169: drop member txd_version from struct rtl8169_private
>       r8169: improve pci region handling
>       r8169: remove jumbo_tx_csum from chip config struct
>       r8169: don't use netif_info et al before net_device has been registered
>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>       r8169: improve rtl8169_set_features
>       r8169: replace magic number for INTT mask with a constant
>       r8169: improve CPlusCmd handling
>       r8169: improve handling of CPCMD quirk mask
>       r8169: simplify rtl_hw_start_8169
>       r8169: remove calls to rtl_set_rx_mode
>       r8169: move common initializations to tp->hw_start
>       r8169: remove unneeded check in r8168_pll_power_down
>       r8169: remove 810x_phy_power_up/down
>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>       r8169: drop member pll_power_ops from struct rtl8169_private
>       r8169: simplify code by using ranges in switch clauses
>       r8169: replace longer if statements with switch statements
>       r8169: drop rtl_generic_op
>       r8169: improve PCI config space access
>       r8169: avoid potentially misaligned access when getting mac address
>       r8169: replace get_protocol with vlan_get_protocol
>       r8169: fix network error on resume from suspend
> 
> Kai-Heng Feng (4):
>       Revert "r8169: fix interrupt number after adding support for MSI-X interrupts"
>       Revert "r8169: improve interrupt handling"
>       Revert "r8169: disable WOL per default"
>       Revert "r8169: remove some WOL-related dead code"
> 
> Ville Syrjälä (1):
>       r8169: Fix netpoll oops
> 
>  drivers/net/ethernet/realtek/r8169.c | 2101 +++++++++++-----------------------
>  include/uapi/linux/pci_regs.h        |    2 +
>  2 files changed, 649 insertions(+), 1454 deletions(-)
> 
Though this is isolated to one driver, it is a driver that is in wide-spread
use. The risk of regression is to high to justify pulling in that huge delta.
Even more for PM reasons. This is what HWE kernels are for.

-Stefan
Kai-Heng Feng June 25, 2018, 7:31 a.m. | #2
at 15:24, Stefan Bader <stefan.bader@canonical.com> wrote:

> On 25.06.2018 08:56, 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.
>>
>> Sync r8169 driver with mainline, with one fix from net-next.
>> Everything is cleanly cherry-picked.
>> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
>>
>> [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]
>> Low.
>> The code was in mainline for a while, it should be well-tested by now.
>>
>> The following changes since commit  
>> 123dad8e7f35b815fdf6d0647b056c096f14d052:
>>
>>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device (2018-06-18 14:57:01 -0400)
>>
>> are available in the Git repository at:
>>
>>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
>>
>> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
>>
>>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
>>
>> ----------------------------------------------------------------
>> Andy Shevchenko (2):
>>       r8169: Dereference MMIO address immediately before use
>>       r8169: switch to device-managed functions in probe (part 2)
>>
>> Heiner Kallweit (55):
>>       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
>>       r8169: remove some WOL-related dead code
>>       r8169: remove not needed PHY soft reset in rtl8168e_2_hw_phy_config
>>       r8169: disable WOL per default
>>       r8169: simplify and improve check for dash
>>       r8169: improve interrupt handling
>>       r8169: convert remaining feature flag and remove enum features
>>       r8169: fix interrupt number after adding support for MSI-X interrupts
>>       r8169: simplify rtl_set_mac_address
>>       r8169: change type of first argument in rtl_tx_performance_tweak
>>       r8169: change type of argument in rtl_disable/enable_clock_request
>>       r8169: add helper tp_to_dev
>>       PCI: Add two more values for PCIe Max_Read_Request_Size
>>       r8169: replace magic numbers with PCI MRRS constant
>>       r8169: remove unused member features from struct
>>       r8169: remove member align from struct rtl_cfg_info
>>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>>       r8169: use constant NAPI_POLL_WAIT
>>       r8169: switch to napi_schedule_irqoff
>>       r8169: simplify rtl8169_alloc_rx_data
>>       r8169: improve rtl8169_init_ring
>>       r8169: remove unneeded check in rtl8169_rx_fill
>>       r8169: replace rx_buf_sz with a constant
>>       r8169: remove rtl8169_map_to_asic
>>       r8169: change hw_start argument type
>>       r8169: change argument type of counters handling functions
>>       r8169: change interrupt handler argument type
>>       r8169: drop member opts1_mask from struct rtl8169_private
>>       r8169: don't display tp->mmio_addr address
>>       r8169: improve rtl8169_get_mac_version
>>       r8169: drop member txd_version from struct rtl8169_private
>>       r8169: improve pci region handling
>>       r8169: remove jumbo_tx_csum from chip config struct
>>       r8169: don't use netif_info et al before net_device has been registered
>>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>>       r8169: improve rtl8169_set_features
>>       r8169: replace magic number for INTT mask with a constant
>>       r8169: improve CPlusCmd handling
>>       r8169: improve handling of CPCMD quirk mask
>>       r8169: simplify rtl_hw_start_8169
>>       r8169: remove calls to rtl_set_rx_mode
>>       r8169: move common initializations to tp->hw_start
>>       r8169: remove unneeded check in r8168_pll_power_down
>>       r8169: remove 810x_phy_power_up/down
>>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>>       r8169: drop member pll_power_ops from struct rtl8169_private
>>       r8169: simplify code by using ranges in switch clauses
>>       r8169: replace longer if statements with switch statements
>>       r8169: drop rtl_generic_op
>>       r8169: improve PCI config space access
>>       r8169: avoid potentially misaligned access when getting mac address
>>       r8169: replace get_protocol with vlan_get_protocol
>>       r8169: fix network error on resume from suspend
>>
>> Kai-Heng Feng (4):
>>       Revert "r8169: fix interrupt number after adding support for MSI-X interrupts"
>>       Revert "r8169: improve interrupt handling"
>>       Revert "r8169: disable WOL per default"
>>       Revert "r8169: remove some WOL-related dead code"
>>
>> Ville Syrjälä (1):
>>       r8169: Fix netpoll oops
>>
>>  drivers/net/ethernet/realtek/r8169.c | 2101 +++++++++++-----------------------
>>  include/uapi/linux/pci_regs.h        |    2 +
>>  2 files changed, 649 insertions(+), 1454 deletions(-)
> Though this is isolated to one driver, it is a driver that is in  
> wide-spread
> use. The risk of regression is to high to justify pulling in that huge  
> delta.
> Even more for PM reasons. This is what HWE kernels are for.

Runtime PM is enabled for 4.13 based linux-oem, which will upgrade to  
Bionic's 4.15 soon.
This transition will regress those who uses 4.13 based kernel.

If this series can't get pulled in, then we should probably make it upgrade  
to 4.15 based linux-oem kernel instead of Bionic's kernel.

I'll discuss with other HWE members to find a resolution.

Kai-Heng

>
> -Stefan
Timo Aaltonen June 25, 2018, 7:52 a.m. | #3
On 25.06.2018 10:31, Kai-Heng Feng wrote:
> at 15:24, Stefan Bader <stefan.bader@canonical.com> wrote:
> 
>> On 25.06.2018 08:56, 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.
>>>
>>> Sync r8169 driver with mainline, with one fix from net-next.
>>> Everything is cleanly cherry-picked.
>>> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
>>>
>>> [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]
>>> Low.
>>> The code was in mainline for a while, it should be well-tested by now.
>>>
>>> The following changes since commit
>>> 123dad8e7f35b815fdf6d0647b056c096f14d052:
>>>
>>>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device
>>> (2018-06-18 14:57:01 -0400)
>>>
>>> are available in the Git repository at:
>>>
>>>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
>>>
>>> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
>>>
>>>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
>>>
>>> ----------------------------------------------------------------
>>> Andy Shevchenko (2):
>>>       r8169: Dereference MMIO address immediately before use
>>>       r8169: switch to device-managed functions in probe (part 2)
>>>
>>> Heiner Kallweit (55):
>>>       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
>>>       r8169: remove some WOL-related dead code
>>>       r8169: remove not needed PHY soft reset in
>>> rtl8168e_2_hw_phy_config
>>>       r8169: disable WOL per default
>>>       r8169: simplify and improve check for dash
>>>       r8169: improve interrupt handling
>>>       r8169: convert remaining feature flag and remove enum features
>>>       r8169: fix interrupt number after adding support for MSI-X
>>> interrupts
>>>       r8169: simplify rtl_set_mac_address
>>>       r8169: change type of first argument in rtl_tx_performance_tweak
>>>       r8169: change type of argument in rtl_disable/enable_clock_request
>>>       r8169: add helper tp_to_dev
>>>       PCI: Add two more values for PCIe Max_Read_Request_Size
>>>       r8169: replace magic numbers with PCI MRRS constant
>>>       r8169: remove unused member features from struct
>>>       r8169: remove member align from struct rtl_cfg_info
>>>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>>>       r8169: use constant NAPI_POLL_WAIT
>>>       r8169: switch to napi_schedule_irqoff
>>>       r8169: simplify rtl8169_alloc_rx_data
>>>       r8169: improve rtl8169_init_ring
>>>       r8169: remove unneeded check in rtl8169_rx_fill
>>>       r8169: replace rx_buf_sz with a constant
>>>       r8169: remove rtl8169_map_to_asic
>>>       r8169: change hw_start argument type
>>>       r8169: change argument type of counters handling functions
>>>       r8169: change interrupt handler argument type
>>>       r8169: drop member opts1_mask from struct rtl8169_private
>>>       r8169: don't display tp->mmio_addr address
>>>       r8169: improve rtl8169_get_mac_version
>>>       r8169: drop member txd_version from struct rtl8169_private
>>>       r8169: improve pci region handling
>>>       r8169: remove jumbo_tx_csum from chip config struct
>>>       r8169: don't use netif_info et al before net_device has been
>>> registered
>>>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>>>       r8169: improve rtl8169_set_features
>>>       r8169: replace magic number for INTT mask with a constant
>>>       r8169: improve CPlusCmd handling
>>>       r8169: improve handling of CPCMD quirk mask
>>>       r8169: simplify rtl_hw_start_8169
>>>       r8169: remove calls to rtl_set_rx_mode
>>>       r8169: move common initializations to tp->hw_start
>>>       r8169: remove unneeded check in r8168_pll_power_down
>>>       r8169: remove 810x_phy_power_up/down
>>>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>>>       r8169: drop member pll_power_ops from struct rtl8169_private
>>>       r8169: simplify code by using ranges in switch clauses
>>>       r8169: replace longer if statements with switch statements
>>>       r8169: drop rtl_generic_op
>>>       r8169: improve PCI config space access
>>>       r8169: avoid potentially misaligned access when getting mac
>>> address
>>>       r8169: replace get_protocol with vlan_get_protocol
>>>       r8169: fix network error on resume from suspend
>>>
>>> Kai-Heng Feng (4):
>>>       Revert "r8169: fix interrupt number after adding support for
>>> MSI-X interrupts"
>>>       Revert "r8169: improve interrupt handling"
>>>       Revert "r8169: disable WOL per default"
>>>       Revert "r8169: remove some WOL-related dead code"
>>>
>>> Ville Syrjälä (1):
>>>       r8169: Fix netpoll oops
>>>
>>>  drivers/net/ethernet/realtek/r8169.c | 2101
>>> +++++++++++-----------------------
>>>  include/uapi/linux/pci_regs.h        |    2 +
>>>  2 files changed, 649 insertions(+), 1454 deletions(-)
>> Though this is isolated to one driver, it is a driver that is in
>> wide-spread
>> use. The risk of regression is to high to justify pulling in that huge
>> delta.
>> Even more for PM reasons. This is what HWE kernels are for.
> 
> Runtime PM is enabled for 4.13 based linux-oem, which will upgrade to
> Bionic's 4.15 soon.
> This transition will regress those who uses 4.13 based kernel.
> 
> If this series can't get pulled in, then we should probably make it
> upgrade to 4.15 based linux-oem kernel instead of Bionic's kernel.

There is no 4.15 based linux-oem in xenial..
Stefan Bader June 25, 2018, 9:10 a.m. | #4
On 25.06.2018 09:24, Stefan Bader wrote:
> On 25.06.2018 08:56, 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.
>>
>> Sync r8169 driver with mainline, with one fix from net-next.
>> Everything is cleanly cherry-picked.
>> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
>>
>> [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]
>> Low.
>> The code was in mainline for a while, it should be well-tested by now.
>>
>> The following changes since commit 123dad8e7f35b815fdf6d0647b056c096f14d052:
>>
>>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device (2018-06-18 14:57:01 -0400)
>>
>> are available in the Git repository at:
>>
>>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
>>
>> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
>>
>>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
>>
>> ----------------------------------------------------------------
>> Andy Shevchenko (2):
>>       r8169: Dereference MMIO address immediately before use
>>       r8169: switch to device-managed functions in probe (part 2)
>>
>> Heiner Kallweit (55):
>>       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
>>       r8169: remove some WOL-related dead code
>>       r8169: remove not needed PHY soft reset in rtl8168e_2_hw_phy_config
>>       r8169: disable WOL per default
>>       r8169: simplify and improve check for dash
>>       r8169: improve interrupt handling
>>       r8169: convert remaining feature flag and remove enum features
>>       r8169: fix interrupt number after adding support for MSI-X interrupts
>>       r8169: simplify rtl_set_mac_address
>>       r8169: change type of first argument in rtl_tx_performance_tweak
>>       r8169: change type of argument in rtl_disable/enable_clock_request
>>       r8169: add helper tp_to_dev
>>       PCI: Add two more values for PCIe Max_Read_Request_Size
>>       r8169: replace magic numbers with PCI MRRS constant
>>       r8169: remove unused member features from struct
>>       r8169: remove member align from struct rtl_cfg_info
>>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>>       r8169: use constant NAPI_POLL_WAIT
>>       r8169: switch to napi_schedule_irqoff
>>       r8169: simplify rtl8169_alloc_rx_data
>>       r8169: improve rtl8169_init_ring
>>       r8169: remove unneeded check in rtl8169_rx_fill
>>       r8169: replace rx_buf_sz with a constant
>>       r8169: remove rtl8169_map_to_asic
>>       r8169: change hw_start argument type
>>       r8169: change argument type of counters handling functions
>>       r8169: change interrupt handler argument type
>>       r8169: drop member opts1_mask from struct rtl8169_private
>>       r8169: don't display tp->mmio_addr address
>>       r8169: improve rtl8169_get_mac_version
>>       r8169: drop member txd_version from struct rtl8169_private
>>       r8169: improve pci region handling
>>       r8169: remove jumbo_tx_csum from chip config struct
>>       r8169: don't use netif_info et al before net_device has been registered
>>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>>       r8169: improve rtl8169_set_features
>>       r8169: replace magic number for INTT mask with a constant
>>       r8169: improve CPlusCmd handling
>>       r8169: improve handling of CPCMD quirk mask
>>       r8169: simplify rtl_hw_start_8169
>>       r8169: remove calls to rtl_set_rx_mode
>>       r8169: move common initializations to tp->hw_start
>>       r8169: remove unneeded check in r8168_pll_power_down
>>       r8169: remove 810x_phy_power_up/down
>>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>>       r8169: drop member pll_power_ops from struct rtl8169_private
>>       r8169: simplify code by using ranges in switch clauses
>>       r8169: replace longer if statements with switch statements
>>       r8169: drop rtl_generic_op
>>       r8169: improve PCI config space access
>>       r8169: avoid potentially misaligned access when getting mac address
>>       r8169: replace get_protocol with vlan_get_protocol
>>       r8169: fix network error on resume from suspend
>>
>> Kai-Heng Feng (4):
>>       Revert "r8169: fix interrupt number after adding support for MSI-X interrupts"
>>       Revert "r8169: improve interrupt handling"
>>       Revert "r8169: disable WOL per default"
>>       Revert "r8169: remove some WOL-related dead code"
>>
>> Ville Syrjälä (1):
>>       r8169: Fix netpoll oops
>>
>>  drivers/net/ethernet/realtek/r8169.c | 2101 +++++++++++-----------------------
>>  include/uapi/linux/pci_regs.h        |    2 +
>>  2 files changed, 649 insertions(+), 1454 deletions(-)
>>
> Though this is isolated to one driver, it is a driver that is in wide-spread
> use. The risk of regression is to high to justify pulling in that huge delta.
> Even more for PM reasons. This is what HWE kernels are for.

Hi Kai,

I did read the email from Anthony later, so I understand that this is something
Dell is pushing for. But still it feels scary to make that many changes to a
driver that is in common use. My initial reaction this morning was to reject it
(which I did). And the feeling is still like that. There might be non-technical
reasons to make me change my mind. Lets see. Just wanted to let you know in case
my public reply is sounding too harsh.

-Stefan
> 
> -Stefan
> 
> 
>
Stefan Bader June 25, 2018, 9:52 a.m. | #5
On 25.06.2018 09:52, Timo Aaltonen wrote:
> On 25.06.2018 10:31, Kai-Heng Feng wrote:
>> at 15:24, Stefan Bader <stefan.bader@canonical.com> wrote:
>>
>>> On 25.06.2018 08:56, 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.
>>>>
>>>> Sync r8169 driver with mainline, with one fix from net-next.
>>>> Everything is cleanly cherry-picked.
>>>> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
>>>>
>>>> [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]
>>>> Low.
>>>> The code was in mainline for a while, it should be well-tested by now.
>>>>
>>>> The following changes since commit
>>>> 123dad8e7f35b815fdf6d0647b056c096f14d052:
>>>>
>>>>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device
>>>> (2018-06-18 14:57:01 -0400)
>>>>
>>>> are available in the Git repository at:
>>>>
>>>>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
>>>>
>>>> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
>>>>
>>>>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
>>>>
>>>> ----------------------------------------------------------------
>>>> Andy Shevchenko (2):
>>>>       r8169: Dereference MMIO address immediately before use
>>>>       r8169: switch to device-managed functions in probe (part 2)
>>>>
>>>> Heiner Kallweit (55):
>>>>       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
>>>>       r8169: remove some WOL-related dead code
>>>>       r8169: remove not needed PHY soft reset in
>>>> rtl8168e_2_hw_phy_config
>>>>       r8169: disable WOL per default
>>>>       r8169: simplify and improve check for dash
>>>>       r8169: improve interrupt handling
>>>>       r8169: convert remaining feature flag and remove enum features
>>>>       r8169: fix interrupt number after adding support for MSI-X
>>>> interrupts
>>>>       r8169: simplify rtl_set_mac_address
>>>>       r8169: change type of first argument in rtl_tx_performance_tweak
>>>>       r8169: change type of argument in rtl_disable/enable_clock_request
>>>>       r8169: add helper tp_to_dev
>>>>       PCI: Add two more values for PCIe Max_Read_Request_Size
>>>>       r8169: replace magic numbers with PCI MRRS constant
>>>>       r8169: remove unused member features from struct
>>>>       r8169: remove member align from struct rtl_cfg_info
>>>>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>>>>       r8169: use constant NAPI_POLL_WAIT
>>>>       r8169: switch to napi_schedule_irqoff
>>>>       r8169: simplify rtl8169_alloc_rx_data
>>>>       r8169: improve rtl8169_init_ring
>>>>       r8169: remove unneeded check in rtl8169_rx_fill
>>>>       r8169: replace rx_buf_sz with a constant
>>>>       r8169: remove rtl8169_map_to_asic
>>>>       r8169: change hw_start argument type
>>>>       r8169: change argument type of counters handling functions
>>>>       r8169: change interrupt handler argument type
>>>>       r8169: drop member opts1_mask from struct rtl8169_private
>>>>       r8169: don't display tp->mmio_addr address
>>>>       r8169: improve rtl8169_get_mac_version
>>>>       r8169: drop member txd_version from struct rtl8169_private
>>>>       r8169: improve pci region handling
>>>>       r8169: remove jumbo_tx_csum from chip config struct
>>>>       r8169: don't use netif_info et al before net_device has been
>>>> registered
>>>>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>>>>       r8169: improve rtl8169_set_features
>>>>       r8169: replace magic number for INTT mask with a constant
>>>>       r8169: improve CPlusCmd handling
>>>>       r8169: improve handling of CPCMD quirk mask
>>>>       r8169: simplify rtl_hw_start_8169
>>>>       r8169: remove calls to rtl_set_rx_mode
>>>>       r8169: move common initializations to tp->hw_start
>>>>       r8169: remove unneeded check in r8168_pll_power_down
>>>>       r8169: remove 810x_phy_power_up/down
>>>>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>>>>       r8169: drop member pll_power_ops from struct rtl8169_private
>>>>       r8169: simplify code by using ranges in switch clauses
>>>>       r8169: replace longer if statements with switch statements
>>>>       r8169: drop rtl_generic_op
>>>>       r8169: improve PCI config space access
>>>>       r8169: avoid potentially misaligned access when getting mac
>>>> address
>>>>       r8169: replace get_protocol with vlan_get_protocol
>>>>       r8169: fix network error on resume from suspend
>>>>
>>>> Kai-Heng Feng (4):
>>>>       Revert "r8169: fix interrupt number after adding support for
>>>> MSI-X interrupts"
>>>>       Revert "r8169: improve interrupt handling"
>>>>       Revert "r8169: disable WOL per default"
>>>>       Revert "r8169: remove some WOL-related dead code"
>>>>
>>>> Ville Syrjälä (1):
>>>>       r8169: Fix netpoll oops
>>>>
>>>>  drivers/net/ethernet/realtek/r8169.c | 2101
>>>> +++++++++++-----------------------
>>>>  include/uapi/linux/pci_regs.h        |    2 +
>>>>  2 files changed, 649 insertions(+), 1454 deletions(-)
>>> Though this is isolated to one driver, it is a driver that is in
>>> wide-spread
>>> use. The risk of regression is to high to justify pulling in that huge
>>> delta.
>>> Even more for PM reasons. This is what HWE kernels are for.
>>
>> Runtime PM is enabled for 4.13 based linux-oem, which will upgrade to
>> Bionic's 4.15 soon.
>> This transition will regress those who uses 4.13 based kernel.
>>
>> If this series can't get pulled in, then we should probably make it
>> upgrade to 4.15 based linux-oem kernel instead of Bionic's kernel.
> 
> There is no 4.15 based linux-oem in xenial..

Not yet, but I thought the currently 4.13 based one would be changed to a 4.15
based one at some point.

-Stefan
> 
> 
>
Timo Aaltonen June 25, 2018, 9:58 a.m. | #6
On 25.06.2018 12:52, Stefan Bader wrote:
> On 25.06.2018 09:52, Timo Aaltonen wrote:
>> On 25.06.2018 10:31, Kai-Heng Feng wrote:
>>> at 15:24, Stefan Bader <stefan.bader@canonical.com> wrote:
>>>
>>>> On 25.06.2018 08:56, 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.
>>>>>
>>>>> Sync r8169 driver with mainline, with one fix from net-next.
>>>>> Everything is cleanly cherry-picked.
>>>>> This means the fix for LP: #1752772 is dropped and re-cherry-picked.
>>>>>
>>>>> [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]
>>>>> Low.
>>>>> The code was in mainline for a while, it should be well-tested by now.
>>>>>
>>>>> The following changes since commit
>>>>> 123dad8e7f35b815fdf6d0647b056c096f14d052:
>>>>>
>>>>>   ixgbe/ixgbevf: Free IRQ when PCI error recovery removes the device
>>>>> (2018-06-18 14:57:01 -0400)
>>>>>
>>>>> are available in the Git repository at:
>>>>>
>>>>>   git://git.launchpad.net/~kaihengfeng/+git/linux sru-bionic-r8169-rpm
>>>>>
>>>>> for you to fetch changes up to e88347ecb4e961abebde8810973c8a62455f4ea6:
>>>>>
>>>>>   r8169: Fix netpoll oops (2018-06-25 12:32:25 +0800)
>>>>>
>>>>> ----------------------------------------------------------------
>>>>> Andy Shevchenko (2):
>>>>>       r8169: Dereference MMIO address immediately before use
>>>>>       r8169: switch to device-managed functions in probe (part 2)
>>>>>
>>>>> Heiner Kallweit (55):
>>>>>       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
>>>>>       r8169: remove some WOL-related dead code
>>>>>       r8169: remove not needed PHY soft reset in
>>>>> rtl8168e_2_hw_phy_config
>>>>>       r8169: disable WOL per default
>>>>>       r8169: simplify and improve check for dash
>>>>>       r8169: improve interrupt handling
>>>>>       r8169: convert remaining feature flag and remove enum features
>>>>>       r8169: fix interrupt number after adding support for MSI-X
>>>>> interrupts
>>>>>       r8169: simplify rtl_set_mac_address
>>>>>       r8169: change type of first argument in rtl_tx_performance_tweak
>>>>>       r8169: change type of argument in rtl_disable/enable_clock_request
>>>>>       r8169: add helper tp_to_dev
>>>>>       PCI: Add two more values for PCIe Max_Read_Request_Size
>>>>>       r8169: replace magic numbers with PCI MRRS constant
>>>>>       r8169: remove unused member features from struct
>>>>>       r8169: remove member align from struct rtl_cfg_info
>>>>>       r8169: use skb_copy_to_linear_data in rtl8169_try_rx_copy
>>>>>       r8169: use constant NAPI_POLL_WAIT
>>>>>       r8169: switch to napi_schedule_irqoff
>>>>>       r8169: simplify rtl8169_alloc_rx_data
>>>>>       r8169: improve rtl8169_init_ring
>>>>>       r8169: remove unneeded check in rtl8169_rx_fill
>>>>>       r8169: replace rx_buf_sz with a constant
>>>>>       r8169: remove rtl8169_map_to_asic
>>>>>       r8169: change hw_start argument type
>>>>>       r8169: change argument type of counters handling functions
>>>>>       r8169: change interrupt handler argument type
>>>>>       r8169: drop member opts1_mask from struct rtl8169_private
>>>>>       r8169: don't display tp->mmio_addr address
>>>>>       r8169: improve rtl8169_get_mac_version
>>>>>       r8169: drop member txd_version from struct rtl8169_private
>>>>>       r8169: improve pci region handling
>>>>>       r8169: remove jumbo_tx_csum from chip config struct
>>>>>       r8169: don't use netif_info et al before net_device has been
>>>>> registered
>>>>>       r8169: remove unneeded call to __rtl8169_set_features in rtl_open
>>>>>       r8169: improve rtl8169_set_features
>>>>>       r8169: replace magic number for INTT mask with a constant
>>>>>       r8169: improve CPlusCmd handling
>>>>>       r8169: improve handling of CPCMD quirk mask
>>>>>       r8169: simplify rtl_hw_start_8169
>>>>>       r8169: remove calls to rtl_set_rx_mode
>>>>>       r8169: move common initializations to tp->hw_start
>>>>>       r8169: remove unneeded check in r8168_pll_power_down
>>>>>       r8169: remove 810x_phy_power_up/down
>>>>>       r8169: merge r810x_pll_power_down/up into r8168_pll_power_down/up
>>>>>       r8169: drop member pll_power_ops from struct rtl8169_private
>>>>>       r8169: simplify code by using ranges in switch clauses
>>>>>       r8169: replace longer if statements with switch statements
>>>>>       r8169: drop rtl_generic_op
>>>>>       r8169: improve PCI config space access
>>>>>       r8169: avoid potentially misaligned access when getting mac
>>>>> address
>>>>>       r8169: replace get_protocol with vlan_get_protocol
>>>>>       r8169: fix network error on resume from suspend
>>>>>
>>>>> Kai-Heng Feng (4):
>>>>>       Revert "r8169: fix interrupt number after adding support for
>>>>> MSI-X interrupts"
>>>>>       Revert "r8169: improve interrupt handling"
>>>>>       Revert "r8169: disable WOL per default"
>>>>>       Revert "r8169: remove some WOL-related dead code"
>>>>>
>>>>> Ville Syrjälä (1):
>>>>>       r8169: Fix netpoll oops
>>>>>
>>>>>  drivers/net/ethernet/realtek/r8169.c | 2101
>>>>> +++++++++++-----------------------
>>>>>  include/uapi/linux/pci_regs.h        |    2 +
>>>>>  2 files changed, 649 insertions(+), 1454 deletions(-)
>>>> Though this is isolated to one driver, it is a driver that is in
>>>> wide-spread
>>>> use. The risk of regression is to high to justify pulling in that huge
>>>> delta.
>>>> Even more for PM reasons. This is what HWE kernels are for.
>>>
>>> Runtime PM is enabled for 4.13 based linux-oem, which will upgrade to
>>> Bionic's 4.15 soon.
>>> This transition will regress those who uses 4.13 based kernel.
>>>
>>> If this series can't get pulled in, then we should probably make it
>>> upgrade to 4.15 based linux-oem kernel instead of Bionic's kernel.
>>
>> There is no 4.15 based linux-oem in xenial..
> 
> Not yet, but I thought the currently 4.13 based one would be changed to a 4.15
> based one at some point.

Nope, because we don't want to support it that long (until 2021).
Instead, linux-oem will upgrade to hwe backport from bionic when the
time is right (after the next cycle or so).