Re: [PATCH net-next v4 1/2] r8152: Add support for 5Gbit Link Speeds and EEE

[Birger Koblitz]

Hi Jakub,

thanks a lot for reviewing the patch!
On 3/29/26 21:34, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> r8152: Add support for 5Gbit Link Speeds and EEE
>
> This commit adds support for 5GBit link speeds and EEE to the RTL8157
> driver. It updates the speed and duplex settings, ethtool integration,
> and EEE advertisement parameters to handle the new 5G capabilities.
>
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 3b6d4252d34c..bab8e10e5f99 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -604,6 +604,7 @@ enum spd_duplex {
> >   	FORCE_100M_FULL,
> >   	FORCE_1000M_FULL,
> >   	NWAY_2500M_FULL,
> > +	NWAY_5000M_FULL,
> >   };
>
> Are there missing switch cases for NWAY_5000M_FULL in the hardware UPS
> flag configurations, such as r8156_ups_flags?
This question has now been raised the third time, see here:
https://lkml.org/lkml/2026/3/24/1902 and here https://lkml.org/lkml/2026/3/19/1765
However, while I share the concern, I cannot provide a better answer than
the first time or second time.

> Is it intentional that the MAC idle speed down feature (IDLE_SPDWN_EN) is
> disabled for _2500bps but remains enabled for _5000bps?
Yes, this is intentional. IDLE_SPDWN_EN is indeed only disabled for _2500bps
in the out-of-tree driver (and not for 5GBit or 10GBit).

> > @@ -6343,15 +6356,20 @@ static int rtl8152_set_speed(struct r8152 *tp, u8 autoneg, u32 speed, u8 duplex,
> >   				r8152_mdio_write(tp, MII_CTRL1000, new1);
> >   		}
> >   
> > -		if (tp->support_2500full) {
> > +		if (tp->support_2500full || tp->support_5000full) {
> >   			orig = ocp_reg_read(tp, OCP_10GBT_CTRL);
> > -			new1 = orig & ~MDIO_AN_10GBT_CTRL_ADV2_5G;
> > +			new1 = orig & ~(MDIO_AN_10GBT_CTRL_ADV2_5G | MDIO_AN_10GBT_CTRL_ADV5G);
> >   
> >   			if (advertising & RTL_ADVERTISED_2500_FULL) {
> >   				new1 |= MDIO_AN_10GBT_CTRL_ADV2_5G;
> >   				tp->ups_info.speed_duplex = NWAY_2500M_FULL;
> >   			}
> >   
> > +			if (advertising & RTL_ADVERTISED_5000_FULL) {
> > +				new1 |= MDIO_AN_10GBT_CTRL_ADV5G;
> > +				tp->ups_info.speed_duplex = NWAY_5000M_FULL;
> > +			}
> > +
>
> Could this unintentionally advertise 5GbE on a 2.5GbE-only adapter if
> userspace requests it?
>
> The earlier check in this function:
>
> 		if (!(advertising & support))
> 			return -EINVAL;
>
> only ensures that at least one supported speed is requested, but it does
> not strip unsupported speeds from the advertising mask. Since this block
> is now entered when support_2500full is true, checking the advertising mask
> for RTL_ADVERTISED_5000_FULL without verifying tp->support_5000full could
> erroneously write the 5G bit to the PHY and misconfigure the hardware
> state.

I will change this to:

        advertising &= support;
        if (!advertising)
                return -EINVAL;

in v5, to be on the safe side.

> [ ... ]
>
> > @@ -8793,6 +8814,19 @@ int rtl8152_get_link_ksettings(struct net_device *netdev,
> >   			cmd->base.speed = SPEED_2500;
> >   	}
> >   
> > +	if (tp->support_5000full) {
> > +		linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > +				 cmd->link_modes.advertising,
> > +				 ocp_reg_read(tp, OCP_10GBT_CTRL) & MDIO_AN_10GBT_CTRL_ADV5G);
> > +
> > +		linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
> > +				 cmd->link_modes.lp_advertising,
> > +				 ocp_reg_read(tp, OCP_10GBT_STAT) & MDIO_AN_10GBT_STAT_LP5G);
> > +
> > +		if (is_speed_5000(rtl8152_get_speed(tp)))
> > +			cmd->base.speed = SPEED_5000;
> > +	}
>
> Since 5G adapters like the RTL8157 will have both support_2500full and
> support_5000full set, will this result in duplicate blocking USB control
> transfers?
You are right. Also in view of future 10GBit support, I will change this to:
        if (tp->support_2500full || tp->support_5000full) {
                u16 ocp_10bt_ctrl = ocp_reg_read(tp, OCP_10GBT_CTR)L;
                u16 ocp_10bt_stat = ocp_reg_read(tp, OCP_10GBT_STAT);

                if (tp->support_2500full) {
                        ...
                }
                if (tp->support_5000full) {
                        ...
                }
        }

> In r8153_get_eee, the reading of OCP_EEE_ADV2 and OCP_EEE_LPABLE2 is
> performed inside the if (tp->support_2500full) block that precedes this new
> code.
>
> Since those registers contain the EEE capability bits for both 2.5G
> and 5G speeds, what happens if a device has support_5000full enabled but
> support_2500full disabled? It looks like the registers would never be read,
> and the 5Gbps EEE capabilities would fail to populate correctly.
The driver supports RTL815x-chips, and all RTL815x-chips, which support 5GBit also
support 2.5gbit (the RTL8157 and the RTL8159). My understanding is, that the
802.3bz standard even requires this fallback. However, to be on the safe side and
to also support possible future chips, I will change this to:
	if (tp->support_2500full || tp->support_5000full) {
		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 (tp->support_2500full) {
		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, eee->supported);

		if (speed & _2500bps)
			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
	}

	if (tp->support_5000full) {
		linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, eee->supported);

		if (speed & _5000bps)
			linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common);
	}


Cheers,
  Birger