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

From: Andrew Lunn <andrew@lunn.ch>
To: Hau <hau@realtek.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	nic_swsd <nic_swsd@realtek.com>
Subject: Re: [PATCH net] r8169: fix rtl8168h wol fail
Date: Fri, 6 Jan 2023 15:03:33 +0100	[thread overview]
Message-ID: <Y7gqNfpRvOpeFPEN@lunn.ch> (raw)
In-Reply-To: <9ee2f626bab3481697b71c58091e7def@realtek.com>

On Fri, Jan 06, 2023 at 06:53:12AM +0000, 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.

You mean you will read the EEPROM in the SFP to determine what it
supports? If so, please use phylink, and the SFP driver, which will do
this for you.

     Andrew