Re: [PATCH net-next v8 07/11] 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In drivers/net/usb/r8152.c, __rtl_get_hw_ver() no longer maps
  chip ID 0x2020 (RTL8159) straight to a supported version.

--

r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

This commit splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100.
When the chip ID is 0x2020, a new helper, __rtl_get_pkg_det(), reads the
PLA_PKG_DET register to detect the package, and unrecognised values fall
back to RTL_VER_UNKNOWN. r8157_init() also gains a QFN68-only u2phy backup
restore write.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index a1c2e1e08b161..b6f252722a5eb 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -10227,9 +10254,38 @@ 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 == sizeof(*tmp)) {
> +		if (i != 1)
> +			dev_warn(&udev->dev,
> +				 "Needed %d retries to read package\n", i - 1);
> +
> +		pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> +	}
> +
> +	kfree(tmp);
> +	return pkg_det;
> +}
> +

[ ... ]

> @@ -10312,7 +10368,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;
> +			dev_info(&udev->dev, "Unknown package 0x%04x\n",
> +				 pkg_det);
> +		}

[Severity: Low]
If the PLA_PKG_DET read fails, does a 0x2020 chip that was handled as
RTL_VER_17 before this patch now become RTL_VER_UNKNOWN?

__rtl_get_pkg_det() returns 0 in three cases: kmalloc_obj() fails, all
three usb_control_msg() attempts fail, or the reply is short. A 0 here
takes the else branch. The log then says "Unknown package 0x0000" instead
of reporting a read error.

With RTL_VER_UNKNOWN, rtl8152_cfgselector_choose_configuration() returns
early and never selects the vendor configuration:

    if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
        return -ENODEV;

rtl8153_ecm_probe() then binds the device, because rtl8152_get_version()
now returns 0.

The read might also succeed in the cfgselector and fail later in
rtl8152_probe(). In that case the device is already in vendor mode and no
driver binds to it.

Apart from the QFN68-only 0xb99c write, RTL_VER_17_QFN100 behaves exactly
like the old RTL_VER_17. Could a read failure fall back to QFN100? Or could
it at least be reported separately from an unrecognised package value?

The commit message describes the RTL_VER_UNKNOWN fallback only for
unrecognised package values, not for I/O or allocation failures.

>  		break;
>  	default:
>  		version = RTL_VER_UNKNOWN;

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