Re: [PATCH net-next v8 02/11] r8152: add rtl8157_unload() for RTL8157 and RTL8159

[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 commit message for "r8152: add rtl8157_unload() for RTL8157
  and RTL8159" does not say what goes wrong for users.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but older
  sibling chips have the same init/unload mismatch the patch removes for…

--

r8152: add rtl8157_unload() for RTL8157 and RTL8159

This patch adds rtl8157_unload() for RTL8157 and RTL8159. On unload,
power cut is now disabled with r8157_power_cut_en() instead of
r8153_power_cut_en(). The Interrupt Mitigation clear of 0xcf04 also moves
out of rtl8153_unload() and into the new helper.

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

[ ... ]

> @@ -9711,6 +9706,16 @@ static void rtl8153b_unload(struct r8152 *tp)
>  	r8153b_power_cut_en(tp, false);
>  }
>  
> +static void rtl8157_unload(struct r8152 *tp)
> +{
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +		return;
> +
> +	r8157_power_cut_en(tp, false);

[Severity: Low]

Could the commit message say what goes wrong for users with the old
sequence? The patch has two Fixes: tags (fd3c7d080df5 and ad2a55b63b41),
but the only reason given is:

  Per the datasheet they need r8157_power_cut_en(), which r8156_init()
  already calls for them.

At unbind, the two helpers change different registers. The old path does:

r8153_power_cut_en(tp, false):
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
			  PWR_EN | PHASE2_EN);
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);

The new path does:

r8157_power_cut_en(tp, false):
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN);
	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2, BIT(1));

With this patch, unload no longer clears PHASE2_EN in USB_POWER_CUT, and
it now clears BIT(1) in USB_MISC_2. PHASE2_EN can be set at this point,
because r8156_ups_en(tp, true) sets it.

This runs from rtl8152_disconnect() and from the probe error path, but
only while RTL8152_INACCESSIBLE is clear. In practice that means unbind
or rmmod with the adapter still attached, or a probe failure.

Did the old sequence cause a visible problem, such as a failed re-bind,
a power or wake issue, or a stuck device? If so, could the commit
message describe it along with the register change above?

> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
> +			  BIT(0) | BIT(1) | BIT(2) | BIT(7));
> +}
> +

[ ... ]

> @@ -9889,7 +9894,7 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->disable		= rtl8153_disable;
>  		ops->up			= rtl8156_up;
>  		ops->down		= rtl8156_down;
> -		ops->unload		= rtl8153_unload;
> +		ops->unload		= rtl8157_unload;

[Severity: Medium]

This isn't a bug introduced by this patch, but do the older sibling
chips have the same init/unload mismatch that this patch removes for
RTL8157 and RTL8159?

In rtl_opsx_init(), RTL_VER_10/11 (RTL8156), RTL_VER_12/13/15 (RTL8156B)
and RTL_VER_14 (RTL8153C) still use rtl8153_unload():

	case RTL_VER_10:
		ops->init		= r8156_init;
		...
		ops->unload		= rtl8153_unload;

	case RTL_VER_14:
		ops->init		= r8153c_init;
		...
		ops->unload		= rtl8153_unload;

rtl8153_unload() calls r8153_power_cut_en(tp, false), which clears both
PWR_EN and PHASE2_EN. The init and down paths for these chips use
r8153b_power_cut_en(tp, false) instead, which clears only PWR_EN:

r8156_init():
	if (tp->version >= RTL_VER_16)
		r8157_power_cut_en(tp, false);
	else
		r8153b_power_cut_en(tp, false);

rtl8156_down():
	if (tp->version < RTL_VER_16) {
		r8153_u2p3en(tp, false);
		r8153b_power_cut_en(tp, false);
	}

r8153c_init():
	r8153b_power_cut_en(tp, false);

These assignments go back to 195aae321c82 ("r8152: support RTL8156").
They also look unchanged later in this series, at 20694877fe24.

It isn't clear that clearing PHASE2_EN at unbind actually causes a
problem on these chips. Do the RTL8156, RTL8156B and RTL8153C datasheets
have the same requirement as RTL8157 and RTL8159?

>  		ops->eee_get		= r8153_get_eee;
>  		ops->eee_set		= r8152_set_eee;
>  		ops->in_nway		= rtl8153_in_nway;

[ ... ]

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