Re: [PATCH net-next v4 2/8] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] A failed or unrecognised PLA_PKG_DET read makes an already-
  identified RTL8159 (chip ID 0x2020) unusable.
- [Low] The commit message states the patch splits the enum and updates
  'every switch/case on RTL_VER_17', but it does not mention that the…
- [Low] The new diagnostic in __rtl_get_hw_ver() uses
  `dev_info(&udev->dev, "Unknown package %#02x\n", pkg_det)`.

--

r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

This splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100 and
updates the version switches accordingly. The package is detected by
reading the new PLA_PKG_DET register through a new __rtl_get_pkg_det()
helper called from __rtl_get_hw_ver() for chip ID 0x2020.

A couple of questions below.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 00b0fda306c30..9ce072c38b5b5 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -6404,8 +6419,15 @@ static int rtl8156_enable(struct r8152 *tp)
>  	set_tx_qlen(tp);
>  	rtl_set_eee_plus(tp);
>  
> -	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)
> -		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
> +	switch (tp->version) {
> +	case RTL_VER_10:
> +	case RTL_VER_11:
> +		break;
> +	default:
> +		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_RX_AGGR_NUM,
> +				  RX_AGGR_NUM_MASK);
> +		break;
> +	}

[Severity: Low]
This isn't a bug, but the changelog only says "update every switch/case on
RTL_VER_17 in the driver to handle both", and this hunk does something
else: the positive range test

	if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)

becomes an exclusion list, so every version other than RTL_VER_10 and
RTL_VER_11 that reaches rtl8156_enable() now clears RX_AGGR_NUM_MASK by
default.

With the versions rtl_ops_init() currently routes to rtl8156_enable() the
resulting register writes are identical, and the later patch "r8152: split
rtl8156_enable/up/down into per-chip-family functions" removes this switch
again, so there is no behavioural change in the tree.

Could the changelog mention the inverted version gate, since it is not just
an enum rename?

[ ... ]

> @@ -10182,9 +10211,33 @@ static int rtl_fw_init(struct r8152 *tp)
>  	return 0;
>  }
>  
> +static u32 __rtl_get_pkg_det(struct usb_device *udev)
> +{
> +	u32 pkg_det = 0;
> +	__le32 *tmp;
> +	int ret, i;
> +
> +	tmp = kmalloc_obj(*tmp);
> +	if (!tmp)
> +		return 0;
> +
> +	for (i = 0, ret = 0; i < 3 && ret != 4; i++)
> +		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +				      RTL8152_REQ_GET_REGS, RTL8152_REQT_READ,
> +				      PLA_PKG_DET, MCU_TYPE_PLA, tmp,
> +				      sizeof(*tmp), USB_CTRL_GET_TIMEOUT);
> +
> +	if (ret > 0)
> +		pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> +
> +	kfree(tmp);
> +	return pkg_det;
> +}

[Severity: High]
Can __rtl_get_pkg_det() report a read failure to its caller?

Both the allocation failure path and the "all three control transfers
failed" path return 0, which is also a legal register value (PKG_DET bits
[4:1] all zero), so the caller cannot tell an -EPIPE/-ETIMEDOUT/-ENOMEM
from real strap data.

Two smaller points in the same helper:

The loop's success condition is ret == 4, but the value is consumed when
ret > 0, so a short control transfer would be accepted and the unread
bytes of the kmalloc'ed (non-zeroed) buffer would be folded into pkg_det.
Should this test be ret == sizeof(*tmp), matching the loop?

The version read a few lines above retries and complains with
dev_warn("Needed %d retries to read version") when the transfer misbehaves.
Would it be worth propagating an error here in the same spirit, e.g.
returning the usb_control_msg() error via an out parameter?

>  static u8 __rtl_get_hw_ver(struct usb_device *udev)
>  {
>  	u32 ocp_data = 0;
> +	u32 pkg_det = 0;
>  	__le32 *tmp;
>  	u8 version;
>  	int ret;

[ ... ]

> @@ -10267,7 +10320,16 @@ static u8 __rtl_get_hw_ver(struct usb_device *udev)
>  		version = RTL_VER_16;
>  		break;
>  	case 0x2020:
> -		version = RTL_VER_17;
> +		pkg_det = __rtl_get_pkg_det(udev);
> +		if (pkg_det == 0x1e || pkg_det == 0x1c) {
> +			version = RTL_VER_17_QFN68;
> +		} else if (pkg_det == 0x18 || pkg_det == 0x1a) {
> +			version = RTL_VER_17_QFN100;
> +		} else {
> +			version = RTL_VER_UNKNOWN;

[Severity: High]
Following on from the helper above: does a single failed PLA_PKG_DET read
make an already identified RTL8159 unusable?

The chip has been positively identified from PLA_TCR0/TCR1 as 0x2020 at
this point, but a failed read yields pkg_det == 0, which lands in the else
branch and becomes RTL_VER_UNKNOWN. In rtl8152_probe():

	version = rtl8152_get_version(intf);
	if (version == RTL_VER_UNKNOWN)
		return -ENODEV;

	for (i = 0; i < RTL8152_PROBE_TRIES; i++) {

so the -ENODEV happens before the RTL8152_PROBE_TRIES retry loop and a
transient control-transfer error is not retried at probe level either.

rtl8152_cfgselector_choose_configuration() has the same test:

	/* Switch the device to vendor mode, if and only if the vendor mode
	 * driver supports it.
	 */
	if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
		return -ENODEV;

which leaves the device in its default CDC configuration, and
rtl8153_ecm_probe() then treats the zero version as "r8152 does not
support this device":

drivers/net/usb/r8153_ecm.c:
	if (rtl8152_get_version(intf))
		return -ENODEV;

Before this patch chip ID 0x2020 unconditionally bound as RTL_VER_17.
Would it make sense to keep binding when the PKG_DET read itself fails,
and only fall back to RTL_VER_UNKNOWN for a value that was actually read?

The same else branch is also taken for the twelve other encodings
PKG_MASK = 0x1e can produce. Is rejecting the device the intended
behaviour for those, or should one of the two packages be the default?

> +			dev_info(&udev->dev, "Unknown package %#02x\n",
> +				 pkg_det);

[Severity: Low]
Does this format string produce the intended padding? The kernel
vsnprintf() counts the "0x" emitted by the '#' flag inside the field
width, so "%#02x" behaves like "%#x": pkg_det == 0 prints as 0x0 rather
than 0x00. The sibling message in the default case of the same switch uses

	dev_info(&udev->dev, "Unknown version 0x%04x\n", ocp_data);

Would "%#04x" be what was meant here?

> +		}
>  		break;
>  	default:
>  		version = RTL_VER_UNKNOWN;

[ ... ]

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