[PATCH net-next v2 1/1] net: phy: Clear link-specific data on link down

[PATCH net-next v2 1/1] net: phy: Clear link-specific data on link down

[Oleksij Rempel]

When a network interface is brought down, the associated PHY is stopped.
However, several link-specific parameters within the phy_device struct
are not cleared. This leads to userspace tools like ethtool reporting
stale information from the last active connection, which is misleading
as the link is no longer active.

For example, after running `ip link set dev lan2 down`, ethtool might
show the following outdated information, indicating a 1000Mb/s Full
duplex link with a link partner, even though the link is detected as down:

  # ethtool lan2
  Settings for lan2:
        Supported ports: [ TP MII ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        ...
        Link partner advertised link modes:  10baseT/Half 10baseT/Full
                                             100baseT/Half 100baseT/Full
                                             1000baseT/Full
        ...
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        master-slave cfg: preferred master
        master-slave status: slave
        ...
        Link detected: no

This patch fixes the issue by clearing all outdated link parameters within
the `phy_link_down()` function. This seems to be the correct place to reset
this data, as it is called whenever the link transitions to a down state.

The following parameters are now reset:
- Speed and duplex are set to UNKNOWN.
- Link partner advertising information is zeroed out.
- Master/slave state is set to UNKNOWN.
- MDI-X status is set to INVALID.
- EEE active status is set to false.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v2:
- rebase and resned against net-next

 drivers/net/phy/phy.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 13df28445f02..6bdd49d93740 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -83,6 +83,16 @@ static void phy_link_down(struct phy_device *phydev)
 {
 	phydev->phy_link_change(phydev, false);
 	phy_led_trigger_change_speed(phydev);
+
+	/* Clear the outdated link parameters */
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	if (phydev->master_slave_state != MASTER_SLAVE_STATE_UNSUPPORTED)
+		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+	phydev->mdix = ETH_TP_MDI_INVALID;
+	linkmode_zero(phydev->lp_advertising);
+	phydev->eee_active = false;
+
 	WRITE_ONCE(phydev->link_down_events, phydev->link_down_events + 1);
 }

--
2.39.5

Re: [PATCH net-next v2 1/1] net: phy: Clear link-specific data on link down

[Russell King (Oracle)]

On Fri, Aug 22, 2025 at 11:09:47AM +0200, Oleksij Rempel wrote:
> When a network interface is brought down, the associated PHY is stopped.
> However, several link-specific parameters within the phy_device struct
> are not cleared. This leads to userspace tools like ethtool reporting
> stale information from the last active connection, which is misleading
> as the link is no longer active.

This is not a good idea. Consider the case where the PHY has been
configured with autoneg disabled, and phydev->speed etc specifies
the desired speed.

When the link goes down, all that state gets cleared, and we lose
the user's settings.

So no, I don't think this is appropriate.

I think it is appropriate to clear some of the state, but anything that
the user can configure (such as ->speed and ->duplex) must not be
cleared.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2 1/1] net: phy: Clear link-specific data on link down

[Oleksij Rempel]

On Fri, Aug 22, 2025 at 10:19:24AM +0100, Russell King (Oracle) wrote:
> On Fri, Aug 22, 2025 at 11:09:47AM +0200, Oleksij Rempel wrote:
> > When a network interface is brought down, the associated PHY is stopped.
> > However, several link-specific parameters within the phy_device struct
> > are not cleared. This leads to userspace tools like ethtool reporting
> > stale information from the last active connection, which is misleading
> > as the link is no longer active.
> 
> This is not a good idea. Consider the case where the PHY has been
> configured with autoneg disabled, and phydev->speed etc specifies
> the desired speed.
> 
> When the link goes down, all that state gets cleared, and we lose
> the user's settings.
> 
> So no, I don't think this is appropriate.
> 
> I think it is appropriate to clear some of the state, but anything that
> the user can configure (such as ->speed and ->duplex) must not be
> cleared.

Hm... should it be cleared conditionally? If autoneg is used, clear the
speed and duplex?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v2 1/1] net: phy: Clear link-specific data on link down

[Russell King (Oracle)]

On Fri, Aug 22, 2025 at 11:29:01AM +0200, Oleksij Rempel wrote:
> On Fri, Aug 22, 2025 at 10:19:24AM +0100, Russell King (Oracle) wrote:
> > On Fri, Aug 22, 2025 at 11:09:47AM +0200, Oleksij Rempel wrote:
> > > When a network interface is brought down, the associated PHY is stopped.
> > > However, several link-specific parameters within the phy_device struct
> > > are not cleared. This leads to userspace tools like ethtool reporting
> > > stale information from the last active connection, which is misleading
> > > as the link is no longer active.
> > 
> > This is not a good idea. Consider the case where the PHY has been
> > configured with autoneg disabled, and phydev->speed etc specifies
> > the desired speed.
> > 
> > When the link goes down, all that state gets cleared, and we lose
> > the user's settings.
> > 
> > So no, I don't think this is appropriate.
> > 
> > I think it is appropriate to clear some of the state, but anything that
> > the user can configure (such as ->speed and ->duplex) must not be
> > cleared.
> 
> Hm... should it be cleared conditionally? If autoneg is used, clear the
> speed and duplex?

Actually no, you can't do any of this here.

Look at phy_ethtool_set_eee_noneg(). If the LPI configuration changes,
it needs to inform the MAC of the change, and it does that by
_simulating_ a brief loss of link by calling phy_link_down() followed
by phy_link_up() - expecting all state to be preserved except that
which needs to be modified.

Maybe we should do something in the PHY_HALTED state in
_phy_state_machine() ?

Note that in the usual case of link down, it is the responsibility of
phy_read_status() to update most of what you're clearing, the
exception is phydev->eee_active and phydev->enable_tx_lpi which are
managed by phylib itself.

I think that both the PHY_HALTED and PHY_ERROR states should be
clearing phydev->eee_active and phydev->enable_tx_lpi, so that
should be in the existing if().

I'm not sure we want to clear out the remainder of the state on
PHY_ERROR, as this would aid debugging when something goes wrong.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!