Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

[Kai Heng Feng]

> On 15 Nov 2017, at 6:53 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
> 
> From: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> Date: Wed, 15 Nov 2017 04:00:18 -0500
> 
>> Commit ("r8169: enable ALDPS for power saving") caused a regression on
>> RTL8168evl/8111evl [1], so it got reverted.
>> 
>> Instead of reverting the whole commit, let's reinstate ALDPS for all
>> models other than RTL8168evl/8111evl.
>> 
>> [1] https://lkml.org/lkml/2013/1/5/36
>> 
>> Cc: Francois Romieu <romieu@xxxxxxxxxxxxx>
>> Cc: Hayes Wang <hayeswang@xxxxxxxxxxx>
>> Cc: JÃrg Otte <jrg.otte@xxxxxxxxx>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> 
> Sorry, this isn't going to work.
> 
> The problem is that we have no idea what chips this really
> works for.  All we know is that it definitely does not work
> for one particular chip variant.

Another guy tried to enable ASPM before [1], with extra knobs,
which is discouraged.
I want to do the same without the knobs, with the intention not to
re-introduce the regression to RTL8168evl/8111evl.

> The amount of coverage this change is going to get is very small as
> well, meaning an even greater change of regressions.
> 
> Therefore the only acceptable way to handle this is to have
> a white-list, specific chips that have been explicitly tested
> and are known to work with this feature, rather than the other
> way around.

I think a chip-specific whitelist will be quite hard. That requires *both*
Realtek and OEM/ODM to do the test. IIUC, the very same chip can be used
in different Laptops/Motherboards, regression may happen on a very specific 
combination.

A more plausible solution is model specific whitelist. In this case, 
extra knobs are desirable, so users can do the testing themselves before
adding new models to whitelist.

Currently users report that by enabling r8169âs ASPM on Dell Inspiron 7559,
power consumption can be drastically reduced.

> Furthermore, you're not even checking the chip version, you're
> checking instead whether the firmware is loaded or not.  That
> doesn't seem like a safe way to guard this at all.

Hopefully Hayes (or Realtek) can shed more lights on the issue. Apparently
ALDPS and ASPM for r8169 is enabled in different commercial products, just
not in Linux mainline.

Kai-Heng

[1] https://www.spinics.net/lists/netdev/msg403994.html