Re: [PATCH net-next v8 11/11] r8152: enable UPS for RTL8157 and RTL8159 QFN68

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] r8152: unrecoverable RTL8152_INACCESSIBLE latch in
  r8157_ups_en()
--

commit be4db4d81b51525250cdaed81bde5b818204e21b
Author: Chih Kai Hsu <hsu.chih.kai@realtek.com>

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This commit adds r8157_ups_en() and updates r8156_ups_flags() and
rtl8157_runtime_enable() to enable UPS during runtime suspend/resume on
RTL8157 and RTL8159 QFN68.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 703ec5483778b..9c7031f938bc2 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4496,6 +4534,58 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
>  	}
>  }
>  
> +static void r8157_ups_en(struct r8152 *tp, bool enable)
> +{
> +	u32 ocp_data;
> +	int ret;
> +
> +	if (enable) {
> +		r8156_ups_flags(tp);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE | PHASE2_EN);
> +
> +		ocp_byte_set_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +	} else {
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
> +				  UPS_EN | USP_PREWAKE);
> +
> +		ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2,
> +				  UPS_FORCE_PWR_DOWN);
> +
> +		ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
> +		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +			return;
> +
> +		if (ocp_data & PCUT_STATUS) {
> +			ret = read_poll_timeout(ocp_read_word, ocp_data,
> +						test_bit(RTL8152_INACCESSIBLE,
> +							 &tp->flags) ||
> +						(ocp_data & BACKUP_RESTRORE),
> +						200, 20000, false, tp,
> +						MCU_TYPE_USB, USB_GPHY_CTRL);
> +			if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +				return;
> +
> +			if (ret) {
> +				rtl_set_inaccessible(tp);
> +				dev_err(&tp->intf->dev,
> +					"backup-restore timed out\n");
> +				return;
> +			}

[Severity: Medium]
As reported by Jakub Kicinski on v7 [1], does setting RTL8152_INACCESSIBLE
here without queuing a USB reset or returning an error leave the interface
permanently dead on runtime resume?

When rtl8152_runtime_resume() -> rtl8157_runtime_enable(tp, false) ->
r8157_ups_en(tp, false) times out waiting for BACKUP_RESTRORE,
r8157_ups_en() returns void and rtl8152_runtime_resume() returns 0, while
rtl8152_resume() skips its usb_reset_device() fallback when runtime_resume
is true:

drivers/net/usb/r8152.c:rtl8152_resume() {
    ...
	if (!runtime_resume && test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		usb_reset_device(tp->udev);
    ...
}

Because RTL8152_INACCESSIBLE causes subsequent r8152_control_msg() calls to
bail out with -ENODEV before reaching usb_queue_reset_device(), and
rtl_set_accessible() only runs in rtl8152_post_reset(), nothing recovers the
device until a physical replug or system suspend/resume.

[1] https://lore.kernel.org/r/20261021005214.1255071-1-kuba@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110032416.16968-1-nic_swsd@realtek.com?part=11