On 24/03/2026 11:55 am, Paolo Abeni wrote:

quoted

+static u32 ocp_adv_read(struct r8152 *tp, u16 cmd, u16 addr, u32 *data)
+{
+	int ret;
+
+	ret = wait_cmd_ready(tp);
+	if (ret)
+		return ret;
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_ADV_ADDR, addr);
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_ADV_CMD, cmd);
+
+	ret = wait_cmd_ready(tp);
+	if (ret)
+		return ret;
+
+	ret = ocp_read_dword(tp, MCU_TYPE_USB, USB_ADV_DATA, data);
+
+	return ret;
+}

Should ocp_adv_read() return int instead of u32? The function returns
'int ret' which can be 0 or -ETIMEDOUT from wait_cmd_ready(), but the
function signature declares u32. The companion function ocp_adv_write()
and all callers (rtl_bmu_read, rtl_ip_read) correctly use 'int ret' and
check 'ret < 0' for errors.

This is a clear mistake. I will change the return type to "int" and 
submit a new patch version. As a side remark, however, the patch-code 
you refer to, here, is not what I submitted. My submission can be found 
here:
https://lkml.org/lkml/2026/3/20/831

It reads:
+
+static u32 ocp_adv_read(struct r8152 *tp, u16 cmd, u16 addr, u32 *data)
+{
+	int ret;
+
+	ret = wait_cmd_ready(tp, USB_ADV_CMD);
+	if (ret < 0)
+		goto out;
+
+	ocp_write_word(tp, MCU_TYPE_USB, USB_ADV_ADDR, addr);
+
+	cmd |= ADV_CMD_BUSY;
+	ocp_write_word(tp, MCU_TYPE_USB, USB_ADV_CMD, cmd);
+
+	ret = wait_cmd_ready(tp, USB_ADV_CMD);
+	if (ret < 0)
+		goto out;
+
+	*data = ocp_read_dword(tp, MCU_TYPE_USB, USB_ADV_DATA);
+
+out:
+	return ret;
+}

Note that the call parameters of ocp_read_dword are different. It 
appears that the AI is mixing the patch I submitted with the out-of-tree 
driver the patch is based on.

quoted

+static int r8152_tx_csum(void *d, struct sk_buff *skb, u32 len)
  {
  	u32 mss = skb_shinfo(skb)->gso_size;
+	struct rx_desc *desc = d;

Is this the correct descriptor type? r8152_tx_csum() casts the void *d
parameter to 'struct rx_desc *desc' but this function handles TX
descriptors. The sibling TX functions (r8152_tx_vlan_tag, r8152_tx_len)
use 'struct tx_desc *'. The bug is latent because both struct tx_desc
and struct rx_desc have opts1 and opts2 at identical offsets.

This is a typo. I will fix it to tx_desc.

quoted

@@ -2923,13 +3283,14 @@ static netdev_features_t rtl8152_features_check(struct sk_buff *skb,
  						 netdev_features_t features)
  {
  	u32 mss = skb_shinfo(skb)->gso_size;
  	int max_offset = mss ? GTTCPHO_MAX : TCPHO_MAX;

  	if ((mss || skb->ip_summed == CHECKSUM_PARTIAL) &&
  	    skb_transport_offset(skb) > max_offset)
  		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
  	else if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz)
  		features &= ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);

  	return features;
  }

Does this match the RTL8157 limits? rtl8152_features_check() uses
TCPHO_MAX (0x7ff = 2047) for non-GSO checksummed packets on all chip
versions, but the new RTL8157 (RTL_VER_16) hardware has a smaller limit
TCPHO_MAX_2 (0x3ff = 1023) as checked in r8157_tx_csum(). For RTL8157,
packets with transport offsets between 1024 and 2047 will pass
features_check, but r8157_tx_csum will reject them and fall back to
r8152_csum_workaround, producing netif_warn messages.

I do not have any further information on the TCPHO_MAX value for the 
RTL8157 other than what is found in the out-of-tree driver. However, if 
this is explicitly changed, it must be assumed the lower limit is 
correct. I will fix this to read:

static netdev_features_t
rtl8152_features_check(struct sk_buff *skb, struct net_device *dev,
		       netdev_features_t features)
{
	struct r8152 *tp = netdev_priv(dev);
	u32 mss = skb_shinfo(skb)->gso_size;
	int max_offset;

	if (tp->version < RTL_VER_16)
		max_offset = mss ? GTTCPHO_MAX : TCPHO_MAX;
	else
		max_offset = mss ? GTTCPHO_MAX : TCPHO_MAX_2;

	if ((mss || skb->ip_summed == CHECKSUM_PARTIAL) &&
	    skb_transport_offset(skb) > max_offset)

in order to be on the safe side.

[ ... ]

quoted

@@ -8252,25 +8711,27 @@ static void r8156b_init(struct r8152 *tp)
  	u16 data;
  	u8 speed;

+	tp->coalesce = 15000;	/* 15 us */
+	data = r8153_phy_status(tp, 0);
+
  	if (test_bit(RTL8152_UNPLUG, &tp->flags))
  		return;

-	tp->coalesce = 15000;	/* 15 us */
  	r8153b_u1u2en(tp, false);

  	r8156_fc_parameter(tp);
-	rtl_tally_reset(tp);

+	if (tp->version < RTL_VER_13)
+		r8156_mdio_force_mode(tp);

[ ... ]

quoted

+	if (tp->version >= RTL_VER_13 && tp->version <= RTL_VER_15)
+		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_U2P3_CTRL, RX_DETECT8);

Are the version guards correct for RTL8156B? When merging r8156b_init into
r8156_init, the condition 'tp->version >= RTL_VER_13 && tp->version <=
RTL_VER_15' excludes RTL_VER_12. Before this patch, RTL_VER_12 was handled
by r8156b_init which executed these code paths unconditionally. The commit
message states VER_12 is RTL8156B hardware.

Three specific behavioral changes for RTL_VER_12: (1) RX_DETECT8 is no
longer set on USB_U2P3_CTRL (shown above), (2) the flow control
initialization block (SLOT_EN clear, FLOW_CTRL_EN set, FC_TIMER, FW_CTRL,
FW_TASK) is skipped later, and (3) the PHY_STAT_EXT_INIT path
ocp_reg_clr_bits(tp, 0xa466, BIT(0)) is skipped because 'tp->version >=
RTL_VER_13' excludes VER_12.

Should these be 'tp->version >= RTL_VER_12' instead to preserve the old
r8156b_init behavior for RTL_VER_12 hardware?

[ ... ]

Indeed, it appears that I had excluded RTL_VER_12 incorrectly for the 
RTL8156b. I will replace RTL_VER_13 in the comparisons with RTL_VER_12.

Again, the AI appears to refer to code that I did not submit. The patch 
I submitted does no longer have a r8156b_init function, however it is 
mentioned as present in the patch you cite.

B.