netdev - Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

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: <2c2fb98e-28fc-bacb-ff58-d28f23bd1409@gmail.com>
Date:   Tue, 13 Sep 2016 11:11:35 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     John Crispin <john@...ozen.org>, Andrew Lunn <andrew@...n.ch>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, qsdk-review@....qualcomm.com
Subject: Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

On 09/13/2016 11:07 AM, John Crispin wrote:
> 
> 
> On 13/09/2016 19:09, Florian Fainelli wrote:
>> On 09/13/2016 08:59 AM, Andrew Lunn wrote:
>>>> Hi Andrew,
>>>>
>>>> this function does indeed duplicate the functionality of
>>>> phy_ethtool_get_eee() with the small difference, that e->eee_active is
>>>> also set which phy_ethtool_get_eee() does not set.
>>>>
>>>> dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
>>>> get_eee() op has been called. would it be ok to move the code setting
>>>> eee_active to  phy_ethtool_get_eee().
>>
>> Humm, AFAIR, the reason why eee_active is set outside of
>> phy_ethtool_set_eee() is because this is a MAC + PHY thing, both need to
>> agree and support that, and so while the PHY may be configured to have
>> EEE advertised and enabled, you also need to take care of the MAC
>> portion and enable EEE in there as well. Is not there such a thing for
>> the qca8k switch where the PHY needs to be configured through the
>> standard phylib calls, but the switch's transmitter/receiver also needs
>> to have EEE enabled?
>>
> 
> Hi Florian,
> 
> the switch needs to enable the eee on a per mac absis, but there is no
> way to tell if the autonegotiate worked and eee is enabled without
> reading the phys registers.

OK, that does not sound atypical here, most drivers I see do have a way
to tell if EEE is active by reading e.g: the LPI indication register, or
something that is able to reflect the negotiated result.

> 
> setting the eee_active inside phy_ethtool_get_eee() would break those
> dsa drivers that have a register telling if AN worked. if it is ok i
> will just call phy_ethtool_get_eee() inside get_eee().

Ok, let's see the code and then we can discuss from there, not very
clear on the proposed change here.
-- 
Florian