Re: [PATCH net-next] net: phy: introduce phydev->port

On Wed, Feb 10, 2021 at 02:51:34AM +0100, Andrew Lunn wrote:

This is a general comment, not a problem specific to this patch.

There is some interesting race conditions here. The marvell driver
first checks the fibre page and gets the status of the fiber port. As
you can see from the hunk above, it clears out pause, duplex, speed,
sets port to PORT_FIBRE, and then reads the PHY registers to set these
values. If link is not detected on the fibre, it swaps page and does
it all again, but for the copper port. So once per second,
phydev->port is going to flip flop PORT_FIBER->PORT_TP, if copper has
link.

Now, the read_status() call into the driver should be performed while
holding the phydev->lock. So to the PHY state machine, this flip/flop
does not matter, it is atomic with respect to the lock. But
phy_ethtool_ksettings_get() is not talking the lock. It could see
speed, duplex, and port while they have _UNKNOWN values, or port is
part way through a flip flop. I think we need to take the lock here.
phy_ethtool_ksettings_set() should also probably take the lock.

phy_ethtool_ksettings_get() needs to take the lock, otherwise it could
read the phy_device members in the middle of an update. This is likely
a long-standing phylib bug.

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