Re: [net-next,v3,2/2] r8152: Add support for the RTL8157 hardware

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
r8152: Add support for the RTL8157 hardware

The RTL8157 uses a different packet descriptor format compared to the
previous generation of chips. Add support for this format by adding a
descriptor format structure into the r8152 structure and corresponding
desc_ops functions which abstract the vlan-tag, tx/rx len and
tx/rx checksum algorithms.

Also, add support for the SRAM access interface of the RTL8157 and
the ADV indirect access interface and PHY setup.

For initialization of the RTL8157, combine the existing RTL8156B and
RTL8156 init functions and add RTL8157-specific functinality in order
to improve code readability and maintainability.
r8156_init() is now called with RTL_VER_10 and RTL_VER_11 for the RTL8156,
with RTL_VER_12, RTL_VER_13 and RTL_VER_15 for the RTL8156B and with
RTL_VER_16 for the RTL8157 and checks the version for chip-specific code.
Also add USB power control functions for the RTL8157.

Add support for the USB device ID of Realtek RTL8157-based adapters. Detect
the RTL8157 as RTL_VER_16 and set it up.

Signed-off-by: Birger Koblitz <mail@birger-koblitz.de>

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index bab8e10e5f99..6cd3c321edb4 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -1617,6 +1734,122 @@ static int generic_ocp_write(struct r8152 *tp, u16 index, u16 byteen,
>  	return ret;
>  }
>
> +static int wait_cmd_ready(struct r8152 *tp)
> +{
> +	u32 val;
> +
> +	return poll_timeout_us(ocp_read_dword(tp, MCU_TYPE_USB, USB_ADV_CMD, &val),
> +			       val, !(val & ADV_CMD_BUSY), 50, 1000000);
> +}
> +
> +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.

[ ... ]

> @@ -2252,8 +2507,17 @@ static inline void rtl_tx_vlan_tag(struct tx_desc *desc,
>  	}
>  }
>
> +static void r8152_tx_vlan_tag(void *d, struct sk_buff *skb)
> +{
> +	struct tx_desc *desc = d;
> +
> +	rtl_tx_vlan_tag(desc, skb);
> +}
> +
>  static int r8152_tx_csum(struct r8152 *tp, struct tx_desc *desc,
> -			 struct sk_buff *skb, u32 len)
> +			 struct sk_buff *skb, u32 len);
> +
> +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.

>  	u32 opts1, opts2 = 0;
>  	int ret = TX_CSUM_SUCCESS;
>
> @@ -2359,6 +2626,74 @@ static inline void rtl_rx_vlan_tag(struct rx_desc *desc,
>  		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2));
>  }
>
> +static void r8152_rx_vlan_tag(void *d, struct sk_buff *skb)
> +{
> +	struct rx_desc *desc = d;
> +
> +	rtl_rx_vlan_tag(desc, skb);
> +}
> +
> +static u32 r8152_tx_len(void *d)
> +{
> +	struct tx_desc *desc = d;
> +
> +	return le32_to_cpu(desc->opts1) & TX_LEN_MASK;
> +}

[ ... ]

> @@ -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.

[ ... ]

> @@ -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?

[ ... ]