| Message ID | 20250204061020.1199124-2-yong.liang.choong@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Enable SGMII and 2500BASEX interface mode switching for Intel platforms | expand |
On Tue, Feb 04, 2025 at 02:10:14PM +0800, Choong Yong Liang wrote: > When the interface mode is SGMII and act_link_an_mode is MLO_AN_INBAND, > switching to the 2500BASE-X interface mode will trigger > phylink_major_config, and act_link_an_mode will be updated to MLO_AN_PHY > in phylink_pcs_neg_mode when the PCS does not support in-band mode. > The MAC and PCS will configure according to the interface mode > and act_link_an_mode. act_link_an_mode must only ever be updated by phylink_major_config() since it defines state for the currently configured mode, and must stay in sync with how the hardware has been programmed at all times. > However, when the interface goes link down and then link up again, the MAC > will attempt to attach the PHY. Why is the MAC trying to disconnect and reconnect the PHY on link changes? Do you really mean "link down" and "link up" as in "connection with the link partner" or do you mean administratively taking the interface down and up (which is a completely different thing.) > The interface mode remains as 2500BASE-X, > but cfg_link_an_mode still holds MLO_AN_INBAND. This causes a failure to > attach the PHY. Hmm. pl->link_interface is the configured setting from firmware etc and doesn't change. pl->cfg_link_an_mode is the configured mode from firmware etc which was passed to phylink_create(), and again doesn't change. So there should be no difference unless something weird is going on, which as you're talking about stmmac, could be the case. More information needed, but as this patch currently stands, I deem it to be incorrect, sorry.
On 4/2/2025 8:04 pm, Russell King (Oracle) wrote: > On Tue, Feb 04, 2025 at 02:10:14PM +0800, Choong Yong Liang wrote: >> When the interface mode is SGMII and act_link_an_mode is MLO_AN_INBAND, >> switching to the 2500BASE-X interface mode will trigger >> phylink_major_config, and act_link_an_mode will be updated to MLO_AN_PHY >> in phylink_pcs_neg_mode when the PCS does not support in-band mode. >> The MAC and PCS will configure according to the interface mode >> and act_link_an_mode. > > act_link_an_mode must only ever be updated by phylink_major_config() > since it defines state for the currently configured mode, and must > stay in sync with how the hardware has been programmed at all times. > >> However, when the interface goes link down and then link up again, the MAC >> will attempt to attach the PHY. > > Why is the MAC trying to disconnect and reconnect the PHY on link > changes? Do you really mean "link down" and "link up" as in "connection > with the link partner" or do you mean administratively taking the > interface down and up (which is a completely different thing.) > The "link down" and "link up" I mention in this part refer to using the command: ifconfig <interface> down/up >> The interface mode remains as 2500BASE-X, >> but cfg_link_an_mode still holds MLO_AN_INBAND. This causes a failure to >> attach the PHY. > > Hmm. > > pl->link_interface is the configured setting from firmware etc and doesn't > change. > > pl->cfg_link_an_mode is the configured mode from firmware etc which was > passed to phylink_create(), and again doesn't change. > > So there should be no difference unless something weird is going on, > which as you're talking about stmmac, could be the case. > Thank you for pointing that out. I think I was confused between pl->link_interface and pl->link_config.interface. The function phylink_attach_phy() uses pl->link_interface, whereas phylink_expects_phy() uses pl->link_config.interface. When the interface switches from SGMII to 2500BASE-X, pl->link_config.interface is updated by phylink_major_config(). So, when the STMMAC link goes down and comes up again, it is blocked by phylink_expects_phy(). At this point, pl->cfg_link_an_mode is MLO_AN_INBAND and pl->link_config.interface is 2500BASE-X. Since phylink_expects_phy() is trying to achieve the same checking condition as phylink_attach_phy(), it should use pl->link_interface for the check as well. Does that make sense to you? > More information needed, but as this patch currently stands, I deem it > to be incorrect, sorry. >
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 214b62fba991..b0f9725e0494 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1903,6 +1903,7 @@ int phylink_set_fixed_link(struct phylink *pl, pl->cfg_link_an_mode = MLO_AN_FIXED; pl->req_link_an_mode = pl->cfg_link_an_mode; + pl->act_link_an_mode = pl->cfg_link_an_mode; return 0; } @@ -2002,6 +2003,7 @@ struct phylink *phylink_create(struct phylink_config *config, } pl->req_link_an_mode = pl->cfg_link_an_mode; + pl->act_link_an_mode = pl->cfg_link_an_mode; ret = phylink_register_sfp(pl, fwnode); if (ret < 0) { @@ -2044,8 +2046,8 @@ EXPORT_SYMBOL_GPL(phylink_destroy); */ bool phylink_expects_phy(struct phylink *pl) { - if (pl->cfg_link_an_mode == MLO_AN_FIXED || - (pl->cfg_link_an_mode == MLO_AN_INBAND && + if (pl->act_link_an_mode == MLO_AN_FIXED || + (pl->act_link_an_mode == MLO_AN_INBAND && phy_interface_mode_is_8023z(pl->link_config.interface))) return false; return true; @@ -2280,8 +2282,8 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy, { u32 flags = 0; - if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED || - (pl->cfg_link_an_mode == MLO_AN_INBAND && + if (WARN_ON(pl->act_link_an_mode == MLO_AN_FIXED || + (pl->act_link_an_mode == MLO_AN_INBAND && phy_interface_mode_is_8023z(interface) && !pl->sfp_bus))) return -EINVAL;
When the interface mode is SGMII and act_link_an_mode is MLO_AN_INBAND, switching to the 2500BASE-X interface mode will trigger phylink_major_config, and act_link_an_mode will be updated to MLO_AN_PHY in phylink_pcs_neg_mode when the PCS does not support in-band mode. The MAC and PCS will configure according to the interface mode and act_link_an_mode. However, when the interface goes link down and then link up again, the MAC will attempt to attach the PHY. The interface mode remains as 2500BASE-X, but cfg_link_an_mode still holds MLO_AN_INBAND. This causes a failure to attach the PHY. This patch uses act_link_an_mode to determine if there is a PHY to attach. Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> --- drivers/net/phy/phylink.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)