Messages in this thread | | | From | Sean Anderson <> | Subject | Re: [PATCH] net: phylink: Pass state to pcs_config | Date | Tue, 14 Dec 2021 19:16:53 -0500 |
| |
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
| |