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

[Chih Kai Hsu <hsu.chih.kai@realtek.com>]

On Wednesday, October 21, 2026 8:52 AM, Kicinski wrote: 
> 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) {

Agree. But I think this issue is out of scope.

> 
> [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;
> > +                     }
> 

I will add test_bit(tp->flag, RTL8152_INACCESSIBLE) to prevent the garbage value from being used.

> [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?

Agreed that the latch is not recoverable from runtime resume. I kept it consistent
with the existing r8159_wait_backup_restore() timeout in r8159_init(), which also
latches without a reset. Queuing a reset from here without a retry budget could
loop through reset, probe, runtime suspend, resume, and timeout again.

> 
> > +
> > +                     /* 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()?

Agreed that the failure leaves EEE and ALDPS off, but this isn't new. The same
early return after rtl_phy_patch_request() has been in r8156_hw_phy_cfg() and
r8156b_hw_phy_cfg() since RTL8157 support was added, and it is shared by
the hw_phy_work, r8153b_ups_en() and r8156_ups_en() callers. r8157_ups_en()
follows the same pattern as the existing UPS paths. Fixing it means changing
hw_phy_cfg to return a status, or restoring the state on the early-return path,
for every chip, so I'd keep it out of this patch

> 
> 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);
> > +             }
> > +     }
> > +}
> 
> [ ... ]

Best Regards,
Chih Kai