Re: [net-next,v2] r8152: Add 2500baseT EEE status/configuration support

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
r8152: Add 2500baseT EEE status/configuration support

The r8152 driver supports the RTL8156, which is a 2.5Gbit Ethernet
controller for USB 3.0, for which support is added for configuring and
displaying the EEE advertisement status for 2.5GBit connections. The
patch also corrects the determination of whether EEE is active to include
the 2.5GBit connection status and make the determination dependent not on
the desired speed configuration (tp->speed), but on the actual speed used
by the controller.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 8d36162f36df..3df1a1f4a350 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8922,7 +8924,8 @@ static void rtl8152_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>
>  static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
> +	u16 speed = rtl8152_get_speed(tp);
>  	u16 val;
>
>  	val = r8152_mmd_read(tp, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);

This isn't a bug, but the variable declarations are not in reverse
Christmas tree order. Simon Horman's AI review raised this in v1, you
acknowledged it and said it would be fixed in v2, but the ordering issue
remains. The longest line should come first, followed by shorter lines.
Should be:

	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
	u16 speed = rtl8152_get_speed(tp);
	u16 val;

(Reference: https://lore.kernel.org/all/20260224170206.146491-3-horms@xxxxxxxxxx/)

[ ... ]

> @@ -8956,7 +8968,8 @@ static int r8152_set_eee(struct r8152 *tp, struct ethtool_keee *eee)
>
>  static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
> +	u16 speed = rtl8152_get_speed(tp);
>  	u16 val;
>
>  	val = ocp_reg_read(tp, OCP_EEE_ABLE);

Same reverse Christmas tree order issue here. The same pattern should be
applied as in r8152_get_eee().

[ ... ]

> @@ -8968,10 +8981,29 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  	val = ocp_reg_read(tp, OCP_EEE_LPABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->lp_advertised, val);
>
> +	if (tp->support_2500full) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, eee->supported);
> +
> +		val = ocp_reg_read(tp, OCP_EEE_ADV2);
> +		mii_eee_cap2_mod_linkmode_adv_t(eee->advertised, val);
> +
> +		val = ocp_reg_read(tp, OCP_EEE_LPABLE2);
> +		mii_eee_cap2_mod_linkmode_adv_t(eee->lp_advertised, val);
> +
> +		if (speed & _2500bps)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
> +	}
> +
>  	eee->eee_enabled = tp->eee_en;
>
> -	linkmode_and(common, eee->advertised, eee->lp_advertised);
> -	eee->eee_active = phy_check_valid(tp->speed, tp->duplex, common);
> +	if (speed & _1000bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, common);
> +	if (speed & _100bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, common);
> +
> +	linkmode_and(common, common, eee->advertised);
> +	linkmode_and(common, common, eee->lp_advertised);
> +	eee->eee_active = !linkmode_empty(common);
>
>  	return 0;
>  }

The eee_active determination logic is now duplicated between
r8152_get_eee() and r8153_get_eee(). Both functions convert speed
register bits to linkmode bits, AND with advertised and lp_advertised,
and test emptiness in the same way. Would it make sense to factor this
into a shared helper to reduce maintenance burden?

> @@ -9512,6 +9544,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  	case RTL_VER_11:
>  		tp->eee_en		= true;
>  		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
> +		tp->eee_adv2		= MDIO_EEE_2_5GT;
>  		fallthrough;
>  	case RTL_VER_10:
>  		ops->init		= r8156_init;

[ ... ]

> @@ -9537,6 +9570,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  	case RTL_VER_15:
>  		tp->eee_en		= true;
>  		tp->eee_adv		= MDIO_EEE_1000T | MDIO_EEE_100TX;
> +		tp->eee_adv2		= MDIO_EEE_2_5GT;
>  		ops->init		= r8156b_init;
>  		ops->enable		= rtl8156b_enable;
>  		ops->disable		= rtl8153_disable;

Does RTL_VER_15 support 2.5G links? Looking at the code, only RTL_VER_12
and RTL_VER_13 set tp->support_2500full before falling through, but
RTL_VER_15 does not. With this change, tp->eee_adv2 = MDIO_EEE_2_5GT is
set for VER_15.

When r8156_eee_en() is called for a VER_15 device with EEE enabled:

static void r8156_eee_en(struct r8152 *tp, bool enable)
{
	u16 config;

	config = ocp_reg_read(tp, OCP_EEE_ADV2);

	if (enable && (tp->eee_adv2 & MDIO_EEE_2_5GT))
		config |= MDIO_EEE_2_5GT;

The condition (tp->eee_adv2 & MDIO_EEE_2_5GT) will be true, causing
MDIO_EEE_2_5GT to be written into the OCP_EEE_ADV2 hardware register.
This would advertise 2.5G EEE capability to the link partner on a device
that does not support 2.5Gbit links.

Should the eee_adv2 initialization be moved into the RTL_VER_12/13 block
(before the fallthrough), or should RTL_VER_15 be given eee_adv2 = 0 to
remain consistent with its lack of support_2500full?