[PATCH net] r8169: fix rtl8168h wol fail

[PATCH net] r8169: fix rtl8168h wol fail

[Chunhao Lin]

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.

Signed-off-by: Chunhao Lin <hau@realtek.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)
+{
+	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);
 	}
 }
-- 
2.39.0

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

[Heiner Kallweit]

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@realtek.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);
>  	}
>  }

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

[Andrew Lunn]

On Thu, Jan 05, 2023 at 08:37:07PM +0100, Heiner Kallweit wrote:
> 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.

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.

     Andrew

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

[Hau]

> > > 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.

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

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

[Andrew Lunn]

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

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

[Heiner Kallweit]

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.

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

[Hau]

> 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.

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

[Heiner Kallweit]

On 10.01.2023 18:03, Hau wrote:
>> 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.

Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in case of a 100Mbps fiber module?

> So userspace workaround is good but it may not always work for this issue.

When would it not work? If you know the fiber module speed you can set the advertisement accordingly.

> Not speed down during system suspend may be the simplest workaround for this issue.
> 

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

[Hau]

> 
> On 10.01.2023 18:03, Hau wrote:
> >> 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.
> 
> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in case
> of a 100Mbps fiber module?
Yes.

> > So userspace workaround is good but it may not always work for this issue.
> 
> When would it not work? If you know the fiber module speed you can set
> the advertisement accordingly.
Sure, user can set internal PHY advertisement according to fiber module speed.
But we would like to have a solution that does not need user to do anything.
So this userspace workaround may not meet our need.

> > Not speed down during system suspend may be the simplest workaround
> for this issue.
> >
------Please consider the environment before printing this e-mail.

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

[Heiner Kallweit]

On 11.01.2023 18:23, Hau wrote:
>>
>> On 10.01.2023 18:03, Hau wrote:
>>>> 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.
>>
>> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in case
>> of a 100Mbps fiber module?
> Yes.
> 
I think in this case internal PHY and RTL8211FS would negotiate 1Gbps,
not matching the speed of the 100Mbps fiber module.
How does this work?

>>> So userspace workaround is good but it may not always work for this issue.
>>
>> When would it not work? If you know the fiber module speed you can set
>> the advertisement accordingly.
> Sure, user can set internal PHY advertisement according to fiber module speed.
> But we would like to have a solution that does not need user to do anything.
> So this userspace workaround may not meet our need.
> 
>>> Not speed down during system suspend may be the simplest workaround
>> for this issue.
>>>

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

[Andrew Lunn]

> >>> In this application(rtl8168h + rtl8211fs) it also supports 100Mbps fiber
> >> module.
> >>
> >> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in case
> >> of a 100Mbps fiber module?
> > Yes.
> > 
> I think in this case internal PHY and RTL8211FS would negotiate 1Gbps,
> not matching the speed of the 100Mbps fiber module.
> How does this work?

Fibre line side has no autoneg. Both ends need to be using the same
speed, or the SERDES does not synchronise and does not establish link.

You can ask the SFP module what baud rate it supports, and then use
anything up to that baud rate. I've got systems where the SFP is fast
enough to support a 2.5Gbps link, so the MAC indicates both 2.5G and
1G, defaults to 2.5G, and fails to connect to a 1G link peer. You need
to use ethtool to force it to the lower speed before the link works.

But from what i understand, you cannot use a 1000Base-X SFP, set the
MAC to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for
me, the RTL8211FS should not be advertise 100Mbps and 1Gbps, it needs
to talk to the SFP figure out exactly what it is, and only advertise
the one mode which is supported.

    Andrew

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

[Hau]

> > >>> In this application(rtl8168h + rtl8211fs) it also supports 100Mbps
> > >>> fiber
> > >> module.
> > >>
> > >> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in
> > >> case of a 100Mbps fiber module?
> > > Yes.
> > >
> > I think in this case internal PHY and RTL8211FS would negotiate 1Gbps,
> > not matching the speed of the 100Mbps fiber module.
> > How does this work?

My mistake. With 100Mbps fiber module RTL8211FS will only advertise 100Mbps
on the UTP/MDI side. With 1Gbps fiber module it will advertise both 100Mbps and
1Gbps. So issue will only happen with 1Gbps fiber module.

> Fibre line side has no autoneg. Both ends need to be using the same speed,
> or the SERDES does not synchronise and does not establish link.
> 
> You can ask the SFP module what baud rate it supports, and then use
> anything up to that baud rate. I've got systems where the SFP is fast enough
> to support a 2.5Gbps link, so the MAC indicates both 2.5G and 1G, defaults to
> 2.5G, and fails to connect to a 1G link peer. You need to use ethtool to force
> it to the lower speed before the link works.
> 
> But from what i understand, you cannot use a 1000Base-X SFP, set the MAC
> to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for me, the
> RTL8211FS should not be advertise 100Mbps and 1Gbps, it needs to talk to
> the SFP figure out exactly what it is, and only advertise the one mode which
> is supported.

It is the RTL8211FS firmware bug. This patch is for workaround this issue.

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

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

[Andrew Lunn]

On Fri, Jan 13, 2023 at 04:23:45PM +0000, Hau wrote:
> > > >>> In this application(rtl8168h + rtl8211fs) it also supports 100Mbps
> > > >>> fiber
> > > >> module.
> > > >>
> > > >> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in
> > > >> case of a 100Mbps fiber module?
> > > > Yes.
> > > >
> > > I think in this case internal PHY and RTL8211FS would negotiate 1Gbps,
> > > not matching the speed of the 100Mbps fiber module.
> > > How does this work?
> 
> My mistake. With 100Mbps fiber module RTL8211FS will only advertise 100Mbps
> on the UTP/MDI side. With 1Gbps fiber module it will advertise both 100Mbps and
> 1Gbps. So issue will only happen with 1Gbps fiber module.
> 
> > Fibre line side has no autoneg. Both ends need to be using the same speed,
> > or the SERDES does not synchronise and does not establish link.
> > 
> > You can ask the SFP module what baud rate it supports, and then use
> > anything up to that baud rate. I've got systems where the SFP is fast enough
> > to support a 2.5Gbps link, so the MAC indicates both 2.5G and 1G, defaults to
> > 2.5G, and fails to connect to a 1G link peer. You need to use ethtool to force
> > it to the lower speed before the link works.
> > 
> > But from what i understand, you cannot use a 1000Base-X SFP, set the MAC
> > to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for me, the
> > RTL8211FS should not be advertise 100Mbps and 1Gbps, it needs to talk to
> > the SFP figure out exactly what it is, and only advertise the one mode which
> > is supported.
> 
> It is the RTL8211FS firmware bug. This patch is for workaround this issue.

So if it is advertising both 100Mbps and 1Gbps, we know the SFP is
actually 1G, and we can remove the 100Mbps advertisement? That should
then solve all the problems?

     Andrew

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

[Heiner Kallweit]

On 13.01.2023 17:36, Andrew Lunn wrote:
> On Fri, Jan 13, 2023 at 04:23:45PM +0000, Hau wrote:
>>>>>>> In this application(rtl8168h + rtl8211fs) it also supports 100Mbps
>>>>>>> fiber
>>>>>> module.
>>>>>>
>>>>>> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side in
>>>>>> case of a 100Mbps fiber module?
>>>>> Yes.
>>>>>
>>>> I think in this case internal PHY and RTL8211FS would negotiate 1Gbps,
>>>> not matching the speed of the 100Mbps fiber module.
>>>> How does this work?
>>
>> My mistake. With 100Mbps fiber module RTL8211FS will only advertise 100Mbps
>> on the UTP/MDI side. With 1Gbps fiber module it will advertise both 100Mbps and
>> 1Gbps. So issue will only happen with 1Gbps fiber module.
>>
>>> Fibre line side has no autoneg. Both ends need to be using the same speed,
>>> or the SERDES does not synchronise and does not establish link.
>>>
>>> You can ask the SFP module what baud rate it supports, and then use
>>> anything up to that baud rate. I've got systems where the SFP is fast enough
>>> to support a 2.5Gbps link, so the MAC indicates both 2.5G and 1G, defaults to
>>> 2.5G, and fails to connect to a 1G link peer. You need to use ethtool to force
>>> it to the lower speed before the link works.
>>>
>>> But from what i understand, you cannot use a 1000Base-X SFP, set the MAC
>>> to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for me, the
>>> RTL8211FS should not be advertise 100Mbps and 1Gbps, it needs to talk to
>>> the SFP figure out exactly what it is, and only advertise the one mode which
>>> is supported.
>>
>> It is the RTL8211FS firmware bug. This patch is for workaround this issue.
> 
> So if it is advertising both 100Mbps and 1Gbps, we know the SFP is
> actually 1G, and we can remove the 100Mbps advertisement? That should
> then solve all the problems?
> 
Right, that's what I proposed too, removing 1Gbps advertisement of the
RTL8168H-internal PHY via userspace tool, e.g. ethtool. For me this is
the cleanest solution. Adding a workaround for a firmware bug of a
specific external PHY to the r8169 MAC driver would be somewhat hacky.




>      Andrew

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

[Hau]

> On 13.01.2023 17:36, Andrew Lunn wrote:
> > On Fri, Jan 13, 2023 at 04:23:45PM +0000, Hau wrote:
> >>>>>>> In this application(rtl8168h + rtl8211fs) it also supports
> >>>>>>> 100Mbps fiber
> >>>>>> module.
> >>>>>>
> >>>>>> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side
> in
> >>>>>> case of a 100Mbps fiber module?
> >>>>> Yes.
> >>>>>
> >>>> I think in this case internal PHY and RTL8211FS would negotiate
> >>>> 1Gbps, not matching the speed of the 100Mbps fiber module.
> >>>> How does this work?
> >>
> >> My mistake. With 100Mbps fiber module RTL8211FS will only advertise
> >> 100Mbps on the UTP/MDI side. With 1Gbps fiber module it will
> >> advertise both 100Mbps and 1Gbps. So issue will only happen with 1Gbps
> fiber module.
> >>
> >>> Fibre line side has no autoneg. Both ends need to be using the same
> >>> speed, or the SERDES does not synchronise and does not establish link.
> >>>
> >>> You can ask the SFP module what baud rate it supports, and then use
> >>> anything up to that baud rate. I've got systems where the SFP is
> >>> fast enough to support a 2.5Gbps link, so the MAC indicates both
> >>> 2.5G and 1G, defaults to 2.5G, and fails to connect to a 1G link
> >>> peer. You need to use ethtool to force it to the lower speed before the
> link works.
> >>>
> >>> But from what i understand, you cannot use a 1000Base-X SFP, set the
> >>> MAC to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for
> >>> me, the RTL8211FS should not be advertise 100Mbps and 1Gbps, it
> >>> needs to talk to the SFP figure out exactly what it is, and only
> >>> advertise the one mode which is supported.
> >>
> >> It is the RTL8211FS firmware bug. This patch is for workaround this issue.
> >
> > So if it is advertising both 100Mbps and 1Gbps, we know the SFP is
> > actually 1G, and we can remove the 100Mbps advertisement? That should
> > then solve all the problems?
> >
> Right, that's what I proposed too, removing 1Gbps advertisement of the
> RTL8168H-internal PHY via userspace tool, e.g. ethtool. For me this is the
> cleanest solution. Adding a workaround for a firmware bug of a specific
> external PHY to the r8169 MAC driver would be somewhat hacky.
> 
Thanks for your suggestions. But because it needs user to execute userspace tool.
This workaround may not be accepted by our customer

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

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

[Heiner Kallweit]

On 16.01.2023 18:04, Hau wrote:
>> On 13.01.2023 17:36, Andrew Lunn wrote:
>>> On Fri, Jan 13, 2023 at 04:23:45PM +0000, Hau wrote:
>>>>>>>>> In this application(rtl8168h + rtl8211fs) it also supports
>>>>>>>>> 100Mbps fiber
>>>>>>>> module.
>>>>>>>>
>>>>>>>> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI side
>> in
>>>>>>>> case of a 100Mbps fiber module?
>>>>>>> Yes.
>>>>>>>
>>>>>> I think in this case internal PHY and RTL8211FS would negotiate
>>>>>> 1Gbps, not matching the speed of the 100Mbps fiber module.
>>>>>> How does this work?
>>>>
>>>> My mistake. With 100Mbps fiber module RTL8211FS will only advertise
>>>> 100Mbps on the UTP/MDI side. With 1Gbps fiber module it will
>>>> advertise both 100Mbps and 1Gbps. So issue will only happen with 1Gbps
>> fiber module.
>>>>
>>>>> Fibre line side has no autoneg. Both ends need to be using the same
>>>>> speed, or the SERDES does not synchronise and does not establish link.
>>>>>
>>>>> You can ask the SFP module what baud rate it supports, and then use
>>>>> anything up to that baud rate. I've got systems where the SFP is
>>>>> fast enough to support a 2.5Gbps link, so the MAC indicates both
>>>>> 2.5G and 1G, defaults to 2.5G, and fails to connect to a 1G link
>>>>> peer. You need to use ethtool to force it to the lower speed before the
>> link works.
>>>>>
>>>>> But from what i understand, you cannot use a 1000Base-X SFP, set the
>>>>> MAC to 100Mbps, and expect it to connect to a 100Base-FX SFP. So for
>>>>> me, the RTL8211FS should not be advertise 100Mbps and 1Gbps, it
>>>>> needs to talk to the SFP figure out exactly what it is, and only
>>>>> advertise the one mode which is supported.
>>>>
>>>> It is the RTL8211FS firmware bug. This patch is for workaround this issue.
>>>
>>> So if it is advertising both 100Mbps and 1Gbps, we know the SFP is
>>> actually 1G, and we can remove the 100Mbps advertisement? That should
>>> then solve all the problems?
>>>
>> Right, that's what I proposed too, removing 1Gbps advertisement of the
>> RTL8168H-internal PHY via userspace tool, e.g. ethtool. For me this is the
>> cleanest solution. Adding a workaround for a firmware bug of a specific
>> external PHY to the r8169 MAC driver would be somewhat hacky.
>>
> Thanks for your suggestions. But because it needs user to execute userspace tool.
> This workaround may not be accepted by our customer
> 

In this case you can provide your customer with a downstream kernel including
your patch.

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

[Hau]

> On 16.01.2023 18:04, Hau wrote:
> >> On 13.01.2023 17:36, Andrew Lunn wrote:
> >>> On Fri, Jan 13, 2023 at 04:23:45PM +0000, Hau wrote:
> >>>>>>>>> In this application(rtl8168h + rtl8211fs) it also supports
> >>>>>>>>> 100Mbps fiber
> >>>>>>>> module.
> >>>>>>>>
> >>>>>>>> Does RTL8211FS advertise 100Mbps and 1Gbps on the UTP/MDI
> side
> >> in
> >>>>>>>> case of a 100Mbps fiber module?
> >>>>>>> Yes.
> >>>>>>>
> >>>>>> I think in this case internal PHY and RTL8211FS would negotiate
> >>>>>> 1Gbps, not matching the speed of the 100Mbps fiber module.
> >>>>>> How does this work?
> >>>>
> >>>> My mistake. With 100Mbps fiber module RTL8211FS will only advertise
> >>>> 100Mbps on the UTP/MDI side. With 1Gbps fiber module it will
> >>>> advertise both 100Mbps and 1Gbps. So issue will only happen with
> >>>> 1Gbps
> >> fiber module.
> >>>>
> >>>>> Fibre line side has no autoneg. Both ends need to be using the
> >>>>> same speed, or the SERDES does not synchronise and does not
> establish link.
> >>>>>
> >>>>> You can ask the SFP module what baud rate it supports, and then
> >>>>> use anything up to that baud rate. I've got systems where the SFP
> >>>>> is fast enough to support a 2.5Gbps link, so the MAC indicates
> >>>>> both 2.5G and 1G, defaults to 2.5G, and fails to connect to a 1G
> >>>>> link peer. You need to use ethtool to force it to the lower speed
> >>>>> before the
> >> link works.
> >>>>>
> >>>>> But from what i understand, you cannot use a 1000Base-X SFP, set
> >>>>> the MAC to 100Mbps, and expect it to connect to a 100Base-FX SFP.
> >>>>> So for me, the RTL8211FS should not be advertise 100Mbps and
> >>>>> 1Gbps, it needs to talk to the SFP figure out exactly what it is,
> >>>>> and only advertise the one mode which is supported.
> >>>>
> >>>> It is the RTL8211FS firmware bug. This patch is for workaround this issue.
> >>>
> >>> So if it is advertising both 100Mbps and 1Gbps, we know the SFP is
> >>> actually 1G, and we can remove the 100Mbps advertisement? That
> >>> should then solve all the problems?
> >>>
> >> Right, that's what I proposed too, removing 1Gbps advertisement of
> >> the RTL8168H-internal PHY via userspace tool, e.g. ethtool. For me
> >> this is the cleanest solution. Adding a workaround for a firmware bug
> >> of a specific external PHY to the r8169 MAC driver would be somewhat
> hacky.
> >>
> > Thanks for your suggestions. But because it needs user to execute
> userspace tool.
> > This workaround may not be accepted by our customer
> >
> 
> In this case you can provide your customer with a downstream kernel
> including your patch.
> 
Thanks. We will include it as one of the options.

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