Re: [PATCH net-next 1/5] net: wangxun: add flow control support

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

On Wed, Nov 22, 2023 at 05:15:05PM +0100, Andrew Lunn wrote:
> > +static void ngbe_get_pauseparam(struct net_device *netdev,
> > +				struct ethtool_pauseparam *pause)
> > +{
> > +	struct wx *wx = netdev_priv(netdev);
> > +
> > +	pause->autoneg = !wx->fc.disable_fc_autoneg;
> 
> Maybe call it enable_fs_autoneg, since that is what the kAPI uses?

fs?

> > +	wx->fc.disable_fc_autoneg = !pause->autoneg;
> > +	wx->fc.tx_pause = pause->tx_pause;
> > +	wx->fc.rx_pause = pause->rx_pause;
> > +
> > +	phy_set_asym_pause(wx->phydev, pause->rx_pause, pause->tx_pause);
> 
> You should only be doing this if pause->autoneg is true. If it is
> false, you ignore the results from autoneg, and just configure the
> hardware as indicated by pause->{tr}x_pause.

However, if autoneg is supported, but pause autoneg is disabled, there
is still the need to update the PHY's advertisement so the remote end
knows what's going on, as documented in the user API:

" * If the link is autonegotiated, drivers should use
  * mii_advertise_flowctrl() or similar code to set the advertised
  * pause frame capabilities based on the @rx_pause and @tx_pause flags,
  * even if @autoneg is zero. ... "

You are correct that when !pause->autoneg, tx_pause/rx_pause are to be
used in place of the negotiated versions.

Also... when getting the pause parameters, tx_pause/rx_pause _should_
reflect what was set for these parameters via the set function, *not*
the current state affected by negotiation.

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