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

[Simon Horman <horms@kernel.org>]

On Wed, Feb 25, 2026 at 11:02:17PM +0100, Birger Koblitz wrote:
> Hi Simon,
> 
> Thanks for the thorough analysis!
> 
> On 25/02/2026 8:48 pm, Simon Horman wrote:
> 
> > 
> > > @@ -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;
> > 
> I do not understand what the AI is complaining about, here. The lines in the
> v2 patch are in exactly the order you requested:
> -	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
> +	u16 speed = rtl8152_get_speed(tp);
>  	u16 val;
> gives
> 	__ETHTOOL_DECLARE_LINK_MODE_MASK(common) = {};
> 	u16 speed = rtl8152_get_speed(tp);
>  	u16 val;

Sorry, there was supposed to be some commentary from me included in
my previous email. But somehow it got lost.

I agree the AI is wrong here.

> 
> > (Reference: https://lore.kernel.org/all/20260224170206.146491-3-horms@kernel.org/)
> > 
> > [ ... ]
> > 
> > > @@ -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().
> Same argument as above.

Agreed.

> > [ ... ]
> > 
> > > @@ -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?
> This could make sense, but I tried to make this as minimally intrusive as
> possible and was already at a doubt whether I should even fix the RTL8152
> version of get_eee() to correctly depend on the actual, not the desired
> speed. Then I decided for fixing this incorrect behavior and have
> consistency. The style of the existing code has plenty of duplicate code for
> the different chip-versions and is already difficult to follow to this point
> depending on the chip version. Personally, I find the readability improved
> if the few lines of code are repeated instead of separating this out into
> yet another nested function, and readability also helps code maintenance.
> Plus, it is quite likely that these functions further diverge with e.g.
> support for the RTL8157 (5GBit/s).

Understood. I did think this point was worth raising.
And I appreciate your feedback. We can let this one go for now.

> 
> > 
> > > @@ -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.
> VER 15 supports 2.5GBit links. The existing code enabled 2.5GBit EEE
> unconditionally in rtl_eee_enable():
> 	case RTL_VER_10:
> 	case RTL_VER_11:
> 	case RTL_VER_12:
> 	case RTL_VER_13:
> 	case RTL_VER_15:
> 		if (enable) {
> 			r8156_eee_en(tp, true);
> 			ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
> 		} else {
> 			r8156_eee_en(tp, false);
> 			ocp_reg_write(tp, OCP_EEE_ADV, 0);
> 		}
> 		break;
> Note that previously r8156_eee_en() did not conditionally enable the 2.5GBit
> advertisement depending on tp->eee_adv2, but always enabled it, also for VER
> 15.

Sorry, I missed that when reviewing the AI generated feedback.

> > 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?
> No, see above.

Ack.

It looks like the AI was all false positives this time.
And I'll mark it accordingly in the system. Thanks for looking over it.