linux-kernel - Re: [PATCH net] net: spacemit: Implement emac_set

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] [day] [month] [year] [list]

Message-ID: <2eb5f9fc-d173-4b9e-89a3-87ad17ddd163@lunn.ch>
Date: Thu, 30 Oct 2025 22:32:24 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Vivian Wang <wangruikang@...as.ac.cn>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Yixun Lan <dlan@...too.org>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
	netdev@...r.kernel.org, linux-riscv@...ts.infradead.org,
	spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

On Thu, Oct 30, 2025 at 10:31:44PM +0800, Vivian Wang wrote:
> emac_set_pauseparam (the set_pauseparam callback) didn't properly update
> phydev->advertising. Fix it by changing it to call phy_set_asym_pause.

This patch is doing a lot more than that.

Please break this patch up into smaller parts.

One obvious part you can break out is emac_get_pauseparam() reading
from hardware rather that state variables.

>  static int emac_set_pauseparam(struct net_device *dev,
>  			       struct ethtool_pauseparam *pause)
>  {
>  	struct emac_priv *priv = netdev_priv(dev);
> -	u8 fc = 0;
> +	struct phy_device *phydev = dev->phydev;
>  
> -	priv->flow_control_autoneg = pause->autoneg;
> +	if (!phydev)
> +		return -ENODEV;

I'm not sure that is the correct condition. emac_up() will fail if it
cannot find the PHY. What you need to be testing here is if the
interface is admin down, and so is not connected to the PHY. If so,
-ENETDOWN would be more appropriate.

> -	if (pause->autoneg) {
> -		emac_set_fc_autoneg(priv);
> -	} else {
> -		if (pause->tx_pause)
> -			fc |= FLOW_CTRL_TX;
> +	if (!phy_validate_pause(phydev, pause))
> +		return -EINVAL;
>  
> -		if (pause->rx_pause)
> -			fc |= FLOW_CTRL_RX;
> +	priv->flow_control_autoneg = pause->autoneg;
>  
> -		emac_set_fc(priv, fc);
> -	}
> +	phy_set_asym_pause(dev->phydev, pause->rx_pause, pause->tx_pause);

It is hard to read what this patch is doing, but there are 3 use cases.

1) general autoneg for link speed etc, and pause autoneg
2) general autoneg for link speed etc, and forced pause
3) forced link speed etc, and forced pause.

I don't see all these being handled. It gets much easier to get this
right if you make use of phylink, since phylink handles all the
business logic for you.

	Andrew