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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 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] If the new backup-restore poll in r8157_ups_en(tp, false)
  times out during runtime resume on RTL_VER_16 or RTL_VER_17_QFN68, the…

--

r8152: enable UPS for RTL8157 and RTL8159 QFN68

This patch adds r8157_ups_en() and calls it from rtl8157_runtime_enable()
for RTL_VER_16 and RTL_VER_17_QFN68. When UPS is disabled and PCUT_STATUS
is set, the new disable path polls for backup-restore to finish. It also
extends r8156_ups_flags() with per-speed EEE flags and the 5G/10G speed
encodings.

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

[ ... ]

> @@ -4486,6 +4524,49 @@ static void r8156_ups_en(struct r8152 *tp, bool enable)
>  	}
>  }
>  
> +static void r8157_ups_en(struct r8152 *tp, bool enable)
> +{
> +	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);
> +
> +		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
> +			u32 ocp_data;
> +			int ret;
> +
> +			ret = read_poll_timeout(ocp_read_word, ocp_data,
> +						ocp_data & BACKUP_RESTRORE, 200,
> +						20000, false, tp, MCU_TYPE_USB,
> +						USB_GPHY_CTRL);
> +			if (ret) {
> +				rtl_set_inaccessible(tp);
> +				dev_err(&tp->intf->dev,
> +					"backup-restore timed out\n");
> +				return;
> +			}

[Severity: Medium]
If this read_poll_timeout() times out during a runtime resume on
RTL_VER_16 or RTL_VER_17_QFN68, does anything recover the device?

The path looks like this:

rtl8152_resume()
  rtl8152_runtime_resume()
    tp->rtl_ops.autosuspend_en(tp, false)
      rtl8157_runtime_enable(tp, false)
        r8157_ups_en(tp, false)
          rtl_set_inaccessible(tp)

r8157_ups_en() returns void, so rtl8152_runtime_resume() keeps going and
returns 0. The reset fallback in rtl8152_resume() skips the runtime case:

	if (!runtime_resume && test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		usb_reset_device(tp->udev);

The register reads in the poll succeeded, so r8152_control_msg() never
queued a reset. After that, every access returns early before
r8152_control_msg() gets to its reset logic:

	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		return -ENODEV;

r8152_submit_rx(), the interrupt callback and bottom_half() also return
early while the flag is set.

Would the interface then stay unusable until some unrelated reset, such
as rtl8152_tx_timeout() when TX traffic stalls, or a replug? An idle
interface would get neither.

Before this patch these chips never entered UPS on runtime suspend, so
this resume failure looks new.

Could this path queue a reset, or otherwise pass the error back, in the
runtime resume case?

> +
> +			/* clear USB fw_ver_reg */
> +			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USB_VER, 0);
> +
> +			tp->rtl_ops.hw_phy_cfg(tp);
> +
> +			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> +					  tp->duplex, tp->advertising);
> +		}
> +	}
> +}

[ ... ]

> @@ -4644,9 +4725,28 @@ static void rtl8157_runtime_enable(struct r8152 *tp, bool enable)

[ ... ]

>  	} else {
>  		r8153_queue_wake(tp, false);
>  		rtl_runtime_suspend_enable(tp, false);
> +
> +		switch (tp->version) {
> +		case RTL_VER_16:
> +		case RTL_VER_17_QFN68:
> +			r8157_ups_en(tp, false);
> +			break;
> +		default:
> +			break;
> +		}
> +
>  		r8157_u2p3en(tp, true);
>  		if (tp->udev->speed >= USB_SPEED_SUPER)
>  			r8153b_u1u2en(tp, true);
>  	}

[ ... ]

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