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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: enable UPS for RTL8157 and RTL8159 QFN68

This patch adds r8157_ups_en() and calls it from rtl8157_runtime_enable(),
so UPS is now switched on and off during runtime suspend and resume for
RTL_VER_16 and RTL_VER_17_QFN68. It also adds per-speed EEE flags and
5G/10G speed encodings to r8156_ups_flags(), and skips the lite_mode
encoding for those chips.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 95f510da376de..7a1b34db1d88b 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> @@ -4484,6 +4522,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);

[Severity: Medium]
This is a pre-existing issue, but this patch adds more callers of the
ocp_byte_w0w1() read/modify/write path, and that path depends on
get_registers():

drivers/net/usb/r8152.c:get_registers() {
    ...
	if (ret < 0)
		memset(data, 0xff, size);
	else
		memcpy(data, tmp, size);
    ...
}

usb_control_msg() accepts short IN transfers. Could a malicious device
reply with fewer than size bytes and leave stale kmalloc() data in the
part of tmp it never wrote?

ocp_byte_w0w1() then writes that byte back with only the masked bits
changed:

	ocp_data = ocp_read_byte(tp, type, index);
	ocp_data = (ocp_data & ~clear) | set;
	ocp_write_byte(tp, type, index, ocp_data);

That would let the device read back up to 8 bits of stale heap data per
read/modify/write. This does not happen with init_on_alloc. Every
read/modify/write in the driver has the same pattern, and this patch does
not change get_registers().

> +
> +		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) {

[Severity: Low]
This isn't a bug introduced by this patch, but ocp_read_word() ignores
the return value of generic_ocp_read() and uses its local tmp anyway:

drivers/net/usb/r8152.c:ocp_read_word() {
	__le32 tmp;
    ...
	generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);

	data = __le32_to_cpu(tmp);
    ...
}

If RTL8152_INACCESSIBLE is already set, generic_ocp_read() returns
-ENODEV without writing the buffer. The same happens if kmalloc() fails
in get_registers(). Would the PCUT_STATUS check here, and the
BACKUP_RESTRORE poll below, then test an uninitialized stack value?

When the device is inaccessible, the writes that follow do nothing, so
the most likely visible effect is a KMSAN report. Every ocp_read_*
caller is affected.

> +			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]
Can this leave the interface unusable after a runtime resume, with
nothing to recover it?

With this patch, runtime suspend on RTL_VER_16 and RTL_VER_17_QFN68
enables UPS power cut by default. That means any runtime resume that
finds PCUT_STATUS set will run this wait:

rtl8152_runtime_resume()
  tp->rtl_ops.autosuspend_en(tp, false)
    rtl8157_runtime_enable(tp, false)
      r8157_ups_en(tp, false)
        read_poll_timeout() times out
        rtl_set_inaccessible(tp)

r8157_ups_en() returns void, so rtl8157_runtime_enable() carries on, and
r8157_u2p3en() and r8153b_u1u2en() quietly do nothing.
rtl8152_runtime_resume() then runs napi_disable()/napi_enable(),
rtl8152_get_speed() and usb_submit_urb(), and returns 0.

rtl8152_resume() has a reset fallback, but it is skipped for runtime
resume:

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

Once the flag is set, r8152_control_msg() also returns early:

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

This happens before it gets to the usb_queue_reset_device() logic, so
later register accesses cannot start a recovery either.

The netdev stays registered and up while every hardware access is
skipped. That lasts until a replug, a system suspend/resume, or an
unrelated reset. Before this patch, rtl8157_runtime_enable() had no such
wait.

The commit message mentions this and defers recovery to a follow-up
series. Since this path is on by default for these chips, should this
patch at least queue a USB reset here (for example with
usb_queue_reset_device(tp->intf)) or pass the error back?

> +
> +			/* clear USB fw_ver_reg */
> +			ocp_write_byte(tp, MCU_TYPE_USB, USB_FW_USB_VER, 0);
> +
> +			tp->rtl_ops.hw_phy_cfg(tp);

[Severity: Low]
This isn't a bug introduced by this patch, but r8157_hw_phy_cfg() can
return early after it has already turned EEE and ALDPS off:

drivers/net/usb/r8152.c:r8157_hw_phy_cfg() {
    ...
	r8153_aldps_en(tp, false);
	rtl_eee_enable(tp, false);
    ...
	if (rtl_phy_patch_request(tp, true, true))
		return;
    ...
}

If the PHY patch handshake times out (for example -ETIME when
PATCH_READY never appears), the code skips re-enabling EEE and ALDPS and
skips set_bit(PHY_RESET). By then r8153_eee_en() has already cleared
tp->ups_info.eee.

hw_phy_cfg returns void, so r8157_ups_en() goes straight on to
rtl8152_set_speed() and runtime resume reports success. Does EEE and
ALDPS then stay off until the PHY is reconfigured? Would the next UPS
suspend also leave out the new per-speed EEE flags in r8156_ups_flags()?

The hw_phy_work path and r8153b_ups_en()/r8156_ups_en() share the same
early return. This patch adds one more caller.

> +
> +			rtl8152_set_speed(tp, tp->autoneg, tp->speed,
> +					  tp->duplex, tp->advertising);
> +		}
> +	}
> +}

[ ... ]