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

[Paolo Abeni]

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 support for 5Gbit Link Speeds and EEE

The RTL8157 supports 5GBit Link speeds. Add support for this speed
in the setup and setting/getting through ethtool. Also add 5GBit EEE.
Add functionality for setup and ethtool get/set methods.

> 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

[ ... ]

> @@ -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;
> +		}
> +
>  		if (orig != new1)
>  			ocp_reg_write(tp, OCP_10GBT_CTRL, new1);
>  	}

Does r8156_ups_flags() correctly handle the new NWAY_5000M_FULL value?

When 5000M is advertised, rtl8152_set_speed() stores NWAY_5000M_FULL
(enum value 11) in tp->ups_info.speed_duplex. However, r8156_ups_flags()
has a switch statement that only handles values up to NWAY_2500M_FULL
(value 10):

drivers/net/usb/r8152.c:r8156_ups_flags() {
	switch (tp->ups_info.speed_duplex) {
	case NWAY_10M_HALF:
		return ups_flags_speed(UPS_FLAGS_SPEED_10);
	case NWAY_10M_FULL:
		return ups_flags_speed(UPS_FLAGS_SPEED_10);
	case NWAY_100M_HALF:
		return ups_flags_speed(UPS_FLAGS_SPEED_100);
	case NWAY_100M_FULL:
		return ups_flags_speed(UPS_FLAGS_SPEED_100);
	case NWAY_1000M_FULL:
		return ups_flags_speed(UPS_FLAGS_SPEED_1000);
	case NWAY_2500M_FULL:
		return ups_flags_speed(UPS_FLAGS_SPEED_2500);
	default:
		break;
	}
}

The new NWAY_5000M_FULL falls through to 'default: break;', which means
ups_flags_speed() is never called. This results in incorrect UPS flags
(speed bits = 0) being written to the USB_UPS_FLAGS hardware register via
ocp_write_dword().

When the device enters UPS power saving mode at 5000M link speed, will the
hardware receive correct speed information?

The follow-up commit defca1127dd51 ('r8152: Add support for the RTL8157
hardware') enables support_5000full for RTL_VER_16 but also does not add a
NWAY_5000M_FULL case to r8156_ups_flags().