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

On Thu, Dec 30, 2021 at 05:23:56PM -0800, Dimitris Michailidis wrote:

quoted

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

quoted

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?

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.

OK. If I understand correctly AN on and pause->autoneg != 0 means
negotiate requested settings and accept resolution result, while
AN on and pause->autoneg == 0 means negotiate settings and adopt
them as the resolution result. I'll mark the latter combination unsupported
for now.

    Andrew