netdev - Re: [PATCH] net: phylink: Pass state to pcs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <de1f7214-58c8-cdc6-1d29-08c979ce68f1@seco.com>
Date:   Tue, 14 Dec 2021 19:16:53 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Marcin Wojtas <mw@...ihalf.com>, UNGLinuxDriver@...rochip.com,
        "David S . Miller" <davem@...emloft.net>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: 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:
>> 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