Re: [PATCH net] net: libwx: fix to set pause param

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

On Fri, Apr 25, 2025 at 05:20:53PM +0800, Jiawen Wu wrote:
> On Fri, Apr 25, 2025 3:38 PM, Russell King (Oracle) wrote:
> > On Fri, Apr 25, 2025 at 03:09:42PM +0800, Jiawen Wu wrote:
> > > @@ -266,11 +266,20 @@ int wx_set_pauseparam(struct net_device *netdev,
> > >  		      struct ethtool_pauseparam *pause)
> > >  {
> > >  	struct wx *wx = netdev_priv(netdev);
> > > +	int err;
> > >
> > >  	if (wx->mac.type == wx_mac_aml)
> > >  		return -EOPNOTSUPP;
> > >
> > > -	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > > +	err = phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (wx->fc.rx_pause != pause->rx_pause ||
> > > +	    wx->fc.tx_pause != pause->tx_pause)
> > > +		return wx_fc_enable(wx, pause->tx_pause, pause->rx_pause);
> > 
> > Why? phylink_ethtool_set_pauseparam() will cause mac_link_down() +
> > mac_link_up() to be called with the new parameters.
> > 
> > One of the points of phylink is to stop drivers implementing stuff
> > buggily - which is exactly what the above is.
> > 
> > ->rx_pause and ->tx_pause do not set the pause enables unconditionally.
> > Please read the documentation in include/uapi/linux/ethtool.h which
> > states how these two flags are interpreted, specifically the last
> > paragraph of the struct's documentation.
> > 
> > I'm guessing your change comes from a misunderstanding how the
> > interface is supposed to work and you believe that phylink isn't
> > implementing it correctly.
> 
> You are right.
> I should set autoneg off first, although there has no autoneg bit in this link mode.

Yes, "autoneg" in the pause API selects between using the result of
autonegotiation if enabled, or using the values from tx/rx in the
pause API.

If autonegotiation (as in the control in SLINKSETTINGS) is disabled
or autonegotiation is unsupported, then "autoneg" set in the pause
parameters results in no pause. The same is incidentally true of EEE
settings as well.

So, "autoneg" in SLINKSETTINGS is like the big switch allowing or
preventing all autonegotiation over the link. The other "autoneg"s
control whether the result of autonegotiation is used.

There is one thing in the ethtool_pauseparam documentation that should
be removed:

 * Drivers should reject a non-zero setting of @autoneg when
 * autoneogotiation is disabled (or not supported) for the link.

Let's think about what that means.

- I have a 100baseT/FD link for example, and it used autoneg, and has
  pause enabled.
- I decide to disable autoneg, instead selecting fixed-link mode
  through SLINKSETTINGS.
- Reading the pause parameter settings returns the original state of
  "autoneg" which is set.
- Writing that back results in "rejection" if the above statement is
  followed - which is non-sensical. Let's say it's forced to zero.
- I later re-enable autoneg via SLINKSETTINGS
- I now have to remember to modify the pause mode parameters to
  re-enable pause autoneg.

Things get worse if instead of the above, disabling SLINKSETTINGS
autoneg results in the pause param autoneg being immediately disabled
without API changes - we then end up with one API making magic changes
to settings in another API, and I don't think that is what anyone
would reasonably expect to happen.

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