Re: [net-next,v3,2/2] r8152: Add support for the RTL8157 hardware
From: Birger Koblitz
Date: Tue Mar 24 2026 - 12:59:57 EST
On 24/03/2026 11:55 am, Paolo Abeni wrote:
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:+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.
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.
This is a typo. I will fix it to tx_desc.+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.
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:
@@ -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.
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.
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.
[ ... ]
@@ -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);
[ ... ]
+ 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?
[ ... ]
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.