Re: [PATCH net-next 4/8] net/funeth: ethtool operations

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 30, 2021 at 05:23:56PM -0800, Dimitris Michailidis wrote:
> On Thu, Dec 30, 2021 at 2:26 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > > I _think_ this is wrong. pause->autoneg means we are autoneg'ing
> > > > pause, not that we are using auto-neg in general. The user can have
> > > > autoneg turned on, but force pause by setting pause->autoneg to False.
> > > > In that case, the pause->rx_pause and pause->tx_pause are given direct
> > > > to the MAC, not auto negotiated.
> > >
> > > Having this mixed mode needs device FW support, which isn't there today.
> >
> > So if you are asked to set pause with pause->autoneg False, return
> > -EOPNOTSUPP. And pause get should always return True. It is O.K, to
> > support a subset of a feature, and say you don't support the
> > rest. That is much better than wrongly implementing it until your
> > firmware gets the needed support.
> 
> include/uapi/linux/ethtool.h has this comment
> 
>  * 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. ...
> 
> I read this as saying that pause->autoneg is ignored if AN is on and the
> requested pause settings are fed to AN. I believe this is what the code
> here implements.
> 
> Whereas you are saying that pause->autoneg == 0 should force
> despite AN. Right?

Take a look at phylink_ethtool_set_pauseparam() and accompanying
functions. This is a newish central implementation for any MAC/PHY
with Linux controlling the hardware. There are ambiguities in the API
description, so it would be better if your driver/firmware combo does
the same a the core Linux code. When Russell wrote that code, there
was quite a bit of discussion what the documentation actually means.

    Andrew