[PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Heiner Kallweit]

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(-)

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,
-- 
2.43.0

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Andrew Lunn]

> -	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

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Heiner Kallweit]

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

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Andrew Lunn]

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

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] bnxt: convert EEE handling to use linkmode bitmaps

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

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.
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]