netdev - Re: [PATCH net] r8169: fix rtl8168h wol fail

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 Jan 2023 20:37:07 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Chunhao Lin <hau@...ltek.com>
Cc:     netdev@...r.kernel.org, nic_swsd@...ltek.com
Subject: Re: [PATCH net] r8169: fix rtl8168h wol fail

On 05.01.2023 19:04, Chunhao Lin wrote:
> rtl8168h has an application that it will connect to rtl8211fs through mdi
> interface. And rtl8211fs will connect to fiber through serdes interface.
> In this application, rtl8168h revision id will be set to 0x2a.
> 
> Because rtl8211fs's firmware will set link capability to 100M and GIGA
> when link is from off to on. So when system suspend and wol is enabled,
> rtl8168h will speed down to 100M (because rtl8211fs advertise 100M and GIGA
> to rtl8168h). If the link speed between rtl81211fs and fiber is GIGA.
> The link speed between rtl8168h and fiber will mismatch. That will cause
> wol fail.
> 
> In this patch, if rtl8168h is in this kind of application, driver will not
> speed down phy when wol is enabled.
> 
I think the patch title is inappropriate because WoL works normally on
RTL8168h in the standard setup.
What you add isn't a fix but a workaround for a firmware bug in RTL8211FS.
As mentioned in a previous review comment: if speed on fibre side is 1Gbps
then RTL8211FS shouldn't advertise 100Mbps on MDI/UTP side.

Last but not least the user can still use e.g. ethtool to change the speed
to 100Mbps thus breaking the link.

> Signed-off-by: Chunhao Lin <hau@...ltek.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 24592d972523..83d017369ae7 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -1199,6 +1199,12 @@ static enum rtl_dash_type rtl_check_dash(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static bool rtl_mdi_connect_to_phy(struct rtl8169_private *tp)

A comment would be helpful so that a reader of the code knows
what it's good for. A brief description of the non-standard
setup with the internal PHY connected to another PHY in media
converter mode would be good.

> +{
> +	return tp->mac_version == RTL_GIGA_MAC_VER_46 &&
> +		tp->pci_dev->revision == 0x2a;
> +}
> +
>  static void rtl_set_d3_pll_down(struct rtl8169_private *tp, bool enable)
>  {
>  	switch (tp->mac_version) {
> @@ -2453,7 +2459,8 @@ static void rtl_prepare_power_down(struct rtl8169_private *tp)
>  		rtl_ephy_write(tp, 0x19, 0xff64);
>  
>  	if (device_may_wakeup(tp_to_dev(tp))) {
> -		phy_speed_down(tp->phydev, false);
> +		if (!rtl_mdi_connect_to_phy(tp))
> +			phy_speed_down(tp->phydev, false);
>  		rtl_wol_enable_rx(tp);
>  	}
>  }