| Message ID | 5964fa47-2eff-4968-894c-0b7f487d820c@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: improve phylib EEE handling | expand |
On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote: > Link modes in phydev->eee_disabled_modes are filtered out by > genphy_c45_write_eee_adv() and won't be advertised. Therefore > don't accept such modes from userspace. Why do we need this? Surely if the MAC doesn't support modes, then they should be filtered out of phydev->supported_eee so that userspace knows that the mode is not supported by the network interface as a whole, just like we do for phydev->supported. That would give us the checking here.
On 11.01.2025 10:21, Russell King (Oracle) wrote: > On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote: >> Link modes in phydev->eee_disabled_modes are filtered out by >> genphy_c45_write_eee_adv() and won't be advertised. Therefore >> don't accept such modes from userspace. > > Why do we need this? Surely if the MAC doesn't support modes, then they > should be filtered out of phydev->supported_eee so that userspace knows > that the mode is not supported by the network interface as a whole, just > like we do for phydev->supported. > > That would give us the checking here. > Removing EEE modes to be disabled from supported_eee is problematic because of how genphy_c45_write_eee_adv() works. Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we remove 2.5Gbps from supported_eee, then the following check is false: if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES)) What would result in the 2.5Gbps mode not getting disabled.
On Sat, Jan 11, 2025 at 10:44:25AM +0100, Heiner Kallweit wrote: > On 11.01.2025 10:21, Russell King (Oracle) wrote: > > On Sat, Jan 11, 2025 at 10:06:02AM +0100, Heiner Kallweit wrote: > >> Link modes in phydev->eee_disabled_modes are filtered out by > >> genphy_c45_write_eee_adv() and won't be advertised. Therefore > >> don't accept such modes from userspace. > > > > Why do we need this? Surely if the MAC doesn't support modes, then they > > should be filtered out of phydev->supported_eee so that userspace knows > > that the mode is not supported by the network interface as a whole, just > > like we do for phydev->supported. > > > > That would give us the checking here. > > > Removing EEE modes to be disabled from supported_eee is problematic > because of how genphy_c45_write_eee_adv() works. > > Let's say we have a 2.5Gbps PHY and want to disable EEE at 2.5Gbps. If we > remove 2.5Gbps from supported_eee, then the following check is false: > if (linkmode_intersects(phydev->supported_eee, PHY_EEE_CAP2_FEATURES)) > What would result in the 2.5Gbps mode not getting disabled. Ok. Do we at least remove the broken modes from the supported mask reported to userspace?
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c index 468d24611..b566faba9 100644 --- a/drivers/net/phy/phy-c45.c +++ b/drivers/net/phy/phy-c45.c @@ -1559,6 +1559,11 @@ int genphy_c45_ethtool_set_eee(struct phy_device *phydev, phydev_warn(phydev, "At least some EEE link modes are not supported.\n"); return -EINVAL; } + linkmode_and(tmp, adv, phydev->eee_disabled_modes); + if (!linkmode_empty(tmp)) { + phydev_warn(phydev, "At least some EEE link modes are disabled.\n"); + return -EINVAL; + } linkmode_copy(phydev->advertising_eee, adv); } else if (linkmode_empty(phydev->advertising_eee)) { phy_advertise_eee_all(phydev);
Link modes in phydev->eee_disabled_modes are filtered out by genphy_c45_write_eee_adv() and won't be advertised. Therefore don't accept such modes from userspace. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy-c45.c | 5 +++++ 1 file changed, 5 insertions(+)