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

On 12/14/21 6:45 PM, Russell King (Oracle) wrote:

On Tue, Dec 14, 2021 at 06:34:50PM -0500, Sean Anderson wrote:

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel