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

Message-ID: <20171222101441.GK2431@lunn.ch>
Date:   Fri, 22 Dec 2017 11:14:41 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
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

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()?

	Andrew