Re: [PATCH net-next v3 2/8] r8152: support RTL8159 for different packages

[netdev-bot+sashiko@kernel.org]

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

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

- [Low] The changelog describes the patch as a pure enum split ("RTL8159
  comes in two packages, QFN68 and QFN100, which require different…

--

r8152: support RTL8159 for different packages

This splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100 and
updates every version switch accordingly.  Probe now reads PLA_PKG_DET
over a USB control transfer to decide which of the two packages is
present, and r8159_init() gains one package-specific register write.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 013e8d1abfc24..f01f442fa71a0 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -8716,6 +8739,10 @@ static void r8159_init(struct r8152 *tp)
>  	/* TX descriptor Signature */
>  	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd4ae, BIT(1));
>  
> +	/* Enable u2phy backup restore patch */
> +	if (tp->version == RTL_VER_17_QFN68)
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xb99c, BIT(0));
> +

[Severity: Low]
This isn't a bug, but could the changelog mention this write?  The commit
message describes the change as an enum split:

    RTL8159 comes in two packages, QFN68 and QFN100, which require different
    handling. Split RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100.

Every other hunk in the patch is a mechanical rename, and this
ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xb99c, BIT(0)) in r8159_init() is the
only place where the two packages actually behave differently.

RTL8159 has been driven as RTL_VER_17 since commit ad2a55b63b41af ("r8152:
Add support for the RTL8159 chip"), so QFN68 parts are running today without
this u2phy backup restore bit.

Is this write correcting misbehaviour on QFN68 hardware that is already
supported, or is it enablement for silicon that was not previously in the
field?  If it is the former, would a Fixes: ad2a55b63b41af ("r8152: Add
support for the RTL8159 chip") tag be appropriate so it reaches stable, and
could the changelog describe the symptom it addresses?

>  	/* Enable u3phy patch backup */
>  	ocp_write_word(tp, MCU_TYPE_USB, 0xb9a2, 0x0448);
>  

[ ... ]

> @@ -10287,7 +10340,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 %#02x\n",
> +				 pkg_det);
> +		}
>  		break;

[Severity: Low]
Along the same lines, the changelog does not mention that hw version 0x2020
now requires an extra PLA_PKG_DET control read at probe time to be
classified at all.  Could a sentence be added covering that?

Was this new detection path validated against both packages, and which
PLA_PKG_DET values were observed on each?

>  	default:
>  		version = RTL_VER_UNKNOWN;

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