Re: [PATCH net-next v6 03/10] 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] The retry count printed by __rtl_get_pkg_det() is one too high.

--

r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

This patch splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100.
It tells them apart at probe time by reading the new PLA_PKG_DET register
through a new __rtl_get_pkg_det() helper. r8159_init() also gets a
QFN68-only write that enables the u2phy backup restore patch.

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

[ ... ]

> @@ -10180,9 +10202,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);

[Severity: Low]
Is the count printed here one higher than the real number of retries?

The loop increments i before it checks ret in the for condition, so when
the loop exits, i holds the number of attempts. If the first read fails
and the second one returns 4 bytes, i is 2. The message then says "Needed
2 retries" when only one retry was done.

The loop in __rtl_get_hw_ver() breaks before i++, so the same message
there shows the actual retry count:

	for (i = 0; i < 3; i++) {
		ret = usb_control_msg(...);
		if (ret > 0) {
			...
			break;
		}
	}

	if (i != 0 && ret > 0)
		dev_warn(&udev->dev, "Needed %d retries to read version\n", i);

Could this print i - 1, or use the same loop shape as __rtl_get_hw_ver(),
so both helpers report retries the same way?

> +
> +		pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> +	}
> +
> +	kfree(tmp);
> +	return pkg_det;
> +}

[ ... ]

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