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

[Jakub Kicinski <kuba@kernel.org>]

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? 

It looks like if a 5Gbps link is established, it might fall through to the
default case and leave the speed bits as 0. Since ups_flags_speed(0) maps
to FORCE_10M_HALF, this could cause the device's clock scaling and power
management features to be misconfigured for 10M Half-Duplex instead of the
5Gbps link.

[ ... ]

> @@ -725,6 +726,7 @@ enum spd_duplex {
>  #define BP4_SUPER_ONLY		0x1578	/* RTL_VER_04 only */
>  
>  enum rtl_register_content {
> +	_5000bps	= BIT(12),
>  	_2500bps	= BIT(10),

Is it intentional that the MAC idle speed down feature (IDLE_SPDWN_EN) is
disabled for _2500bps but remains enabled for _5000bps? 

In functions like rtl8156_enable, the code checks if the speed is _2500bps
to disable the idle speed down feature, but the new _5000bps constant is
not included in those checks. This might cause latency spikes or instability
at 5Gbps if the power-saving feature is enabled.

[ ... ]

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

[ ... ]

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

The ocp_reg_read calls for OCP_10GBT_CTRL and OCP_10GBT_STAT, along with
rtl8152_get_speed, are also executed in the preceding support_2500full
block. Reading these into local variables before the capability checks
would avoid the overhead of duplicate hardware accesses.

[ ... ]

> @@ -8994,6 +9032,13 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  			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);
> +	}

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.
-- 
pw-bot: cr