Re: [PATCH net-next v8 04/11] r8152: fix flow control for RTL8156/RTL8156B/RTL8157/RTL8159

[netdev-bot+sashiko@kernel.org]

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] On RTL8157/RTL8159 (RTL_VER_16/RTL_VER_17), rtl_fc_pause_pkt_en()
  in drivers/net/usb/r8152.c clears SLOT_EN in PLA_RCR (line 6006).
- [Low] In rtl_fc_pause_pkt_en() (drivers/net/usb/r8152.c, lines
  6023-6031), the fallback branch of the new VER_16/17 speed decode has…

--

r8152: fix flow control for RTL8156/RTL8156B/RTL8157/RTL8159

This commit moves the per-chip flow control setup into a new helper,
rtl_fc_pause_pkt_en(). It adds the missing FC timer and
FLOW_CTRL_PATCH_OPT setup for RTL8156A (VER_10/11). For RTL8157/RTL8159
(VER_16/17) it programs a pause packet limit from rtl8156_enable(),
using the new PLA_PAUSE_LIMIT register and a count that depends on the
link speed.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 1517753f843ef..6f55aed4709e1 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -5961,6 +5966,103 @@ 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:
> +		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);

[Severity: Low]
This isn't a functional bug as far as I can tell, but is clearing
SLOT_EN on RTL_VER_16/RTL_VER_17 intended to be part of this fix? For
these chips the commit message only says:

    VER_16/17: limit pause packets with the new PLA_PAUSE_LIMIT register
    instead of the FW patch.

Before this patch, the only place that cleared SLOT_EN was r8156_init(),
and only for RTL_VER_12..RTL_VER_15. So this changes the receive config
on RTL8157/RTL8159 for the first time.

This helper is called from rtl8156_enable() for these chips. That means
the clear now runs on every enable: set_carrier(), rtl8152_change_mtu(),
rtl8152_set_coalesce() and rtl8152_post_reset(). It also runs on the
"Unknown link speed" path.

Could the commit message mention the PLA_RCR change and say why the
pause frame fix needs it? The code is the same at the end of the series,
where rtl8157_enable() calls the helper for RTL_VER_16 and
RTL_VER_17_QFN68/QFN100.

> +
> +		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]
Is this comment accurate? The driver already defines rate bits for both
of these speeds in enum rtl_register_content:

	_1250bps	= BIT(9),
	_500bps		= BIT(8),

Also, rtl8156_enable() already tests speed & _500bps for VER_10/11. At
500 or 1250 Mbit/s a rate bit is set. The if/else chain just doesn't
check for it.

The case where no rate bit is set looks like link down instead.
rtl8156_enable() passes the raw rtl8152_get_speed() value here without
checking LINK_STATUS.

rtl8152_change_mtu(), rtl8152_set_coalesce() and rtl8152_post_reset()
decide whether to call rtl_ops.enable() using only netif_carrier_ok().
The link change work updates that state later:

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

Suppose the cable is unplugged and the MTU or coalesce settings change
before set_carrier() runs. Wouldn't this print "Unknown link speed" with
dev_err() for what is really a link down?

The next real link up reprograms PLA_PAUSE_LIMIT, so only the comment
and the log message are affected. The code is unchanged at the end of
the series, where rtl8157_enable() calls rtl_fc_pause_pkt_en() the same
way.

[ ... ]

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