Re: [PATCH net-next v6 09/10] r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] rtl_fc_pause_pkt_en() also takes the 'Unknown link speed' else
  branch when the link is down.

--

r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

This commit moves the per-family flow-control pause frame setup into a new
rtl_fc_pause_pkt_en() helper. RTL8156A now gets the FC timer and
FLOW_CTRL_PATCH_OPT setup, and RTL8157/RTL8159 program the new
PLA_PAUSE_LIMIT register from rtl8157_enable() based on the negotiated speed.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 023e7af4c5fa6..b5092fdaa12c7 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6172,6 +6177,104 @@ static void r8152b_enter_oob(struct r8152 *tp)
>  			   RCR_APM | RCR_AM | RCR_AB);
>  }
>  
> +static void rtl_fc_pause_pkt_en(struct r8152 *tp, u16 speed)
> +{

[ ... ]

> +	case RTL_VER_16:
> +	case RTL_VER_17_QFN68:
> +	case RTL_VER_17_QFN100:
> +		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> +
> +		num_pause_pkts = 0xa;
> +		ratio = 10000;
> +
> +		if (speed & _10bps) {
> +			ratio /= 10;
> +		} else if (speed & _100bps) {
> +			ratio /= 100;
> +		} else if (speed & _1000bps) {
> +			ratio /= 1000;
> +		} else if (speed & _2500bps) {
> +			ratio /= 2500;
> +		} else if (speed & _5000bps) {
> +			ratio /= 5000;
> +		} else if (speed & _10000bps) {
> +			ratio /= 10000;
> +		} else {
> +			/* No rate bit is set, e.g. a 500 or 1250 Mbit/s
> +			 * sub-rate, which the driver does not advertise.
> +			 * Disarm the limit.
> +			 */
> +			dev_err(&tp->intf->dev, "Unknown link speed\n");
> +			ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> +					  PAUSE_LIMIT_EN);
> +			break;
> +		}

[Severity: Low]
Does this else branch also run when the link is down, not only for the
500/1250 Mbit/s case the comment describes?

When LINK_STATUS is clear, PLA_PHYSTATUS has no rate bit set.
rtl8157_enable() passes the raw rtl8152_get_speed() value through without
checking LINK_STATUS:

    speed = rtl8152_get_speed(tp);
    rtl_fc_pause_pkt_en(tp, speed);

set_carrier() checks LINK_STATUS before it calls enable. Other callers only
look at the cached carrier state, for example rtl8152_change_mtu():

    if (netif_carrier_ok(dev)) {
        ...
        tp->rtl_ops.disable(tp);
        tp->rtl_ops.enable(tp);

rtl8152_post_reset() and rtl8152_set_coalesce() work the same way.

Here is one possible sequence. The PHY link drops, and intr_callback() sets
RTL8152_LINK_CHG and schedules the work. Before rtl_work_func_t() runs
set_carrier(), a user changes the MTU or coalesce settings, or a USB reset
completes. rtl8157_enable() then runs on a down link. It logs "Unknown link
speed" and clears PAUSE_LIMIT_EN.

The link might come back before the work samples it. In that case
set_carrier() sees LINK_STATUS together with netif_carrier_ok(). It never
calls enable again, so wouldn't pause limiting stay disarmed for that link
session?

Would it make sense to check LINK_STATUS here, or in rtl8157_enable(), so a
down link isn't reported as an unknown speed?

> +
> +		log2_ratio = ilog2(ratio);
> +		num_pause_pkts -= log2_ratio;

[ ... ]

> @@ -6703,6 +6806,8 @@ static int rtl8157_enable(struct r8152 *tp)
>  	r8153_set_rx_early_size(tp);
>  
>  	speed = rtl8152_get_speed(tp);
> +	rtl_fc_pause_pkt_en(tp, speed);
> +
>  	rtl_set_ifg(tp, speed);
>  
>  	return rtl_enable(tp);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008081556.47901-1-nic_swsd%40realtek.com