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

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]

Date:   Wed, 15 Nov 2017 19:53:06 +0900 (KST)
From:   David Miller <davem@...emloft.net>
To:     kai.heng.feng@...onical.com
Cc:     nic_swsd@...ltek.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, romieu@...zoreil.com,
        hayeswang@...ltek.com, jrg.otte@...il.com
Subject: Re: [PATCH 1/2] r8169: reinstate ALDPS for power saving

From: Kai-Heng Feng <kai.heng.feng@...onical.com>
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@...zoreil.com>
> Cc: Hayes Wang <hayeswang@...ltek.com>
> Cc: Jörg Otte <jrg.otte@...il.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>

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.

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.

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.