Re: [PATCH net-next v5 2/9] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-05 09:57:21
Also in:
linux-usb, lkml
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 2 (1) · Low: 0 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). - [Medium] The commit message says the split is needed because QFN68 and QFN100 "need different firmware and a package-specific u2phy backup-… - [Medium] In __rtl_get_pkg_det() (drivers/net/usb/r8152.c), the retry loop `for (i = 0, ret = 0; i < 3 && ret != 4; i++) ret =… Pre-existing issues: - [Medium] This problem wasn't introduced by this patch, but in __rtl_get_hw_ver() (drivers/net/usb/r8152.c) the buffer `tmp` is… -- 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. The package is detected by a new __rtl_get_pkg_det() helper that reads PLA_PKG_DET. Every RTL_VER_17 switch case now handles both variants, and r8159_init() gains a u2phy backup restore write that only applies to QFN68.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index 4d8f0815d79d1..30af6ed3a638f 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -10172,7 +10194,8 @@ static int rtl_fw_init(struct r8152 *tp) case RTL_VER_16: rtl_fw->fw_name = FIRMWARE_8157_1; break; - case RTL_VER_17: + case RTL_VER_17_QFN68: + case RTL_VER_17_QFN100: rtl_fw->fw_name = FIRMWARE_8159_1;
[Severity: Medium]
The commit message says the two packages "need different firmware and a
package-specific u2phy backup-restore patch". However, rtl_fw_init() still
gives both RTL_VER_17_QFN68 and RTL_VER_17_QFN100 the same firmware,
FIRMWARE_8159_1 ("rtl_nic/rtl8159-1.fw").
The driver still has only one 8159 firmware define and one
MODULE_FIRMWARE(FIRMWARE_8159_1). The later patches in the series, up to
"r8152: enable UPS for RTL8157 and RTL8159 QFN68", do not change this
either.
Is per-package firmware selection missing from this patch? Or should the
commit message drop the claim about different firmware?
quoted hunk ↗ jump to hunk
break; default:@@ -10182,9 +10205,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 != 0) + dev_warn(&udev->dev, + "Needed %d retries to read package\n", i);
[Severity: Medium]
Won't this warning fire on every successful read? The loop increments i
after the iteration that succeeds. If the first usb_control_msg() returns
4, i is already 1 when the loop exits, so the i != 0 check is always true.
The printed count is also one too high: 1 for a first-try success, 3 for a
success on the third attempt.
The version read loop in __rtl_get_hw_ver() avoids this by breaking out on
success:
for (i = 0; i < 3; i++) {
ret = usb_control_msg(...);
if (ret > 0) {
ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
break;
}
}
if (i != 0 && ret > 0)
dev_warn(&udev->dev, "Needed %d retries to read version\n", i);
__rtl_get_hw_ver() runs from both the cfgselector and the probe paths, so
the false warning can appear more than once per RTL8159 plug-in. The code
is still the same at the end of the series.
quoted hunk ↗ jump to hunk
+ + pkg_det = __le32_to_cpu(*tmp) & PKG_MASK; + } + + kfree(tmp); + return pkg_det; +} + static u8 __rtl_get_hw_ver(struct usb_device *udev) { u32 ocp_data = 0; + u32 pkg_det = 0; __le32 *tmp; u8 version; int ret;
[Severity: Medium]
This isn't a bug introduced by this patch, but the PLA_TCR0 read in
__rtl_get_hw_ver() accepts a reply of any positive length:
tmp = kmalloc_obj(*tmp);
...
if (ret > 0) {
ocp_data = (__le32_to_cpu(*tmp) >> 16) & VERSION_MASK;
break;
}
kmalloc_obj() does not zero the buffer. If a faulty or malicious device
sends only 1 to 3 bytes, can the upper bytes of *tmp still hold leftover
heap data that then gets decoded as the chip version?
That could misidentify the chip, or leak uninitialized heap bytes to dmesg
through the "Unknown version 0x%04x" message.
The new __rtl_get_pkg_det() helper already requires
ret == sizeof(*tmp). Would the same check make sense here?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001091144.28191-1-nic_swsd%40realtek.com