Re: [PATCH] net: phylink: Pass state to pcs_config

[Sean Anderson]

On 12/14/21 6:45 PM, Russell King (Oracle) wrote:
> On Tue, Dec 14, 2021 at 06:34:50PM -0500, Sean Anderson wrote:
> > Although most PCSs only need the interface and advertising to configure
> > themselves, there is an oddly named "permit_pause_to_mac" parameter
> > included as well, and only used by mvpp2. This parameter indicates
> > whether pause settings should be autonegotiated or not. mvpp2 needs this
> > because it cannot both set the pause mode manually and and advertise
> > pause support. That is, if you want to set the pause mode, you have to
> > advertise that you don't support flow control. We can't just
> > autonegotiate the pause mode and then set it manually, because if
> > the link goes down we will start advertising the wrong thing. So
> > instead, we have to set it up front during pcs_config. However, we can't
> > determine whether we are autonegotiating flow control based on our
> > advertisement (since we advertise flow control even when it is set
> > manually).
> >
> > So we have had this strange additional argument tagging along which is
> > used by one driver (though soon to be one more since mvneta has the same
> > problem). We could stick MLO_PAUSE_AN in the "mode" parameter, since
> > that contains other autonegotiation configuration. However, there are a
> > lot of places in the codebase which do a direct comparison (e.g. mode ==
> > MLO_AN_FIXED), so it would be difficult to add an extra bit without
> > breaking things. But this whole time, mac_config has been getting the
> > whole state, and it has not suffered unduly. So just pass state and
> > eliminate these other parameters.
>
> Please no. This is a major step backwards.
>
> mac_config() suffers from the proiblem that people constantly
> mis-understand what they can access in "state" and what they can't.
> This patch introduces exactly the same problem but for a new API.
>
> I really don't want to make that same mistake again, and this patch
> is making that same mistake.
>
> The reason mvpp2 and mvneta are different is because they have a
> separate bit to allow the results of pause mode negotiation to be
> forwarded to the MAC, and that bit needs to be turned off if the
> pause autonegotiation is disabled

Ok, so let me clarify my understanding. Perhaps this can be eliminated
through a different approach.

When I read the datasheet for mvneta (which hopefully has the same
logic here, since I could not find a datasheet for an mvpp2 device), I
noticed that the Pause_Adv bit said

> It is valid only if flow control mode is defined by Auto-Negotiation
> (as defined by the <AnFcEn> bit).

Which I interpreted to mean that if AnFcEn was clear, then no flow
control was advertised. But perhaps it instead means that the logic is
something like

if (AnFcEn)
	Config_Reg.PAUSE = Pause_Adv;
else
	Config_Reg.PAUSE = SetFcEn;

which would mean that we can just clear AnFcEn in link_up if the
autonegotiated pause settings are different from the configured pause
settings.

> (which is entirely different from normal autonegotiation.)

AFAIK pause autonegotiation happens in the same autonegotiation word
transfer as e.g. duplex autonegotiation. So it is just a subset of the
other which is configurable separately in Linux.

--Sean