netdev - Re: [PATCH RFC 13/18] r8168: replace speed_down with genphy_restart

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] [day] [month] [year] [list]

Date:   Sat, 23 Dec 2017 00:36:14 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        Chun-Hao Lin <hau@...ltek.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 13/18] r8168: replace speed_down with
 genphy_restart_aneg

Am 22.12.2017 um 11:14 schrieb Andrew Lunn:
> On Thu, Dec 21, 2017 at 09:50:39PM +0100, Heiner Kallweit wrote:
>> Dealing with link partner abilities is handled by phylib, so let's
>> just trigger autonegotiation here.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>> ---
>>  drivers/net/ethernet/realtek/r8168.c | 26 +-------------------------
>>  1 file changed, 1 insertion(+), 25 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8168.c b/drivers/net/ethernet/realtek/r8168.c
>> index d33f93a31..6b398915f 100644
>> --- a/drivers/net/ethernet/realtek/r8168.c
>> +++ b/drivers/net/ethernet/realtek/r8168.c
>> @@ -4360,30 +4360,6 @@ static void rtl_init_mdio_ops(struct rtl8168_private *tp)
>>  	}
>>  }
>>  
>> -static void rtl_speed_down(struct rtl8168_private *tp)
>> -{
>> -	u32 adv;
>> -	int lpa;
>> -
>> -	rtl_writephy(tp, 0x1f, 0x0000);
>> -	lpa = rtl_readphy(tp, MII_LPA);
>> -
>> -	if (lpa & (LPA_10HALF | LPA_10FULL))
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>> -	else if (lpa & (LPA_100HALF | LPA_100FULL))
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>> -	else
>> -		adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>> -		      ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>> -		      (tp->mii.supports_gmii ?
>> -		       ADVERTISED_1000baseT_Half |
>> -		       ADVERTISED_1000baseT_Full : 0);
>> -
>> -	rtl8168_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>> -			  adv);
>> -}
>> -
>>  static void rtl_wol_suspend_quirk(struct rtl8168_private *tp)
>>  {
>>  	void __iomem *ioaddr = tp->mmio_addr;
>> @@ -4424,7 +4400,7 @@ static bool rtl_wol_pll_power_down(struct rtl8168_private *tp)
>>  	if (!(__rtl8168_get_wol(tp) & WAKE_ANY))
>>  		return false;
>>  
>> -	rtl_speed_down(tp);
>> +	genphy_restart_aneg(tp->dev->phydev);
>>  	rtl_wol_suspend_quirk(tp);
>>  
>>  	return true;
> 
> I'm not too clear what is going on here? Is this suspend while WOL is
> enabled? There should be no need to change the PHY settings. The PHY
> driver should leave the PHY running in whatever state it was
> configured to. The only danger here is that the MAC driver has called
> phy_stop() during suspend. That should not be done when WOL is
> enabled.
> 
> Is the wol being passed to the phylib? phy_ethtool_set_wol() and
> phy_ethtool_get_wol()?
> 
I also have a hard time to understand what this is good for.
Here's the mail thread regarding introduction of this function:
https://lkml.org/lkml/2013/4/2/669

If I understand this correctly it's about fixing an issue when
aneg is disabled and the PHY automatically changes the speed.

In this case we may have to use genphy_config_aneg here which
calls genphy_setup_forced if aneg is disabled.

> 	Andrew
>