| Message ID | 10510abd-ac26-42d0-8222-8b01fe9b8059@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bnxt: convert EEE handling to use linkmode bitmaps | expand |
> - if (!edata->advertised_u32) { > - edata->advertised_u32 = advertising & eee->supported_u32; > - } else if (edata->advertised_u32 & ~advertising) { > - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", > - edata->advertised_u32, advertising); That warning text looks wrong. I think it should be EEE advertised %x must be a subset of autoneg supported speeds %x and it should print eee->supported, not advertising. Lets leave that to Broadcom to fix. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 03.02.2024 22:59, Andrew Lunn wrote: >> - if (!edata->advertised_u32) { >> - edata->advertised_u32 = advertising & eee->supported_u32; >> - } else if (edata->advertised_u32 & ~advertising) { >> - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", >> - edata->advertised_u32, advertising); > > That warning text looks wrong. I think it should be > > EEE advertised %x must be a subset of autoneg supported speeds %x > > and it should print eee->supported, not advertising. > > Lets leave that to Broadcom to fix. > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > I just saw that I overlooked needed conversions, will send a v2. > Andrew Heiner
On Sat, Feb 3, 2024 at 1:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > - if (!edata->advertised_u32) { > > - edata->advertised_u32 = advertising & eee->supported_u32; > > - } else if (edata->advertised_u32 & ~advertising) { > > - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", > > - edata->advertised_u32, advertising); > > That warning text looks wrong. I think it should be > > EEE advertised %x must be a subset of autoneg supported speeds %x > > and it should print eee->supported, not advertising. > I think it is correct. EEE advertised must be a subset of the advertised speed. Let's say we are only advertising 1G for speed, then EEE must not advertise more than 1G. Advertising for more than 1G doesn't make sense anyway because it will only link up at 1G.
On Sat, Feb 03, 2024 at 04:16:51PM -0800, Michael Chan wrote: > On Sat, Feb 3, 2024 at 1:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > - if (!edata->advertised_u32) { > > > - edata->advertised_u32 = advertising & eee->supported_u32; > > > - } else if (edata->advertised_u32 & ~advertising) { > > > - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", > > > - edata->advertised_u32, advertising); > > > > That warning text looks wrong. I think it should be > > > > EEE advertised %x must be a subset of autoneg supported speeds %x > > > > and it should print eee->supported, not advertising. > > > I think it is correct. EEE advertised must be a subset of the > advertised speed. I don't think that is true. At least, i've not seen any other MAC/PHY driver change EEE advertising when they change the general advertising. What i expect is that the PHY and link partner first resolve the general link speed. They then look at what is being advertised in terms of EEE and decide if EEE is being advertised by both ends at the resolved speed. So it does not matter if the link speed is resolved at 1G, but both ends advertise both 40G and 1G for EEE. The 40G is simply ignored because they have resolved to 1G. What does however matter is that EEE supported lists only 1G, but user space ask for both 1G and 40G to be advertised. I would expect the driver to mask the requested advertised with support, see that unsupported link modes are being asked for and return -EINVAL. Andrew
On Sat, Feb 03, 2024 at 04:16:51PM -0800, Michael Chan wrote: > On Sat, Feb 3, 2024 at 1:59 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > - if (!edata->advertised_u32) { > > > - edata->advertised_u32 = advertising & eee->supported_u32; > > > - } else if (edata->advertised_u32 & ~advertising) { > > > - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", > > > - edata->advertised_u32, advertising); > > > > That warning text looks wrong. I think it should be > > > > EEE advertised %x must be a subset of autoneg supported speeds %x > > > > and it should print eee->supported, not advertising. > > > I think it is correct. EEE advertised must be a subset of the > advertised speed. Where is that a requirement? If a PHY supports e.g. 1G, 100M, and supports EEE at those two speeds, but is only advertising 1G, then the only speed that could be negotiated is 1G. The EEE negotiation will also occur, and if the link partner also advertises EEE at 1G and 100M, the result of that negotiation is that EEE _can_ _be_ _used_ at 1G and 100M speeds. However, the PHY has negotiated 1G speed, so it now checks to see whether the EEE negotiation included 1G speed. The fact that the EEE negotiation also includes 100M is irrelevant - the negotiated speed is 1G, and that's all that determines whether EEE will be used for the negotiated link speed. The exception is if the PHY is buggy and doesn't follow this, e.g. assuming that if any EEE speed was negotiated that EEE is supported, but that would be a recipe for disaster given that a link partner may only advertise EEE at 100M, we may be advertising 100M and 1G (both for EEE and link) so we end up using 1G with EEE despite the link partner not supporting it. So, whatever way I look at it, the statement "the EEE advertisement must be a subset of the link advertisement" makes absolutely no sense to me, and is probably wrong.
On Sat, Feb 3, 2024 at 8:46 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Sat, Feb 03, 2024 at 04:16:51PM -0800, Michael Chan wrote: > > I think it is correct. EEE advertised must be a subset of the > > advertised speed. > > I don't think that is true. At least, i've not seen any other MAC/PHY > driver change EEE advertising when they change the general > advertising. What i expect is that the PHY and link partner first > resolve the general link speed. They then look at what is being > advertised in terms of EEE and decide if EEE is being advertised by > both ends at the resolved speed. So it does not matter if the link > speed is resolved at 1G, but both ends advertise both 40G and 1G for > EEE. The 40G is simply ignored because they have resolved to 1G. I need to check with the FW team to see if it's implemented the way you described. If it accepts EEE advertisement bits that are not set for speed advertisements, then we can make the change. Thanks. > What does however matter is that EEE supported lists only 1G, but user > space ask for both 1G and 40G to be advertised. I would expect the > driver to mask the requested advertised with support, see that > unsupported link modes are being asked for and return -EINVAL. >
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index fde32b32f..adcddfb97 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -10624,7 +10624,7 @@ static int bnxt_hwrm_phy_qcaps(struct bnxt *bp) struct ethtool_keee *eee = &bp->eee; u16 fw_speeds = le16_to_cpu(resp->supported_speeds_eee_mode); - eee->supported_u32 = _bnxt_fw_to_ethtool_adv_spds(fw_speeds, 0); + _bnxt_fw_to_linkmode(eee->supported, fw_speeds); bp->lpi_tmr_lo = le32_to_cpu(resp->tx_lpi_timer_low) & PORT_PHY_QCAPS_RESP_TX_LPI_TIMER_LOW_MASK; bp->lpi_tmr_hi = le32_to_cpu(resp->valid_tx_lpi_timer_high) & @@ -10775,8 +10775,7 @@ int bnxt_update_link(struct bnxt *bp, bool chng_link_state) eee->eee_active = 1; fw_speeds = le16_to_cpu( resp->link_partner_adv_eee_link_speed_mask); - eee->lp_advertised_u32 = - _bnxt_fw_to_ethtool_adv_spds(fw_speeds, 0); + _bnxt_fw_to_linkmode(eee->lp_advertised, fw_speeds); } /* Pull initial EEE config */ @@ -10786,8 +10785,7 @@ int bnxt_update_link(struct bnxt *bp, bool chng_link_state) eee->eee_enabled = 1; fw_speeds = le16_to_cpu(resp->adv_eee_link_speed_mask); - eee->advertised_u32 = - _bnxt_fw_to_ethtool_adv_spds(fw_speeds, 0); + _bnxt_fw_to_linkmode(eee->advertised, fw_speeds); if (resp->eee_config_phy_addr & PORT_PHY_QCFG_RESP_EEE_CONFIG_EEE_TX_LPI) { @@ -11329,15 +11327,18 @@ static bool bnxt_eee_config_ok(struct bnxt *bp) return true; if (eee->eee_enabled) { - u32 advertising = - _bnxt_fw_to_ethtool_adv_spds(link_info->advertising, 0); + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); + __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp); + + _bnxt_fw_to_linkmode(advertising, link_info->advertising); if (!(link_info->autoneg & BNXT_AUTONEG_SPEED)) { eee->eee_enabled = 0; return false; } - if (eee->advertised_u32 & ~advertising) { - eee->advertised_u32 = advertising & eee->supported_u32; + if (linkmode_andnot(tmp, eee->advertised, advertising)) { + linkmode_and(eee->advertised, advertising, + eee->supported); return false; } } diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index 481b835a7..d1b087b90 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1751,31 +1751,21 @@ static int bnxt_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) return 0; } -u32 _bnxt_fw_to_ethtool_adv_spds(u16 fw_speeds, u8 fw_pause) +/* TODO: support 25GB, 40GB, 50GB with different cable type */ +void _bnxt_fw_to_linkmode(unsigned long *mode, u16 fw_speeds) { - u32 speed_mask = 0; + linkmode_zero(mode); - /* TODO: support 25GB, 40GB, 50GB with different cable type */ - /* set the advertised speeds */ if (fw_speeds & BNXT_LINK_SPEED_MSK_100MB) - speed_mask |= ADVERTISED_100baseT_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, mode); if (fw_speeds & BNXT_LINK_SPEED_MSK_1GB) - speed_mask |= ADVERTISED_1000baseT_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, mode); if (fw_speeds & BNXT_LINK_SPEED_MSK_2_5GB) - speed_mask |= ADVERTISED_2500baseX_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseX_Full_BIT, mode); if (fw_speeds & BNXT_LINK_SPEED_MSK_10GB) - speed_mask |= ADVERTISED_10000baseT_Full; + linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, mode); if (fw_speeds & BNXT_LINK_SPEED_MSK_40GB) - speed_mask |= ADVERTISED_40000baseCR4_Full; - - if ((fw_pause & BNXT_LINK_PAUSE_BOTH) == BNXT_LINK_PAUSE_BOTH) - speed_mask |= ADVERTISED_Pause; - else if (fw_pause & BNXT_LINK_PAUSE_TX) - speed_mask |= ADVERTISED_Asym_Pause; - else if (fw_pause & BNXT_LINK_PAUSE_RX) - speed_mask |= ADVERTISED_Pause | ADVERTISED_Asym_Pause; - - return speed_mask; + linkmode_set_bit(ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, mode); } enum bnxt_media_type { @@ -3886,10 +3876,11 @@ static int bnxt_set_eeprom(struct net_device *dev, static int bnxt_set_eee(struct net_device *dev, struct ethtool_keee *edata) { + __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); + __ETHTOOL_DECLARE_LINK_MODE_MASK(tmp); struct bnxt *bp = netdev_priv(dev); struct ethtool_keee *eee = &bp->eee; struct bnxt_link_info *link_info = &bp->link_info; - u32 advertising; int rc = 0; if (!BNXT_PHY_CFG_ABLE(bp)) @@ -3899,7 +3890,7 @@ static int bnxt_set_eee(struct net_device *dev, struct ethtool_keee *edata) return -EOPNOTSUPP; mutex_lock(&bp->link_lock); - advertising = _bnxt_fw_to_ethtool_adv_spds(link_info->advertising, 0); + _bnxt_fw_to_linkmode(advertising, link_info->advertising); if (!edata->eee_enabled) goto eee_ok; @@ -3919,16 +3910,15 @@ static int bnxt_set_eee(struct net_device *dev, struct ethtool_keee *edata) edata->tx_lpi_timer = eee->tx_lpi_timer; } } - if (!edata->advertised_u32) { - edata->advertised_u32 = advertising & eee->supported_u32; - } else if (edata->advertised_u32 & ~advertising) { - netdev_warn(dev, "EEE advertised %x must be a subset of autoneg advertised speeds %x\n", - edata->advertised_u32, advertising); + if (linkmode_empty(edata->advertised)) { + linkmode_and(edata->advertised, advertising, eee->supported); + } else if (linkmode_andnot(tmp, edata->advertised, advertising)) { + netdev_warn(dev, "EEE advertised must be a subset of autoneg advertised speeds\n"); rc = -EINVAL; goto eee_exit; } - eee->advertised_u32 = edata->advertised_u32; + linkmode_copy(eee->advertised, edata->advertised); eee->tx_lpi_enabled = edata->tx_lpi_enabled; eee->tx_lpi_timer = edata->tx_lpi_timer; eee_ok: diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h index a8ecef8ab..694a5861c 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.h @@ -46,7 +46,7 @@ struct bnxt_led_cfg { extern const struct ethtool_ops bnxt_ethtool_ops; u32 bnxt_get_rxfh_indir_size(struct net_device *dev); -u32 _bnxt_fw_to_ethtool_adv_spds(u16, u8); +void _bnxt_fw_to_linkmode(unsigned long *mode, u16 fw_speeds); u32 bnxt_fw_to_ethtool_speed(u16); u16 bnxt_get_fw_auto_link_speeds(u32); int bnxt_hwrm_nvm_get_dev_info(struct bnxt *bp,
Convert EEE handling to use linkmode bitmaps. This prepares for removing the legacy bitmaps from struct ethtool_keee. No functional change intended. When replacing _bnxt_fw_to_ethtool_adv_spds() with _bnxt_fw_to_linkmode(), remove the fw_pause argument because it's always passed as 0. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt.c | 19 +++++---- .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 42 +++++++------------ .../net/ethernet/broadcom/bnxt/bnxt_ethtool.h | 2 +- 3 files changed, 27 insertions(+), 36 deletions(-)