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

[Sean Anderson <sean.anderson@seco.com>]

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