Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
> On 15.11.2024 15:12, Russell King (Oracle) wrote:
> > On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
> >> On 15.11.2024 12:11, Choong Yong Liang wrote:
> >>> From: Choong Yong Liang <yong.liang.choong@intel.com>
> >>>
> >>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> >>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> >>> the driver side is disabled. If we try to enable EEE through
> >>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> >>> matches the setting required to enable EEE in ethnl_set_eee().
> >>>
> >>> This patch series will remove phydev->eee_enabled and replace it with
> >>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> >>> will follow the master configuration to have software and hardware in sync,
> >>> allowing 'ethtool --show-eee' to display the correct value during the
> >>> initial stage.
> >>>
> >>> v2 changes:
> >>>  - Implement the prototype suggested by Russell
> >>>  - Check EEE before calling phy_support_eee()
> >>>
> >>> Thanks to Russell for the proposed prototype in [1].
> >>>
> >>> Reference:
> >>> [1] https://patchwork.kernel.org/comment/26121323/
> >>>
> >>> Choong Yong Liang (2):
> >>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
> >>>   net: stmmac: set initial EEE policy configuration
> >>>
> >>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
> >>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
> >>>  drivers/net/phy/phy_device.c                      |  6 +++---
> >>>  include/linux/phy.h                               |  5 ++---
> >>>  4 files changed, 13 insertions(+), 12 deletions(-)
> >>>
> >>
> >> Russell submitted the proposed patch already:
> >> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
> >> So there's no need for your patch 1.
> > 
> > Patch 1 is an updated version of that patch, minus my authorship and of
> > course no sign-off. I've already marked this series as requiring changes
> > in patchwork (hopefully, if I did it correctly.)
> > 
> 
> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
> and I wonder whether we can do better, as this results in several calls
> with the same argument. The following is an alternative, to be applied
> on top of your original patch. I don't have a clear preference, though.
> 
> ---
>  drivers/net/phy/phy.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 8876f3673..22c9bbebb 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>   * configuration.
>   */
>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> -				      struct ethtool_keee *data)
> +				      struct ethtool_keee *old_cfg)
>  {
> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
> -		eee_to_eeecfg(&phydev->eee_cfg, data);
> +	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
> +	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>  		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>  		if (phydev->link) {
>  			phydev->link = false;
> @@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>   */
>  int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
>  {
> +	struct eee_config old_cfg;
>  	int ret;
>  
>  	if (!phydev->drv)
>  		return -EIO;
>  
> +	old_cfg = phydev->eee_cfg;
> +	eee_to_eeecfg(&phydev->eee_cfg, data);
> +

Hmm, don't we want to do this under phydev->lock, because network
drivers and phylib may be reading from phydev->eee_cfg? If we
update it outside the lock, and then revert, there's a chance that
the phylib state machine / network driver may see the changes
which then get reverted on failure, potentially leading to
inconsistent state.

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