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

[Jakub Kicinski]

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 10Gbit Link Speeds and EEE

This change adds support for 10 Gbit link speeds on RTL8159 via the
ethtool link_ksettings interface and adds 10 Gbit EEE advertisement
handling alongside the existing 2.5G/5G paths.

> 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

[ ... ]

> @@ -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 actually a bit of the EEE advertisement 2 register
(7.62, cap2 / OCP_EEE_ADV2)?

MDIO_EEE_10GT is defined by IEEE 802.3 as a bit of the EEE advertisement
register 7.60 (cap1 / OCP_EEE_ADV), not of register 7.62. The existing
10G EEE advertisement already flows through tp->eee_adv (populated by
linkmode_to_mii_eee_cap1_t() in r8152_set_eee()) and is written to
OCP_EEE_ADV by rtl_eee_enable().

tp->eee_adv2 is populated only by linkmode_to_mii_eee_cap2_t() in
r8152_set_eee(), which covers 2500baseT and 5000baseT and cannot set
MDIO_EEE_10GT, plus the chip-init assignments in rtl_ops_init() which
also do not set this bit.

With that in mind, is the enable && (tp->eee_adv2 & MDIO_EEE_10GT)
condition ever true?  The else branch will then unconditionally clear
bit 3 of OCP_EEE_ADV2 on every call, which is a cap1 bit being written
into the cap2 register.

[ ... ]

> @@ -8968,6 +9008,13 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  			linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT, common);
>  	}
>  
> +	if (tp->support_10000full) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, eee->supported);
> +
> +		if (speed & _10000bps)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT, common);
> +	}
> +
>  	eee->eee_enabled = tp->eee_en;

Should this hunk also extend the earlier gating inside r8153_get_eee()?

A few lines above, the cap2 register reads are still gated on:

	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);
	}

The equivalent disjunction in rtl8152_set_speed() and
rtl8152_get_link_ksettings() was updated to include support_10000full,
but r8153_get_eee() (and r8152_set_eee() at the analogous spot) still
only checks support_2500full || support_5000full.

A hypothetical chip that sets only support_10000full would bypass the
OCP_EEE_ADV2 / OCP_EEE_LPABLE2 reads and never reflect 10G EEE into
eee->advertised.  Should the condition be made consistent with the
other sites?

> @@ -9982,6 +10029,11 @@ static int rtl8152_probe_once(struct usb_interface *intf,
>  			tp->speed = SPEED_5000;
>  			tp->advertising |= RTL_ADVERTISED_5000_FULL;
>  		}
> +		if (tp->support_10000full &&
> +		    tp->udev->speed >= USB_SPEED_SUPER) {
> +			tp->speed = SPEED_10000;
> +			tp->advertising |= RTL_ADVERTISED_10000_FULL;
> +		}
>  		tp->advertising |= RTL_ADVERTISED_1000_FULL;
>  	}
>  	tp->duplex = DUPLEX_FULL;

Is USB_SPEED_SUPER the correct gate for defaulting to a 10 Gbit link?

USB_SPEED_SUPER is USB 3.0 SuperSpeed at 5 Gbit/s signaling, which
cannot carry a 10 Gbit Ethernet link.  The minimum bus rate that can
sustain 10GBASE-T is USB_SPEED_SUPER_PLUS (USB 3.1 Gen 2, 10 Gbit/s).

With this check, a 10G-capable RTL8159 plugged into a USB 3.0 host
will still default to tp->speed = SPEED_10000 and advertise
RTL_ADVERTISED_10000_FULL, so the Ethernet link may negotiate at 10G
while the USB bus is bottlenecked well below that, leading to
misleading ethtool speed reporting and back-pressure on the bus.

The same >= USB_SPEED_SUPER gate is reused from the 2.5G and 5G
defaults, which fit inside the USB 3.0 envelope; should the 10G
default instead use >= USB_SPEED_SUPER_PLUS?
-- 
pw-bot: cr