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

[Andrew Lunn <andrew@lunn.ch>]

> @@ -1552,6 +1552,7 @@ static int marvell_read_status_page(struct phy_device *phydev, int page)
>  	phydev->asym_pause = 0;
>  	phydev->speed = SPEED_UNKNOWN;
>  	phydev->duplex = DUPLEX_UNKNOWN;
> +	phydev->port = fiber ? PORT_FIBRE : PORT_TP;
>  
>  	if (phydev->autoneg == AUTONEG_ENABLE)
>  		err = marvell_read_status_page_an(phydev, fiber, status);

...

> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -308,7 +308,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
>  	if (phydev->interface == PHY_INTERFACE_MODE_MOCA)
>  		cmd->base.port = PORT_BNC;
>  	else
> -		cmd->base.port = PORT_MII;
> +		cmd->base.port = phydev->port;
>  	cmd->base.transceiver = phy_is_internal(phydev) ?
>  				XCVR_INTERNAL : XCVR_EXTERNAL;
>  	cmd->base.phy_address = phydev->mdio.addr;

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.

     Andrew