| 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
| ||
|
Date: Mon, 20 Jul 2020 13:02:21 +0000
From: Ioana Ciornei <ioana.ciornei@....com>
To: Russell King - ARM Linux admin <linux@...linux.org.uk>
CC: Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Vladimir Oltean <vladimir.oltean@....com>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandru Marginean <alexandru.marginean@....com>,
"michael@...le.cc" <michael@...le.cc>,
"olteanv@...il.com" <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC net-next 11/13] net: phylink: re-implement interface
configuration with PCS
On 7/20/20 3:44 PM, Russell King - ARM Linux admin wrote:
> On Mon, Jul 20, 2020 at 11:40:44AM +0000, Ioana Ciornei wrote:
>> On 6/30/20 5:29 PM, Russell King wrote:
>>> With PCS support, how we implement interface reconfiguration is not up
>>> to the job; we end up reconfiguring the PCS for an interface change
>>> while the link could potentially be up. In order to solve this, add
>>> two additional MAC methods for interface configuration, one to prepare
>>> for the change, and one to finish the change.
>>>
>>> This allows mvneta and mvpp2 to shutdown what they require prior to the
>>> MAC and PCS configuration calls, and then restart as appropriate.
>>>
>>> This impacts ksettings_set(), which now needs to identify whether the
>>> change is a minor tweak to the advertisement masks or whether the
>>> interface mode has changed, and call the appropriate function for that
>>> update.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
>>> ---
>>> drivers/net/phy/phylink.c | 80 ++++++++++++++++++++++++++-------------
>>> include/linux/phylink.h | 48 +++++++++++++++++++++++
>>> 2 files changed, 102 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>>> index 09f4aeef15c7..a31a00fb4974 100644
>>> --- a/drivers/net/phy/phylink.c
>>> +++ b/drivers/net/phy/phylink.c
>>> @@ -433,23 +433,47 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
>>> }
>>> }
>>>
>>> -static void phylink_pcs_config(struct phylink *pl, bool force_restart,
>>> - const struct phylink_link_state *state)
>>> +static void phylink_change_interface(struct phylink *pl, bool restart,
>>> + const struct phylink_link_state *state)
>>> {
>>> - bool restart = force_restart;
>>> + int err;
>>> +
>>> + phylink_dbg(pl, "change interface %s\n", phy_modes(state->interface));
>>>
>>> - if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
>>> - pl->cur_link_an_mode,
>>> - state->interface,
>>> - state->advertising,
>>> - !!(pl->link_config.pause &
>>> - MLO_PAUSE_AN)))
>>> - restart = true;
>>> + if (pl->mac_ops->mac_prepare) {
>>> + err = pl->mac_ops->mac_prepare(pl->config, pl->cur_link_an_mode,
>>> + state->interface);
>>> + if (err < 0) {
>>> + phylink_err(pl, "mac_prepare failed: %pe\n",
>>> + ERR_PTR(err));
>>> + return;
>>> + }
>>> + }
>>>
>>> phylink_mac_config(pl, state);
>>>
>>> + if (pl->pcs_ops) {
>>> + err = pl->pcs_ops->pcs_config(pl->config, pl->cur_link_an_mode,
>>> + state->interface,
>>> + state->advertising,
>>> + !!(pl->link_config.pause &
>>> + MLO_PAUSE_AN));
>>> + if (err < 0)
>>> + phylink_err(pl, "pcs_config failed: %pe\n",
>>> + ERR_PTR(err));
>>> + if (err > 0)
>>> + restart = true;
>>> + }
>>> if (restart)
>>> phylink_mac_pcs_an_restart(pl);
>>> +
>>> + if (pl->mac_ops->mac_finish) {
>>> + err = pl->mac_ops->mac_finish(pl->config, pl->cur_link_an_mode,
>>> + state->interface);
>>> + if (err < 0)
>>> + phylink_err(pl, "mac_prepare failed: %pe\n",
>>> + ERR_PTR(err));
>>> + }
>>> }
>>>
>>> /*
>>> @@ -555,7 +579,7 @@ static void phylink_mac_initial_config(struct phylink *pl, bool force_restart)
>>> link_state.link = false;
>>>
>>> phylink_apply_manual_flow(pl, &link_state);
>>> - phylink_pcs_config(pl, force_restart, &link_state);
>>> + phylink_change_interface(pl, force_restart, &link_state);
>>> }
>>>
>>> static const char *phylink_pause_to_str(int pause)
>>> @@ -674,7 +698,7 @@ static void phylink_resolve(struct work_struct *w)
>>> phylink_link_down(pl);
>>> cur_link_state = false;
>>> }
>>> - phylink_pcs_config(pl, false, &link_state);
>>> + phylink_change_interface(pl, false, &link_state);
>>> pl->link_config.interface = link_state.interface;
>>> } else if (!pl->pcs_ops) {
>>> /* The interface remains unchanged, only the speed,
>>> @@ -1450,22 +1474,26 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>>> return -EINVAL;
>>>
>>> mutex_lock(&pl->state_mutex);
>>> - linkmode_copy(pl->link_config.advertising, config.advertising);
>>> - pl->link_config.interface = config.interface;
>>> pl->link_config.speed = config.speed;
>>> pl->link_config.duplex = config.duplex;
>>> - pl->link_config.an_enabled = kset->base.autoneg !=
>>> - AUTONEG_DISABLE;
>>> -
>>> - if (pl->cur_link_an_mode == MLO_AN_INBAND &&
>>> - !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
>>> - /* If in 802.3z mode, this updates the advertisement.
>>> - *
>>> - * If we are in SGMII mode without a PHY, there is no
>>> - * advertisement; the only thing we have is the pause
>>> - * modes which can only come from a PHY.
>>> - */
>>> - phylink_pcs_config(pl, true, &pl->link_config);
>>> + pl->link_config.an_enabled = kset->base.autoneg != AUTONEG_DISABLE;
>>> +
>>> + if (pl->link_config.interface != config.interface) {
>>
>>
>> I cannot seem to understand where in this function config.interface
>> could become different from pl->link_config.interface.
>>
>> Is there something obvious that I am missing?
>
> The validate() method is free to change the interface if required -
> there's a helper that MACs can use to achieve that for switching
> between 1000base-X and 2500base-X. Useful if you have a FC SFP
> plugged in capable of 2500base-X, but want to communicate with a
> 1000base-X link partner.
>
Ok, I was not aware of this possibility. Now that I looked, it's even
documented in phylink's header file.
My confusion stems from the fact that I expected this to come from the
SFP itself requesting an interface type or the other. So the usage here
is to know what is at the other end of the line and configure the
appropriate advertisement with ethtool?
Ioana
Powered by blists - more mailing lists