| 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: <91232aa9-2681-2910-f480-8fc75149a896@seco.com>
Date: Thu, 21 Jul 2022 12:24:50 -0400
From: Sean Anderson <sean.anderson@...o.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Alexandru Marginean <alexandru.marginean@....com>,
Paolo Abeni <pabeni@...hat.com>,
"David S . Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org, Vladimir Oltean <olteanv@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH v2 07/11] net: phylink: Adjust link settings based on rate
adaptation
On 7/20/22 2:50 AM, Russell King (Oracle) wrote:
> On Tue, Jul 19, 2022 at 07:49:57PM -0400, Sean Anderson wrote:
>> If the phy is configured to use pause-based rate adaptation, ensure that
>> the link is full duplex with pause frame reception enabled. As
>> suggested, if pause-based rate adaptation is enabled by the phy, then
>> pause reception is unconditionally enabled.
>>
>> The interface duplex is determined based on the rate adaptation type.
>> When rate adaptation is enabled, so is the speed. We assume the maximum
>> interface speed is used. This is only relevant for MLO_AN_PHY. For
>> MLO_AN_INBAND, the MAC/PCS's view of the interface speed will be used.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
>> ---
>>
>> Changes in v2:
>> - Use the phy's rate adaptation setting to determine whether to use its
>> link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND.
>> - Always use the rate adaptation setting to determine the interface
>> speed/duplex (instead of sometimes using the interface mode).
>>
>> drivers/net/phy/phylink.c | 126 ++++++++++++++++++++++++++++++++++----
>> include/linux/phylink.h | 1 +
>> 2 files changed, 114 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index da0623d94a64..619ef553476f 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -160,16 +160,93 @@ static const char *phylink_an_mode_str(unsigned int mode)
>> * @state: A link state
>> *
>> * Update the .speed and .duplex members of @state. We can determine them based
>> - * on the .link_speed and .link_duplex. This function should be called whenever
>> - * .link_speed and .link_duplex are updated. For example, userspace deals with
>> - * link speed and duplex, and not the interface speed and duplex. Similarly,
>> - * phys deal with link speed and duplex and only implicitly the interface speed
>> - * and duplex.
>> + * on the .link_speed, .link_duplex, .interface, and .rate_adaptation. This
>> + * function should be called whenever .link_speed and .link_duplex are updated.
>> + * For example, userspace deals with link speed and duplex, and not the
>> + * interface speed and duplex. Similarly, phys deal with link speed and duplex
>> + * and only implicitly the interface speed and duplex.
>> */
>> static void phylink_state_fill_speed_duplex(struct phylink_link_state *state)
>> {
>> - state->speed = state->link_speed;
>> - state->duplex = state->link_duplex;
>> + switch (state->rate_adaptation) {
>> + case RATE_ADAPT_NONE:
>> + state->speed = state->link_speed;
>> + state->duplex = state->link_duplex;
>> + return;
>> + case RATE_ADAPT_PAUSE:
>> + state->duplex = DUPLEX_FULL;
>> + break;
>> + case RATE_ADAPT_CRS:
>> + state->duplex = DUPLEX_HALF;
>> + break;
>> + case RATE_ADAPT_OPEN_LOOP:
>> + state->duplex = state->link_duplex;
>> + break;
>> + }
>> +
>> + /* Use the max speed of the interface */
>> + switch (state->interface) {
>> + case PHY_INTERFACE_MODE_100BASEX:
>> + case PHY_INTERFACE_MODE_REVRMII:
>> + case PHY_INTERFACE_MODE_RMII:
>> + case PHY_INTERFACE_MODE_SMII:
>> + case PHY_INTERFACE_MODE_REVMII:
>> + case PHY_INTERFACE_MODE_MII:
>> + state->speed = SPEED_100;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_TBI:
>> + case PHY_INTERFACE_MODE_MOCA:
>> + case PHY_INTERFACE_MODE_RTBI:
>> + case PHY_INTERFACE_MODE_1000BASEX:
>> + case PHY_INTERFACE_MODE_1000BASEKX:
>> + case PHY_INTERFACE_MODE_TRGMII:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII:
>> + case PHY_INTERFACE_MODE_QSGMII:
>> + case PHY_INTERFACE_MODE_SGMII:
>> + case PHY_INTERFACE_MODE_GMII:
>> + state->speed = SPEED_1000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_2500BASEX:
>> + state->speed = SPEED_2500;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_5GBASER:
>> + state->speed = SPEED_5000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_XGMII:
>> + case PHY_INTERFACE_MODE_RXAUI:
>> + case PHY_INTERFACE_MODE_XAUI:
>> + case PHY_INTERFACE_MODE_10GBASER:
>> + case PHY_INTERFACE_MODE_10GKR:
>> + case PHY_INTERFACE_MODE_USXGMII:
>> + state->speed = SPEED_10000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_25GBASER:
>> + state->speed = SPEED_25000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_XLGMII:
>> + state->speed = SPEED_40000;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_INTERNAL:
>> + state->speed = state->link_speed;
>> + return;
>> +
>> + case PHY_INTERFACE_MODE_NA:
>> + case PHY_INTERFACE_MODE_MAX:
>> + state->speed = SPEED_UNKNOWN;
>> + return;
>> + }
>> +
>> + WARN_ON(1);
>> }
>>
>> /**
>> @@ -803,11 +880,12 @@ static void phylink_mac_config(struct phylink *pl,
>> const struct phylink_link_state *state)
>> {
>> phylink_dbg(pl,
>> - "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>> + "%s: mode=%s/%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>> __func__, phylink_an_mode_str(pl->cur_link_an_mode),
>> phy_modes(state->interface),
>> phy_speed_to_str(state->speed),
>> phy_duplex_to_str(state->duplex),
>> + phy_rate_adaptation_to_str(state->rate_adaptation),
>> __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
>> state->pause, state->link, state->an_enabled);
>>
>> @@ -944,6 +1022,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>> linkmode_zero(state->lp_advertising);
>> state->interface = pl->link_config.interface;
>> state->an_enabled = pl->link_config.an_enabled;
>> + state->rate_adaptation = pl->link_config.rate_adaptation;
>> if (state->an_enabled) {
>> state->link_speed = SPEED_UNKNOWN;
>> state->link_duplex = DUPLEX_UNKNOWN;
>> @@ -968,8 +1047,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>> else
>> state->link = 0;
>>
>> - state->link_speed = state->speed;
>> - state->link_duplex = state->duplex;
>> + if (state->rate_adaptation == RATE_ADAPT_NONE) {
>> + state->link_speed = state->speed;
>> + state->link_duplex = state->duplex;
>> + }
>
> So we need to have every PCS driver be udpated to fill in link_speed
> and link_duplex if rate_adaption != none.
The PCS doesn't know what the link speed/duplex is. If rate adaptation is
enabled, then the PCS only knows what the interface speed/duplex is.
> There's got to be a better way - maybe what I suggested in the last
> round of only doing the rate adaption thing in the link_up() functions,
> since that seems to be the only real difference.
>
> I'm not even sure we need to do that - in the "open loop" case, we
> need to be passing the media speed to the MAC driver with the knowledge
> that it should be increasing the IPG.
>
> So, I'm thinking we don't want any of these changes, what we instead
> should be doing is passing the media speed/duplex and the interface
> speed/duplex to the PCS and MAC.
> We can do that by storing the PHY rate adaption state, and processing
> that in phylink_link_up().
This approach sounds better. You patch below looks good. I'll test it
and use it for v3.
--Sean
Powered by blists - more mailing lists