Date:   Tue, 14 Dec 2021 23:45:10 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Sean Anderson <sean.anderson@seco.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Marcin Wojtas <mw@semihalf.com>, UNGLinuxDriver@microchip.com,
        "David S . Miller" <davem@davemloft.net>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Jose Abreu <Jose.Abreu@synopsys.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH] net: phylink: Pass state to pcs_config
Message-ID: <YbkshnqgXP7Gd188@shell.armlinux.org.uk>
References: <20211214233450.1488736-1-sean.anderson@seco.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211214233450.1488736-1-sean.anderson@seco.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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 (which is entirely different
from normal autonegotiation.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!