| Message ID | 27c336a8-ea47-483d-815b-02c45ae41da2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f5d59230ec26aa5e8b59e9f4a4d288703a737479 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RESUBMIT,net-next] r8169: simplify EEE handling | expand |
> @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp) > } > > tp->phydev->mac_managed_pm = true; > - > + if (rtl_supports_eee(tp)) > + linkmode_copy(tp->phydev->advertising_eee, > + tp->phydev->supported_eee); This looks odd. Does it mean something is missing on phylib? Andrew
On 02.02.2024 01:16, Andrew Lunn wrote: >> @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp) >> } >> >> tp->phydev->mac_managed_pm = true; >> - >> + if (rtl_supports_eee(tp)) >> + linkmode_copy(tp->phydev->advertising_eee, >> + tp->phydev->supported_eee); > > This looks odd. Does it mean something is missing on phylib? > Reason is that we treat "normal" advertising and EEE advertising differently in phylib. See this code snippet from phy_probe(). phy_advertise_supported(phydev); /* Get PHY default EEE advertising modes and handle them as potentially * safe initial configuration. */ err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee); For EEE we don't change the initial advertising to what's supported, but preserve the EEE advertising at the time of phy probing. So if I want to mimic the behavior of phy_advertise_supported() for EEE, I have to populate advertising_eee in the driver. Alternative would be to change phy_advertise_supported(), but this may impact systems with PHY's with EEE flaws. > Andrew Heiner
On Fri, Feb 02, 2024 at 07:55:38AM +0100, Heiner Kallweit wrote: > On 02.02.2024 01:16, Andrew Lunn wrote: > >> @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp) > >> } > >> > >> tp->phydev->mac_managed_pm = true; > >> - > >> + if (rtl_supports_eee(tp)) > >> + linkmode_copy(tp->phydev->advertising_eee, > >> + tp->phydev->supported_eee); > > > > This looks odd. Does it mean something is missing on phylib? > > > Reason is that we treat "normal" advertising and EEE advertising differently > in phylib. See this code snippet from phy_probe(). > > phy_advertise_supported(phydev); > /* Get PHY default EEE advertising modes and handle them as potentially > * safe initial configuration. > */ > err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee); > > For EEE we don't change the initial advertising to what's supported, > but preserve the EEE advertising at the time of phy probing. > So if I want to mimic the behavior of phy_advertise_supported() for EEE, > I have to populate advertising_eee in the driver. So the device you are using is advertising less than what it supports? > Alternative would be to change phy_advertise_supported(), but this may > impact systems with PHY's with EEE flaws. If i remember correctly, there was some worry enabling EEE by default could upset some low latency use cases, PTP accuracy etc. So lets leave it as it is. Maybe a helper would be useful phy_advertise_eee_all() with a comment about why it could be used. Andrew
On 02.02.2024 14:12, Andrew Lunn wrote: > On Fri, Feb 02, 2024 at 07:55:38AM +0100, Heiner Kallweit wrote: >> On 02.02.2024 01:16, Andrew Lunn wrote: >>>> @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp) >>>> } >>>> >>>> tp->phydev->mac_managed_pm = true; >>>> - >>>> + if (rtl_supports_eee(tp)) >>>> + linkmode_copy(tp->phydev->advertising_eee, >>>> + tp->phydev->supported_eee); >>> >>> This looks odd. Does it mean something is missing on phylib? >>> >> Reason is that we treat "normal" advertising and EEE advertising differently >> in phylib. See this code snippet from phy_probe(). >> >> phy_advertise_supported(phydev); >> /* Get PHY default EEE advertising modes and handle them as potentially >> * safe initial configuration. >> */ >> err = genphy_c45_read_eee_adv(phydev, phydev->advertising_eee); >> >> For EEE we don't change the initial advertising to what's supported, >> but preserve the EEE advertising at the time of phy probing. >> So if I want to mimic the behavior of phy_advertise_supported() for EEE, >> I have to populate advertising_eee in the driver. > > So the device you are using is advertising less than what it supports? > It may. If on a board with this chip version the BIOS for whatever reason disabled EEE advertising, then users may complain that their system consumes more power with r8169 than with r8125. Typical users don't necessarily know that EEE exists and what it is, and how to enable it with ethtool. Therefore I'd like to ensure that the supported EEE modes are also advertised. >> Alternative would be to change phy_advertise_supported(), but this may >> impact systems with PHY's with EEE flaws. > > If i remember correctly, there was some worry enabling EEE by default > could upset some low latency use cases, PTP accuracy etc. So lets > leave it as it is. Maybe a helper would be useful > phy_advertise_eee_all() with a comment about why it could be used. > Yes, I think that's the way to go. To minimize efforts I'd like to keep this patch here as it is, then I'll add the helper and change this place in r8169 to use the new helper. > Andrew Heiner
On Fri, 2 Feb 2024 17:06:10 +0100 Heiner Kallweit wrote: > >> Alternative would be to change phy_advertise_supported(), but this may > >> impact systems with PHY's with EEE flaws. > > > > If i remember correctly, there was some worry enabling EEE by default > > could upset some low latency use cases, PTP accuracy etc. So lets > > leave it as it is. Maybe a helper would be useful > > phy_advertise_eee_all() with a comment about why it could be used. > > > Yes, I think that's the way to go. > To minimize efforts I'd like to keep this patch here as it is, then I'll > add the helper and change this place in r8169 to use the new helper. Sorry for being slow - on top or as v2? :)
On 02.02.2024 17:15, Jakub Kicinski wrote: > On Fri, 2 Feb 2024 17:06:10 +0100 Heiner Kallweit wrote: >>>> Alternative would be to change phy_advertise_supported(), but this may >>>> impact systems with PHY's with EEE flaws. >>> >>> If i remember correctly, there was some worry enabling EEE by default >>> could upset some low latency use cases, PTP accuracy etc. So lets >>> leave it as it is. Maybe a helper would be useful >>> phy_advertise_eee_all() with a comment about why it could be used. >>> >> Yes, I think that's the way to go. >> To minimize efforts I'd like to keep this patch here as it is, then I'll >> add the helper and change this place in r8169 to use the new helper. > > Sorry for being slow - on top or as v2? :) I'll do it on top.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 31 Jan 2024 21:31:01 +0100 you wrote: > We don't have to store the EEE modes to be advertised in the driver, > phylib does this for us and stores it in phydev->advertising_eee. > phylib also takes care of properly handling the EEE advertisement. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/ethernet/realtek/r8169_main.c | 32 +++-------------------- > 1 file changed, 4 insertions(+), 28 deletions(-) Here is the summary with links: - [RESUBMIT,net-next] r8169: simplify EEE handling https://git.kernel.org/netdev/net-next/c/f5d59230ec26 You are awesome, thank you!
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 3d30d4499..e0abdbcfa 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -629,7 +629,6 @@ struct rtl8169_private { struct rtl8169_counters *counters; struct rtl8169_tc_offsets tc_offset; u32 saved_wolopts; - int eee_adv; const char *fw_name; struct rtl_fw *rtl_fw; @@ -1987,17 +1986,11 @@ static int rtl8169_get_eee(struct net_device *dev, struct ethtool_keee *data) static int rtl8169_set_eee(struct net_device *dev, struct ethtool_keee *data) { struct rtl8169_private *tp = netdev_priv(dev); - int ret; if (!rtl_supports_eee(tp)) return -EOPNOTSUPP; - ret = phy_ethtool_set_eee(tp->phydev, data); - - if (!ret) - tp->eee_adv = phy_read_mmd(dev->phydev, MDIO_MMD_AN, - MDIO_AN_EEE_ADV); - return ret; + return phy_ethtool_set_eee(tp->phydev, data); } static void rtl8169_get_ringparam(struct net_device *dev, @@ -2062,21 +2055,6 @@ static const struct ethtool_ops rtl8169_ethtool_ops = { .set_pauseparam = rtl8169_set_pauseparam, }; -static void rtl_enable_eee(struct rtl8169_private *tp) -{ - struct phy_device *phydev = tp->phydev; - int adv; - - /* respect EEE advertisement the user may have set */ - if (tp->eee_adv >= 0) - adv = tp->eee_adv; - else - adv = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE); - - if (adv >= 0) - phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, adv); -} - static enum mac_version rtl8169_get_mac_version(u16 xid, bool gmii) { /* @@ -2313,9 +2291,6 @@ static void rtl8169_init_phy(struct rtl8169_private *tp) /* We may have called phy_speed_down before */ phy_speed_up(tp->phydev); - if (rtl_supports_eee(tp)) - rtl_enable_eee(tp); - genphy_soft_reset(tp->phydev); } @@ -5058,7 +5033,9 @@ static int r8169_mdio_register(struct rtl8169_private *tp) } tp->phydev->mac_managed_pm = true; - + if (rtl_supports_eee(tp)) + linkmode_copy(tp->phydev->advertising_eee, + tp->phydev->supported_eee); phy_support_asym_pause(tp->phydev); /* PHY will be woken up in rtl_open() */ @@ -5193,7 +5170,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) tp->dev = dev; tp->pci_dev = pdev; tp->supports_gmii = ent->driver_data == RTL_CFG_NO_GBIT ? 0 : 1; - tp->eee_adv = -1; tp->ocp_base = OCP_STD_PHY_BASE; raw_spin_lock_init(&tp->cfg9346_usage_lock);
We don't have to store the EEE modes to be advertised in the driver, phylib does this for us and stores it in phydev->advertising_eee. phylib also takes care of properly handling the EEE advertisement. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 32 +++-------------------- 1 file changed, 4 insertions(+), 28 deletions(-)