Re: [PATCH net] net: spacemit: Implement emac_set_pauseparam properly

[Andrew Lunn <andrew@lunn.ch>]

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