Re: [PATCH net-next 1/4] r8152: Add support for 10Gbit Link Speeds and EEE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] 10G EEE advertisement incorrectly clears bit 3 in ADV2 register instead of configuring ADV1.
- [High] Missing `NWAY_10000M_FULL` switch case in `r8156_ups_flags()` causes incorrect firmware speed configuration.
--

commit 888cb24a1bd62bd09199a0e72a5e959941c2af81
Author: Birger Koblitz <mail@birger-koblitz.de>

r8152: Add support for 10Gbit Link Speeds and EEE

This commit adds support for 10Gbit Link speeds and 10Gbit EEE for the
RTL8159. It updates the setup routines and ethtool getters/setters.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 7337bf1b7d6a..01e65d845f87 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -621,6 +621,7 @@ enum spd_duplex {
>  	FORCE_1000M_FULL,
>  	NWAY_2500M_FULL,
>  	NWAY_5000M_FULL,
> +	NWAY_10000M_FULL,
>  };

When updating link power states, r8156_ups_flags() maps the speed to USB UPS
flags using a switch statement on tp->ups_info.speed_duplex.

Since this patch introduces NWAY_10000M_FULL but does not add a corresponding
case in r8156_ups_flags(), will the 10G speed fall through to the default
case?

If it falls through, it appears the speed bits in ups_flags remain 0, which
corresponds to FORCE_10M_HALF. Could this configure the firmware and power
state for 10Mbps Half Duplex while the link is actually operating at 10Gbps?

[ ... ]

> @@ -5773,6 +5779,11 @@ static void r8156_eee_en(struct r8152 *tp, bool enable)
>  	else
>  		config &= ~MDIO_EEE_5GT;
>  
> +	if (enable && (tp->eee_adv2 & MDIO_EEE_10GT))
> +		config |= MDIO_EEE_10GT;
> +	else
> +		config &= ~MDIO_EEE_10GT;
> +
>  	ocp_reg_write(tp, OCP_EEE_ADV2, config);
>  }

Is MDIO_EEE_10GT correctly placed in OCP_EEE_ADV2? The 10GBASE-T EEE
capability is normally defined in EEE Advertisement 1 (Register 7.60),
rather than Advertisement 2 (Register 7.62).

Also, since tp->eee_adv2 is populated using linkmode_to_mii_eee_cap2_t(),
which ignores the 10G bit, tp->eee_adv2 & MDIO_EEE_10GT might always
evaluate to false.

Does this cause the else branch to execute unconditionally and unintentionally
clear bit 3 of OCP_EEE_ADV2?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-rtl8159_net_next-v1-0-52d03927b46f@birger-koblitz.de?part=1