| 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
| ||
|
Message-ID: <9891e1bd-7d42-8181-b874-e1d3cc64f21c@seco.com>
Date: Thu, 16 Dec 2021 14:00:32 -0500
From: Sean Anderson <sean.anderson@...o.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Jose Abreu <Jose.Abreu@...opsys.com>, Andrew Lunn <andrew@...n.ch>,
linux-kernel@...r.kernel.org,
"David S . Miller" <davem@...emloft.net>,
UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
Ioana Ciornei <ioana.ciornei@....com>,
Jakub Kicinski <kuba@...nel.org>,
Marcin Wojtas <mw@...ihalf.com>,
Horatiu Vultur <horatiu.vultur@...rochip.com>,
Claudiu Beznea <claudiu.beznea@...rochip.com>,
linux-arm-kernel@...ts.infradead.org,
Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH] net: phylink: Pass state to pcs_config
On 12/16/21 1:29 PM, Sean Anderson wrote:
>
>
> On 12/16/21 1:05 PM, Russell King (Oracle) wrote:
>> On Thu, Dec 16, 2021 at 12:51:33PM -0500, Sean Anderson wrote:
>>> On 12/16/21 12:26 PM, Russell King (Oracle) wrote:
>>> > On Thu, Dec 16, 2021 at 12:02:55PM -0500, Sean Anderson wrote:
>>> > > On 12/14/21 8:12 PM, Russell King (Oracle) wrote:
>>> > > > On Wed, Dec 15, 2021 at 12:49:14AM +0000, Russell King (Oracle) wrote:
>>> > > > > On Tue, Dec 14, 2021 at 07:16:53PM -0500, Sean Anderson wrote:
>>> > > > > > 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.
>>> > > > >
>>> > > > > Having actually played with this hardware quite a bit and observed what
>>> > > > > it sends, what it implements for advertising is:
>>> > > > >
>>> > > > > Config_Reg.PAUSE = Pause_Adv;
>>> > >
>>> > > So the above note from the datasheet about Pause_Adv not being valid is
>>> > > incorrect?
>>> > >
>>> > > > > Config_Reg gets sent over the 1000BASE-X link to the link partner, and
>>> > > > > we receive Remote_Reg from the link partner.
>>> > > > >
>>> > > > > Then, the hardware implements:
>>> > > > >
>>> > > > > if (AnFcEn)
>>> > > > > MAC_PAUSE = Config_Reg.PAUSE & Remote_Reg.PAUSE;
>>> > > > > else
>>> > > > > MAC_PAUSE = SetFcEn;
>>> > > > >
>>> > > > > In otherwords, AnFcEn controls whether the result of autonegotiation
>>> > > > > or the value of SetFcEn controls whether the MAC enables symmetric
>>> > > > > pause mode.
>>> > > >
>>> > > > I should also note that in the Port Status register,
>>> > > >
>>> > > > TxFcEn = RxFcEn = MAC_PAUSE;
>>> > > >
>>> > > > So, the status register bits follow SetFcEn when AnFcEn is disabled.
>>> > > >
>>> > > > However, these bits are the only way to report the result of the
>>> > > > negotiation, which is why we use them to report back whether flow
>>> > > > control was enabled in mvneta_pcs_get_state(). These bits will be
>>> > > > ignored by phylink when ethtool -A has disabled pause negotiation,
>>> > > > and in that situation there is no way as I said to be able to read
>>> > > > the negotiation result.
>>> > >
>>> > > Ok, how about
>>> > >
>>> > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>>> > > index b1cce4425296..9b41d8ee71fb 100644
>>> > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>>> > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>>> > > @@ -6226,8 +6226,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
>>> > > * automatically or the bits in MVPP22_GMAC_CTRL_4_REG
>>> > > * manually controls the GMAC pause modes.
>>> > > */
>>> > > - if (permit_pause_to_mac)
>>> > > - val |= MVPP2_GMAC_FLOW_CTRL_AUTONEG;
>>> > > + val |= MVPP2_GMAC_FLOW_CTRL_AUTONEG;
>>> > >
>>> > > /* Configure advertisement bits */
>>> > > mask |= MVPP2_GMAC_FC_ADV_EN | MVPP2_GMAC_FC_ADV_ASM_EN;
>>> > > @@ -6525,6 +6524,9 @@ static void mvpp2_mac_link_up(struct phylink_config *config,
>>> > > }
>>> > > } else {
>>> > > if (!phylink_autoneg_inband(mode)) {
>>> > > + bool cur_tx_pause, cur_rx_pause;
>>> > > + u32 status0 = readl(port->base + MVPP2_GMAC_STATUS0);
>>> > > +
>>> > > val = MVPP2_GMAC_FORCE_LINK_PASS;
>>> > >
>>> > > if (speed == SPEED_1000 || speed == SPEED_2500)
>>> > > @@ -6535,11 +6537,18 @@ static void mvpp2_mac_link_up(struct phylink_config *config,
>>> > > if (duplex == DUPLEX_FULL)
>>> > > val |= MVPP2_GMAC_CONFIG_FULL_DUPLEX;
>>> > >
>>> > > + cur_tx_pause = status0 & MVPP2_GMAC_STATUS0_TX_PAUSE;
>>> > > + cur_rx_pause = status0 & MVPP2_GMAC_STATUS0_RX_PAUSE;
>>> >
>>> > I think you haven't understood everything I've said. These status bits
>>> > report what the MAC is doing. They do not reflect what was negotiated
>>> > _unless_ MVPP2_GMAC_FLOW_CTRL_AUTONEG was set.
>>> >
>>> > So, if we clear MVPP2_GMAC_FLOW_CTRL_AUTONEG, these bits will follow
>>> > MVPP22_XLG_CTRL0_TX_FLOW_CTRL_EN and MVPP22_XLG_CTRL0_RX_FLOW_CTRL_EN.
>>> >
>>> > Let's say we apply this patch. tx/rx pause are negotiated and enabled.
>>> > So cur_tx_pause and cur_rx_pause are both true.
>>> >
>>> > We change the pause settings, forcing tx pause only. This causes
>>> > pcs_config to be called which sets MVPP2_GMAC_FLOW_CTRL_AUTONEG, and
>>> > then link_up gets called with the differing settings. We clear
>>> > MVPP2_GMAC_FLOW_CTRL_AUTONEG and force the pause settings. We now
>>> > have the status register containing MVPP2_GMAC_STATUS0_TX_PAUSE set
>>> > but MVPP2_GMAC_STATUS0_RX_PAUSE clear.
>>> >
>>> > The link goes down e.g. because the remote end has changed and comes
>>> > back. We read the status register and see MVPP2_GMAC_STATUS0_TX_PAUSE
>>> > is set and MVPP2_GMAC_STATUS0_RX_PAUSE is still clear. tx_pause is
>>> > true and rx_pause is false. These agree with the settings, so we
>>> > then set MVPP2_GMAC_FLOW_CTRL_AUTONEG.
>>> >
>>> > If the link goes down and up again, then this cycle repeats - the
>>> > status register will now have both MVPP2_GMAC_STATUS0_TX_PAUSE and
>>> > MVPP2_GMAC_STATUS0_RX_PAUSE set, so we clear
>>> > MVPP2_GMAC_FLOW_CTRL_AUTONEG. If the link goes down/up again, we flip
>>> > back to re-enabling MVPP2_GMAC_FLOW_CTRL_AUTONEG.
Ok, so I think I see what you're getting at here. If we set
MVPP2_GMAC_FLOW_CTRL_AUTONEG after examining the pause mode, then the
pause mode will revert to what was negotiated. But since this platform
uses interrupts, we can clear it in link_up and set it in link_down.
--Sean
Powered by blists - more mailing lists