Re: [net-next] r8152: Add 2500baseT EEE status/configuration support

[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 2500baseT EEE status/configuration support

The r8152 driver supports the RTL8156, which is a 2.5Gbit Ethernet
controller for USB 3.0, for which support is added for configuring and
displaying the EEE advertisement status for 2.5GBit connections. The
patch also corrects the determination of whether EEE is active to include
the 2.5GBit connection status and make the determination dependent not on
the desired speed configuration (tp->speed), but on the actual speed used
by the controller.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 8d36162f36df..a831acd385cd 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8924,6 +8926,7 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>  	u16 val;
> +	u16 speed = rtl8152_get_speed(tp);

Simon says: please declare local variables in reverse xmas tree order -
            longest line to shortest.

>
>  	val = r8152_mmd_read(tp, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->supported, val);
> @@ -8936,8 +8939,14 @@ static int r8152_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>
>  	eee->eee_enabled = tp->eee_en;
>
> -	linkmode_and(common, eee->advertised, eee->lp_advertised);
> -	eee->eee_active = phy_check_valid(tp->speed, tp->duplex, common);
> +	if (speed & _1000bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, common);
> +	if (speed & _100bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, common);
> +
> +	linkmode_and(common, common, eee->advertised);
> +	linkmode_and(common, common, eee->lp_advertised);
> +	eee->eee_active = !linkmode_empty(common);

Does this code read uninitialized memory? The 'common' bitmap is declared
via __ETHTOOL_DECLARE_LINK_MODE_MASK(common), which expands to
DECLARE_BITMAP -- an uninitialized stack array:

unsigned long common[BITS_TO_LONGS(__ETHTOOL_LINK_MODE_MASK_NBITS)];

The new code conditionally sets individual bits via linkmode_set_bit(),
which calls __set_bit() to set a single bit position. This leaves all
other bits in the array containing whatever garbage was on the stack.

Then linkmode_and(common, common, eee->advertised) reads all the
uninitialized bits when performing the bitwise AND operation.

The old code was safe because it used linkmode_and(common,
eee->advertised, eee->lp_advertised), which fully wrote 'common' from two
initialized bitmaps before reading it.

With uninitialized stack data, eee_active could be incorrectly reported as
true if garbage bits happen to overlap with the advertised and
lp_advertised bitmasks.

>
>  	return 0;
>  }
> @@ -8948,7 +8957,10 @@ static int r8152_set_eee(struct r8152 *tp, struct ethtool_keee *eee)
>
>  	tp->eee_en = eee->eee_enabled;
>  	tp->eee_adv = val;
> -
> +	if (tp->support_2500full) {
> +		val = linkmode_to_mii_eee_cap2_t(eee->advertised);
> +		tp->eee_adv2 = val;
> +	}
>  	rtl_eee_enable(tp, tp->eee_en);

[Simon says: I note that Daniel Golle flagged this issue already]

Is the user's 2500baseT EEE configuration actually applied to hardware?
The code stores the user's preference in tp->eee_adv2, but
rtl_eee_enable() never writes this value to the OCP_EEE_ADV2 register.

For RTL_VER_10-15 (the 2500full-capable chips), rtl_eee_enable() calls
r8156_eee_en() which unconditionally sets MDIO_EEE_2_5GT rather than using
tp->eee_adv2:

drivers/net/usb/r8152.c:r8156_eee_en() {
    ...
    if (enable) {
        ...
        set |= MDIO_EEE_2_5GT;
    } else {
        ...
        clr |= MDIO_EEE_2_5GT;
    }
    ocp_data = (ocp_data & ~clr) | set;
    ocp_reg_write(tp, OCP_EEE_ADV2, ocp_data);
}

Then rtl_eee_enable() writes only tp->eee_adv to OCP_EEE_ADV:

drivers/net/usb/r8152.c:rtl_eee_enable() {
    ...
    case RTL_VER_10: ... case RTL_VER_15:
        r8156_eee_en(tp, enable);
        ...
        ocp_reg_write(tp, OCP_EEE_ADV, tp->eee_adv);
        break;
}

The OCP_EEE_ADV2 register is never updated with tp->eee_adv2. This means
user attempts to selectively control 2500baseT EEE advertisement via
ethtool are silently ignored.

The cap1 path correctly writes tp->eee_adv to OCP_EEE_ADV. Shouldn't the
cap2 path similarly write tp->eee_adv2 to OCP_EEE_ADV2?

>
>  	return 0;
>  }
> @@ -8958,6 +8970,7 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(common);
>  	u16 val;
> +	u16 speed = rtl8152_get_speed(tp);
>
>  	val = ocp_reg_read(tp, OCP_EEE_ABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->supported, val);

[ ... ]

> @@ -8968,10 +8981,29 @@ static int r8153_get_eee(struct r8152 *tp, struct ethtool_keee *eee)
>  	val = ocp_reg_read(tp, OCP_EEE_LPABLE);
>  	mii_eee_cap1_mod_linkmode_t(eee->lp_advertised, val);
>
> +	if (tp->support_2500full) {
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, eee->supported);
> +
> +		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);
> +
> +		if (speed & _2500bps)
> +			linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, common);
> +	}
> +
>  	eee->eee_enabled = tp->eee_en;
>
> -	linkmode_and(common, eee->advertised, eee->lp_advertised);
> -	eee->eee_active = phy_check_valid(tp->speed, tp->duplex, common);
> +	if (speed & _1000bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, common);
> +	if (speed & _100bps)
> +		linkmode_set_bit(ETHTOOL_LINK_MODE_100baseT_Full_BIT, common);
> +
> +	linkmode_and(common, common, eee->advertised);
> +	linkmode_and(common, common, eee->lp_advertised);
> +	eee->eee_active = !linkmode_empty(common);

The same uninitialized memory issue exists here in r8153_get_eee. The
'common' bitmap declared via __ETHTOOL_DECLARE_LINK_MODE_MASK is never
zeroed before individual speed bits are conditionally set with
linkmode_set_bit(). Then linkmode_and() reads the uninitialized garbage
bits.

This function affects RTL_VER_03 through RTL_VER_15, which is the majority
of supported devices.