netdev - Re: [PATCH net-next v2 1/1] net: sxgbe: rework EEE handling based on PHY negotiation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z6X0WsUaRw-P-QVt@shell.armlinux.org.uk>
Date: Fri, 7 Feb 2025 11:54:02 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Byungho An <bh74.an@...sung.com>, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	kernel@...gutronix.de, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/1] net: sxgbe: rework EEE handling based on
 PHY negotiation

Hi,

A more in-depth review...

On Fri, Feb 07, 2025 at 11:56:10AM +0100, Oleksij Rempel wrote:
> @@ -228,7 +212,7 @@ static void sxgbe_get_ethtool_stats(struct net_device *dev,
>  	int i;
>  	char *p;
>  
> -	if (priv->eee_enabled) {
> +	if (dev->phydev->eee_active) {
>  		int val = phy_get_eee_err(dev->phydev);
>  
>  		if (val)

Why should the EEE error statistic be dependent on whether EEE has been
negotiated? (I think a follow-up patch addressing this would be
appropriate.)

> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> index 12c8396b6942..8a385c92a6d1 100644
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_main.c
> @@ -87,7 +87,7 @@ static void sxgbe_enable_eee_mode(const struct sxgbe_priv_data *priv)
>  		priv->hw->mac->set_eee_mode(priv->ioaddr);
>  }
>  
> -void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv)
> +static void sxgbe_disable_eee_mode(struct sxgbe_priv_data * const priv)
>  {
>  	/* Exit and disable EEE in case of we are in LPI state. */
>  	priv->hw->mac->reset_eee_mode(priv->ioaddr);

Looking at this function, I think it's buggy and needs fixing prior to
this patch.

This function calls ->reset_eee_mode(), then synchronously deletes the
timer.

However, if the timer is running and fires after ->reset_eee_mode()
has been called, and tx_path_in_lpi_mode is false, then
sxgbe_eee_ctrl_timer() will execute, calling sxgbe_enable_eee_mode().
This will then call set_eee_mode().

In other words, this function is racy if not already called with
tx_path_in_lpi_mode set true - and there are paths in this driver that
does happen (just like in stmmac which I've been fixing - I suspect
one or other driver copied the code since the structure and member
names are identical.)

I would suggest deleting the timer as the very first thing, much like
what I did in:

	net: stmmac: delete software timer before disabling LPI

In fact, given the similarity with stmmac, it's probably worth looking
through my stmmac EEE cleanup patch series, and deciding which of those
changes also apply to sxgbe.

... and given this, I ask again: should there be a generic
software-timer EEE implementation so we're not having to patch multiple
drivers for the same bug(s).

> @@ -110,52 +110,25 @@ static void sxgbe_eee_ctrl_timer(struct timer_list *t)
>  	mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));
>  }
>  
> -/**
> - * sxgbe_eee_init
> - * @priv: private device pointer
> - * Description:
> - *  If the EEE support has been enabled while configuring the driver,
> - *  if the GMAC actually supports the EEE (from the HW cap reg) and the
> - *  phy can also manage EEE, so enable the LPI state and start the timer
> - *  to verify if the tx path can enter in LPI state.
> - */
> -bool sxgbe_eee_init(struct sxgbe_priv_data * const priv)
> +static void sxgbe_eee_adjust(struct sxgbe_priv_data *priv)
>  {
> -	struct net_device *ndev = priv->dev;
> -	bool ret = false;
> +	struct phy_device *phydev = priv->dev->phydev;
>  
> -	/* MAC core supports the EEE feature. */
> -	if (priv->hw_cap.eee) {
> -		/* Check if the PHY supports EEE */
> -		if (phy_init_eee(ndev->phydev, true))
> -			return false;
> +	if (!priv->hw_cap.eee)
> +		return;
>  
> -		timer_setup(&priv->eee_ctrl_timer, sxgbe_eee_ctrl_timer, 0);
> -		priv->eee_ctrl_timer.expires = SXGBE_LPI_TIMER(eee_timer);
> +	if (phydev->enable_tx_lpi) {
>  		add_timer(&priv->eee_ctrl_timer);
> -
>  		priv->hw->mac->set_eee_timer(priv->ioaddr,
>  					     SXGBE_DEFAULT_LPI_TIMER,
> -					     priv->tx_lpi_timer);
> -
> -		pr_info("Energy-Efficient Ethernet initialized\n");
> -
> -		ret = true;
> +					     phydev->eee_cfg.tx_lpi_timer);
> +		priv->eee_enabled = true;
> +	} else {
> +		sxgbe_disable_eee_mode(priv);
> +		priv->eee_enabled = false;

Given what sxgbe_tx_all_clean() does, we need priv->eee_enabled set
false and visible to sxgbe_tx_all_clean() before calling
sxgbe_disable_eee_mode(), otherwise sxgbe_tx_all_clean() may race and
add the eee_ctrl_timer back after sxgbe_disable_eee_mode() has
removed it.

> @@ -802,7 +785,7 @@ static void sxgbe_tx_all_clean(struct sxgbe_priv_data * const priv)
>  		sxgbe_tx_queue_clean(tqueue);
>  	}
>  
> -	if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
> +	if (priv->eee_enabled && !priv->tx_path_in_lpi_mode) {
>  		sxgbe_enable_eee_mode(priv);
>  		mod_timer(&priv->eee_ctrl_timer, SXGBE_LPI_TIMER(eee_timer));

As noted above, this can race with sxgbe_disable_eee_mode().

Thanks.

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