Re: [PATCH net-next v3 7/8] r8152: support rtl_fc_pause_pkt_en()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Commit message mischaracterizes the change as an extraction
  and omits the new register programming it introduces.

--

r8152: support rtl_fc_pause_pkt_en()

This moves the flow control programming out of r8156b_init() into a new
helper rtl_fc_pause_pkt_en(), and calls it from r8156_init(), r8156b_init()
and rtl8157_enable(). The helper switches on tp->version and, for RTL8157
and RTL8159, programs a new pause limit register based on the link speed.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 1fcb1cc5b4a18..2b0972b967385 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -89,6 +89,7 @@
>  #define PLA_MTPS		0xe615
>  #define PLA_TXFIFO_CTRL		0xe618
>  #define PLA_TXFIFO_FULL		0xe61a
> +#define PLA_PAUSE_LIMIT         0xe61e
>  #define PLA_RSTTALLY		0xe800
>  #define PLA_CR			0xe813
>  #define PLA_CRWECR		0xe81c
> @@ -301,6 +302,10 @@
>  #define MTPS_JUMBO		(12 * 1024 / 64)
>  #define MTPS_DEFAULT		(6 * 1024 / 64)
>  
> +/* PLA_PAUSE_LIMIT */
> +#define PAUSE_LIMIT_EN		BIT(3)
> +#define PAUSE_LIMIT_MASK	0xf0
> +
>  /* PLA_RSTTALLY */
>  #define TALLY_RESET		0x0001
>  
> @@ -6088,6 +6093,93 @@ 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)
> +{
> +	int log2_ratio, ratio;
> +	u16 num_pause_pkts;
> +	u32 ocp_data;
> +
> +	switch (tp->version) {
> +	case RTL_VER_10:
> +	case RTL_VER_11:
> +		ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> +			       CTRL_TIMER_EN | (1000 / 8));
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> +				  FLOW_CTRL_PATCH_OPT);
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
> +		break;
> +	case RTL_VER_12:
> +	case RTL_VER_13:
> +	case RTL_VER_15:
> +		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
> +
> +		/* enable fc timer and set timer to 600 ms. */
> +		ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> +			       CTRL_TIMER_EN | (600 / 8));
> +
> +		ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL);
> +		if (!(ocp_data & DACK_DET_EN))
> +			ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> +					  FLOW_CTRL_PATCH_2);
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
> +		break;
> +	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 & LINK_STATUS)) {
> +			dev_dbg(&tp->intf->dev, "No link\n");
> +			goto no_link;
> +		} else 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 {
> +			dev_err(&tp->intf->dev, "Unknown link speed\n");
> +			goto no_link;
> +		}
> +
> +		log2_ratio = ilog2(ratio);
> +		num_pause_pkts -= log2_ratio;
> +
> +		/* Round up if ratio is more than halfway to the next power of 2.
> +		 * Floating-point is avoided by rewriting
> +		 * ratio > 1.5 * 2^log2_ratio as
> +		 * 2 * ratio > 3 * 2^log2_ratio
> +		 */
> +		if (2 * ratio > 3 * (1 << log2_ratio))
> +			num_pause_pkts--;
> +
> +no_link:
> +		ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> +			      PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN,
> +			      num_pause_pkts << 4);
> +
> +		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> +				  PAUSE_LIMIT_EN);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +

[Severity: Medium]
Is the changelog description of "Extract it as rtl_fc_pause_pkt_en()"
accurate for the RTL_VER_10/RTL_VER_11 and RTL_VER_16/RTL_VER_17 arms of
rtl_fc_pause_pkt_en()?  The three arms are not the same sequence that was
inline in r8156b_init().

For RTL_VER_10/RTL_VER_11 the helper programs:

	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
		       CTRL_TIMER_EN | (1000 / 8));
	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
			  FLOW_CTRL_PATCH_OPT);
	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);

At the series baseline the flow control block in r8156_init() was gated as:

	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_15) {
		ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
		ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
		...
	}

so RTL_VER_10/RTL_VER_11 received no flow control programming, and
FLOW_CTRL_PATCH_OPT was only set from the RTL8153B firmware path.  Could
the changelog say why RTL8156A now gets a 1000 ms timer plus
FLOW_CTRL_PATCH_OPT, while RTL8156B keeps the 600 ms timer,
FLOW_CTRL_PATCH_2 and PLA_CPCR FLOW_CTRL_EN?

For RTL_VER_16/RTL_VER_17_QFN68/RTL_VER_17_QFN100 the helper adds a new
register, PLA_PAUSE_LIMIT (0xe61e), the new PAUSE_LIMIT_EN/PAUSE_LIMIT_MASK
bits, and a link-speed dependent pause packet count, but it sets neither
PLA_CPCR FLOW_CTRL_EN nor USB_FC_TIMER even though the changelog says the
flow control patch was "missing for ... RTL8157/8159".  None of the new
register or the pause budget calculation is described in the changelog.

Could the changelog describe the observed malfunction, the link speeds and
traffic conditions it was seen under, and what was tested, so the behaviour
change on already supported RTL8156A hardware and the new pause limit
programming can be judged?

>  static int r8153_pre_firmware_1(struct r8152 *tp)
>  {
>  	int i;
> @@ -6619,6 +6711,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);
> @@ -8751,6 +8845,8 @@ static void r8156_init(struct r8152 *tp)
>  
>  	usb_enable_lpm(tp->udev);
>  
> +	rtl_fc_pause_pkt_en(tp, 0);
> +
>  	r8156_mac_clk_spd(tp, true);
>  

[ ... ]

> @@ -8852,20 +8947,7 @@ static void r8156b_init(struct r8152 *tp)
>  
>  	usb_enable_lpm(tp->udev);
>  
> -	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> -
> -	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
> -
> -	/* enable fc timer and set timer to 600 ms. */
> -	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> -		       CTRL_TIMER_EN | (600 / 8));
> -
> -	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL);
> -	if (!(ocp_data & DACK_DET_EN))
> -		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> -				  FLOW_CTRL_PATCH_2);
> -
> -	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK, FC_PATCH_TASK);
> +	rtl_fc_pause_pkt_en(tp, 0);
>  
>  	r8156_mac_clk_spd(tp, true);
>

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