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

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

The RTL8157 supports 5GBit Link speeds. This commit adds support for this
speed in the setup and setting/getting through ethtool, along with 5GBit
EEE functionality for setup and ethtool get/set methods.

> The RTL8157 supports 5GBit Link speeds. Add support for this speed
> in the setup and setting/getting through ethool. Also add 5GBit EEE.
                                          ^^^^^^
This isn't a bug, but there's a typo: 'ethool' should be 'ethtool'.

> 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() handle the new NWAY_5000M_FULL value?

Looking at r8156_ups_flags() in drivers/net/usb/r8152.c, the function
switches on tp->ups_info.speed_duplex:

static u16 r8156_ups_flags(struct r8152 *tp)
{
	u16 ups_flags = 0;

	if (tp->ups_info.green)
		ups_flags |= UPS_FLAGS_EN_GREEN;

	if (tp->ups_info.aldps)
		ups_flags |= UPS_FLAGS_EN_ALDPS;

	if (tp->ups_info.eee)
		ups_flags |= UPS_FLAGS_EN_EEE;

	if (tp->ups_info.flow_control)
		ups_flags |= UPS_FLAGS_EN_FLOW_CONTROL;

	if (tp->ups_info.eee_ckdiv)
		ups_flags |= UPS_FLAGS_EN_EEE_CKDIV;

	if (tp->ups_info._10m_ckdiv)
		ups_flags |= UPS_FLAGS_EN_10M_CKDIV;

	if (tp->ups_info.eee_cmod_lv)
		ups_flags |= UPS_FLAGS_EEE_CMOD_LV_EN;

	if (tp->ups_info._250m_ckdiv)
		ups_flags |= UPS_FLAGS_250M_CKDIV;

	switch (tp->ups_info.speed_duplex) {
	case FORCE_10M_HALF:
		ups_flags |= ups_flags_speed(0);
		break;
	case FORCE_10M_FULL:
		ups_flags |= ups_flags_speed(1);
		break;
	case FORCE_100M_HALF:
		ups_flags |= ups_flags_speed(2);
		break;
	case FORCE_100M_FULL:
		ups_flags |= ups_flags_speed(3);
		break;
	case NWAY_10M_HALF:
		ups_flags |= ups_flags_speed(4);
		break;
	case NWAY_10M_FULL:
		ups_flags |= ups_flags_speed(5);
		break;
	case NWAY_100M_HALF:
		ups_flags |= ups_flags_speed(6);
		break;
	case NWAY_100M_FULL:
		ups_flags |= ups_flags_speed(7);
		break;
	case NWAY_1000M_FULL:
		ups_flags |= ups_flags_speed(8);
		break;
	case NWAY_2500M_FULL:
		ups_flags |= ups_flags_speed(9);
		break;
	default:
		break;
	}

	return ups_flags;
}

When rtl8152_set_speed() sets tp->ups_info.speed_duplex = NWAY_5000M_FULL
and r8156_ups_en(enable=true) subsequently calls r8156_ups_flags(), the
switch falls through to the default case and no speed bits are set in the
UPS flags word.

Is the UPS power management configuration incorrect for 5Gbit link speeds
without a case for NWAY_5000M_FULL in r8156_ups_flags()?

This path is activated by the next commit in the series (58ba3b6c476d
'r8152: Add support for the RTL8157 hardware') which sets
support_5000full = 1.

[ ... ]