| Message ID | E1jqHG3-0006PX-UZ@rmk-PC.armlinux.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Phylink PCS updates | expand |
On 6/30/20 5:29 PM, Russell King wrote: > When we have a PHY attached, an ethtool ksettings_set() call only > really needs to call through to the phylib equivalent; phylib will > call back to us when the link changes so we can update our state. > Therefore, we can bypass most of our ksettings_set() call for this > case. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > drivers/net/phy/phylink.c | 104 +++++++++++++++++--------------------- > 1 file changed, 47 insertions(+), 57 deletions(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 103d2a550415..967c068d16c8 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -1312,13 +1312,33 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, > const struct ethtool_link_ksettings *kset) > { > __ETHTOOL_DECLARE_LINK_MODE_MASK(support); > - struct ethtool_link_ksettings our_kset; > struct phylink_link_state config; > const struct phy_setting *s; > - int ret; > > ASSERT_RTNL(); > > + if (pl->phydev) { > + /* We can rely on phylib for this update; we also do not need > + * to update the pl->link_config settings: > + * - the configuration returned via ksettings_get() will come > + * from phylib whenever a PHY is present. > + * - link_config.interface will be updated by the PHY calling > + * back via phylink_phy_change() and a subsequent resolve. > + * - initial link configuration for PHY mode comes from the > + * last phy state updated via phylink_phy_change(). > + * - other configuration changes (e.g. pause modes) are > + * performed directly via phylib. > + * - if in in-band mode with a PHY, the link configuration > + * is passed on the link from the PHY, and all of > + * link_config.{speed,duplex,an_enabled,pause} are not used. > + * - the only possible use would be link_config.advertising > + * pause modes when in 1000base-X mode with a PHY, but in > + * the presence of a PHY, this should not be changed as that > + * should be determined from the media side advertisement. > + */ > + return phy_ethtool_ksettings_set(pl->phydev, kset); > + } > + Also tested the PHY use case, no issue encountered with changing the advertisements, autoneg etc > linkmode_copy(support, pl->supported); > config = pl->link_config; > config.an_enabled = kset->base.autoneg == AUTONEG_ENABLE; > @@ -1365,65 +1385,35 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, > return -EINVAL; > } > > - if (pl->phydev) { > - /* If we have a PHY, we process the kset change via phylib. > - * phylib will call our link state function if the PHY > - * parameters have changed, which will trigger a resolve > - * and update the MAC configuration. > - */ > - our_kset = *kset; > - linkmode_copy(our_kset.link_modes.advertising, > - config.advertising); > - our_kset.base.speed = config.speed; > - our_kset.base.duplex = config.duplex; > + /* For a fixed link, this isn't able to change any parameters, > + * which just leaves inband mode. > + */ > + if (phylink_validate(pl, support, &config)) > + return -EINVAL; > > - ret = phy_ethtool_ksettings_set(pl->phydev, &our_kset); > - if (ret) > - return ret; > + /* If autonegotiation is enabled, we must have an advertisement */ > + if (config.an_enabled && phylink_is_empty_linkmode(config.advertising)) > + return -EINVAL; > > - mutex_lock(&pl->state_mutex); > - /* Save the new configuration */ > - linkmode_copy(pl->link_config.advertising, > - our_kset.link_modes.advertising); > - pl->link_config.interface = config.interface; > - pl->link_config.speed = our_kset.base.speed; > - pl->link_config.duplex = our_kset.base.duplex; > - pl->link_config.an_enabled = our_kset.base.autoneg != > - AUTONEG_DISABLE; > - mutex_unlock(&pl->state_mutex); > - } else { > - /* For a fixed link, this isn't able to change any parameters, > - * which just leaves inband mode. > + 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; Is there a specific reason why this is not just using config.an_enabled to sync back to pl->link_config? > + > + 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. > */ > - if (phylink_validate(pl, support, &config)) > - return -EINVAL; > - > - /* If autonegotiation is enabled, we must have an advertisement */ > - if (config.an_enabled && > - phylink_is_empty_linkmode(config.advertising)) > - 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); > - } > - mutex_unlock(&pl->state_mutex); > + phylink_pcs_config(pl, true, &pl->link_config); > } > + mutex_unlock(&pl->state_mutex); > > return 0; > } >
On Mon, Jul 20, 2020 at 10:44:11AM +0000, Ioana Ciornei wrote: > On 6/30/20 5:29 PM, Russell King wrote: > > When we have a PHY attached, an ethtool ksettings_set() call only > > really needs to call through to the phylib equivalent; phylib will > > call back to us when the link changes so we can update our state. > > Therefore, we can bypass most of our ksettings_set() call for this > > case. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > > + 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; > > Is there a specific reason why this is not just using config.an_enabled > to sync back to pl->link_config? Due to the history of the code; and changing it in this patch would be a distraction and actually a separate change. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 103d2a550415..967c068d16c8 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1312,13 +1312,33 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, const struct ethtool_link_ksettings *kset) { __ETHTOOL_DECLARE_LINK_MODE_MASK(support); - struct ethtool_link_ksettings our_kset; struct phylink_link_state config; const struct phy_setting *s; - int ret; ASSERT_RTNL(); + if (pl->phydev) { + /* We can rely on phylib for this update; we also do not need + * to update the pl->link_config settings: + * - the configuration returned via ksettings_get() will come + * from phylib whenever a PHY is present. + * - link_config.interface will be updated by the PHY calling + * back via phylink_phy_change() and a subsequent resolve. + * - initial link configuration for PHY mode comes from the + * last phy state updated via phylink_phy_change(). + * - other configuration changes (e.g. pause modes) are + * performed directly via phylib. + * - if in in-band mode with a PHY, the link configuration + * is passed on the link from the PHY, and all of + * link_config.{speed,duplex,an_enabled,pause} are not used. + * - the only possible use would be link_config.advertising + * pause modes when in 1000base-X mode with a PHY, but in + * the presence of a PHY, this should not be changed as that + * should be determined from the media side advertisement. + */ + return phy_ethtool_ksettings_set(pl->phydev, kset); + } + linkmode_copy(support, pl->supported); config = pl->link_config; config.an_enabled = kset->base.autoneg == AUTONEG_ENABLE; @@ -1365,65 +1385,35 @@ int phylink_ethtool_ksettings_set(struct phylink *pl, return -EINVAL; } - if (pl->phydev) { - /* If we have a PHY, we process the kset change via phylib. - * phylib will call our link state function if the PHY - * parameters have changed, which will trigger a resolve - * and update the MAC configuration. - */ - our_kset = *kset; - linkmode_copy(our_kset.link_modes.advertising, - config.advertising); - our_kset.base.speed = config.speed; - our_kset.base.duplex = config.duplex; + /* For a fixed link, this isn't able to change any parameters, + * which just leaves inband mode. + */ + if (phylink_validate(pl, support, &config)) + return -EINVAL; - ret = phy_ethtool_ksettings_set(pl->phydev, &our_kset); - if (ret) - return ret; + /* If autonegotiation is enabled, we must have an advertisement */ + if (config.an_enabled && phylink_is_empty_linkmode(config.advertising)) + return -EINVAL; - mutex_lock(&pl->state_mutex); - /* Save the new configuration */ - linkmode_copy(pl->link_config.advertising, - our_kset.link_modes.advertising); - pl->link_config.interface = config.interface; - pl->link_config.speed = our_kset.base.speed; - pl->link_config.duplex = our_kset.base.duplex; - pl->link_config.an_enabled = our_kset.base.autoneg != - AUTONEG_DISABLE; - mutex_unlock(&pl->state_mutex); - } else { - /* For a fixed link, this isn't able to change any parameters, - * which just leaves inband mode. + 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. */ - if (phylink_validate(pl, support, &config)) - return -EINVAL; - - /* If autonegotiation is enabled, we must have an advertisement */ - if (config.an_enabled && - phylink_is_empty_linkmode(config.advertising)) - 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); - } - mutex_unlock(&pl->state_mutex); + phylink_pcs_config(pl, true, &pl->link_config); } + mutex_unlock(&pl->state_mutex); return 0; }
When we have a PHY attached, an ethtool ksettings_set() call only really needs to call through to the phylib equivalent; phylib will call back to us when the link changes so we can update our state. Therefore, we can bypass most of our ksettings_set() call for this case. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 104 +++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 57 deletions(-)