RE: [PATCH net] r8169: fix rtl8168h wol fail

From: Hau <hau@realtek.com>
To: Heiner Kallweit <hkallweit1@gmail.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	nic_swsd <nic_swsd@realtek.com>, Andrew Lunn <andrew@lunn.ch>
Subject: RE: [PATCH net] r8169: fix rtl8168h wol fail
Date: Tue, 10 Jan 2023 17:03:35 +0000	[thread overview]
Message-ID: <6ff876a66e154bb4b357b31465c86741@realtek.com> (raw)
In-Reply-To: <4014d243-8f8a-f273-fba8-2ae5a3844ea5@gmail.com>

> On 06.01.2023 07:53, Hau 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.
> >>
> >> I agree with Heiner here. I assume you cannot fix the firmware?
> >>
> >> So can we detect the broken firmware and correctly set
> >> phydev->advertising? That will fix WoL and should prevent the user
> >> from using ethtool to select a slower speed.
> >>
> > It is a rtl8211fs's firmware bug. Because in this application it will
> > support both 100M and GIGA fiber module, so it cannot just set
> > phydev->advertising to 100M or GIGA. We  may need to use bit-bang
> MDIO
> > to detect fiber link speed and set phydev->advertising properly. But it will
> let this patch become more complicated.
> >
> I think there's also a userspace workaround for your problem.
> You can use "ethtool -s <if> advertise .." to adjust what the internal PHY
> advertises.
> phy_speed_down() considers only modes that are currently advertised.
> 
> In your case with a 1Gbps fibre module you could set the advertisement to
> 1Gbps/full only.
> Then phy_speed_down() wouldn't change the speed.
> 
In this application(rtl8168h + rtl8211fs) it also supports 100Mbps fiber module.
So userspace workaround is good but it may not always work for this issue.
Not speed down during system suspend may be the simplest workaround for this issue.

------Please consider the environment before printing this e-mail.