netdev - Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

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:   Tue, 1 Aug 2017 18:06:28 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...oirfairelinux.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel@...oirfairelinux.com,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next 10/11] net: dsa: mv88e6xxx: remove EEE support

On Tue, Aug 01, 2017 at 11:36:13AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@...n.ch> writes:
> 
> > On Mon, Jul 31, 2017 at 06:17:18PM -0400, Vivien Didelot wrote:
> >> The PHY's EEE settings are already accessed by the DSA layer through the
> >> Marvell PHY driver and there is nothing to be done for switch's MACs.
> >
> > I'm confused, or missing something. Does not patch #1 mean that if the
> > DSA driver does not have a set_eee function, we always return -ENODEV
> > in slave.c?
> 
> If there is a PHY, phy_init_eee (if eee_enabled is true) and
> phy_ethtool_set_eee is called. If there is a .set_eee op, it is
> called. If both are absent, -ENODEV is returned.

O.K, i don't think that is correct. EEE should only be enabled if both
the MAC and the PHY supports it. We need some way for the MAC to
indicate it does not support EEE.

If set_eee is optional the meaning of a NULL pointer is that the MAC
does support EEE. So for mv88e6060, lan9303, microchip and mt7530
which currently don't support EEE, you need to add a set_eee which
returns -ENODEV.

Having to implement the op to say you don't implement the feature just
seems wrong.

      Andrew