netdev - Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

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]

Message-ID: <eb4a3c77-6bf4-8749-85a3-b614bff69cf2@gmail.com>
Date:   Wed, 4 Jan 2017 06:33:03 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Giuseppe CAVALLARO <peppe.cavallaro@...com>,
        Andrew Lunn <andrew@...n.ch>,
        Yegor Yefremov <yegorslists@...glemail.com>
Cc:     netdev <netdev@...r.kernel.org>,
        "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        Grygorii Strashko <grygorii.strashko@...com>,
        "N, Mugunthan V" <mugunthanvnm@...com>,
        Rami Rosen <roszenrami@...il.com>,
        Fabrice GASNIER <fabrice.gasnier@...com>
Subject: Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE
 registers

On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>> Peppe, any thoughts on this?
>>
>> I share what you say.
>>
>> In sum, the EEE management inside the stmmac is:
>>
>> - the driver looks at own HW cap register if EEE is supported
>>
>>     (indeed the user could keep disable EEE if bugged on some HW
>>      + Alex, Fabrice: we had some patches for this to propose where we
>>              called the phy_ethtool_set_eee to disable feature at phy
>>              level
>>
>> - then the stmmac asks PHY layer to understand if transceiver and
>>   partners are EEE capable.
>>
>> - If all matches the EEE is actually initialized.
>>
>> the logic above should be respected when use ethtool, hmm, I will
>> check the stmmac_ethtool_op_set_eee asap.
>>
>> Hoping this is useful
> 
> This is definitively useful, the only part that I am struggling to
> understand in phy_init_eee() is this:
> 
>                 eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>                                                 MDIO_MMD_AN);
>                 if (eee_adv <= 0)
>                         goto eee_exit_err;
> 
> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
> the time we call phy_init_eee(), then we cannot complete the EEE
> configuration at the PHY level, and presumably we should abort the EEE
> configuration at the MAC level.
> 
> While this condition makes sense if e.g: you are re-negotiating the link
> with your partner for instance and if EEE was already advertised, the
> very first time this function is called, it seems to be like we should
> skip the check, because phy_init_eee() should actually tell us if, as a
> result of a successful check, we should be setting EEE as something we
> advertise?
> 
> Do you remember what was the logic behind this check when you added it?

Peppe, can you remember why phy_init_eee() was written in a way that you
need to have already locally advertised EEE for the function to
successfully return? Thank you!
-- 
Florian