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

On Thu, Dec 30, 2021 at 2:26 PM Andrew Lunn [off-list ref] wrote:

quoted

quoted

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?

quoted

If you force pause, then the link flaps and negotiates FW will apply the new
negotiated settings. For your scenario you'd want it to support only partial
application. Meanwhile the partner doesn't know we don't obey the negotiated
settings so I am suspicious that all of this would work.

What happen when Linux is controlling the hardware, not firmware, is
that phylib makes a callback into the MAC driver telling it the
results of the autoneg. Part of those results are what pause has been
negotiated, if pause is part of the negotiation. The MAC driver then
needs to program the MAC hardware with that information. If pause
autoneg is not being used, you directly program the hardware. When you
have hidden the hardware from Linux, you need a similar API to the
firmware to tell it how to program the hardware.

Forcing pause is mostly there to work around broken link peers who get
pause wrong. And unfortunately, lots of drivers get pause wrong, and
it is not helped by the API being poorly defined, and people
re-inventing the wheel by using firmware, not Linux to control the
hardware. But it also means you don't care too much if the link peer
is confused, it was probably doing the wrong thing anyway.

      Andrew