RE: [PATCH net-next v7 01/10] r8152: split r8156_init by generation and factor out wait_autoload_done

[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: split r8156_init by generation and factor out wait_autoload_done
> 
> The patch splits r8156_init() into two functions. r8156_init() now covers
> RTL8156/RTL8156B, and the new r8157_init() covers RTL8157/RTL8159. It
> also moves the repeated PLA_BOOT_CTRL AUTOLOAD_DONE polling loop into
> a new
> wait_autoload_done() helper built on read_poll_timeout(), and converts the
> r8153 init and ups_en paths to use it.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > af3d7dcb2f141..518a2b42c2200 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -4172,6 +4172,23 @@ static u16 r8153_phy_status(struct r8152 *tp,
> u16 desired)
> >       return data;
> >  }
> >
> > +static int wait_autoload_done(struct r8152 *tp) {
> > +     u16 ocp_data;
> > +     int ret;
> > +
> > +     ret = read_poll_timeout(ocp_read_word, ocp_data,
> > +                             test_bit(RTL8152_INACCESSIBLE,
> &tp->flags) ||
> > +                             (ocp_data & AUTOLOAD_DONE),
> > +                             20000, 10 * USEC_PER_SEC, false, tp,
> > +                             MCU_TYPE_PLA, PLA_BOOT_CTRL);
> > +
> > +     if (ret)
> > +             dev_err(&tp->intf->dev, "autoload done timeout\n");
> > +
> > +     return test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : 0;
> > +}
> > +
> 
> [Severity: Medium]
> Can this end up reading uninitialized stack memory in ocp_read_word()?
> 
> read_poll_timeout() runs op before it evaluates cond, on the first iteration,
> after every sleep, and once more after the timeout expires.
> So once RTL8152_INACCESSIBLE gets set, the helper still issues another
> ocp_read_word() on PLA_BOOT_CTRL before the test_bit() in the condition
> gets a chance to stop the loop.
> 
> In that case generic_ocp_read() returns -ENODEV without writing to the local
> tmp. ocp_read_word() ignores that return value and still does
> __le32_to_cpu(tmp), then hands the result back as ocp_data.
> 
> The loops being replaced did not do this. r8153b_ups_en() and
> r8153c_ups_en() tested the flag before each read. The init functions tested it
> right after msleep(20) and left the loop before the next read.
> 
> The test_bit() short-circuit means the garbage is never acted on, but the read
> itself is still undefined behaviour of the sort KMSAN is meant to catch, and it
> now happens whenever the device goes away while we are waiting for
> AUTOLOAD_DONE.
> 
> A flag check before the poll would only cover the entry case, since the flag can
> be set during the sleep. Would it be better to initialize tmp in ocp_read_word()
> and the other ocp_read_*() helpers, given that plenty of existing callers
> already read without checking the flag?
> Or is an open-coded loop that tests the flag first preferable here?

Before testing AUTOLOAD_DONE in PLA_BOOT_CTRL, the RTL8152_INACCESSIBLE flag
was tested. Therefore, the garbage is never used to check AUTOLOAD_DONE.

KMSAN reports an uninitialized value only when it is used; the reads wouldn't trigger it.

> 
> [ ... ]
> 
> > @@ -7225,22 +7226,14 @@ static void r8152b_init(struct r8152 *tp)
> > static void r8153_init(struct r8152 *tp)  {
> >       u32 ocp_data;
> > -     int i;
> >
> >       if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> >               return;
> >
> >       r8153_u1u2en(tp, false);
> >
> > -     for (i = 0; i < 500; i++) {
> > -             if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL)
> &
> > -                 AUTOLOAD_DONE)
> > -                     break;
> > -
> > -             msleep(20);
> > -             if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > -                     break;
> > -     }
> > +     if (wait_autoload_done(tp))
> > +             return;
> >
> >       r8153_phy_status(tp, 0);
> >
> > @@ -7341,22 +7334,13 @@ static void r8153_init(struct r8152 *tp)
> >
> >  static void r8153b_init(struct r8152 *tp)  {
> > -     int i;
> > -
> >       if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> >               return;
> >
> >       r8153b_u1u2en(tp, false);
> >
> > -     for (i = 0; i < 500; i++) {
> > -             if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL)
> &
> > -                 AUTOLOAD_DONE)
> > -                     break;
> > -
> > -             msleep(20);
> > -             if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > -                     break;
> > -     }
> > +     if (wait_autoload_done(tp))
> > +             return;
> >
> >       r8153_phy_status(tp, 0);
> >
> 
> [Severity: Medium]
> Is the commit message accurate for r8153_init() and r8153b_init()? It
> says:
> 
>   a plain AUTOLOAD_DONE timeout is logged but otherwise treated as
>   success, same as every one of the loops it replaces, which only bailed
>   out early on RTL8152_INACCESSIBLE and otherwise fell through to the
>   rest of the init sequence.
> 
> and:
> 
>   No functional change intended.
> 
> In these two functions the old loops did a break, not a return, when
> RTL8152_INACCESSIBLE was set:
> 
>                 msleep(20);
>                 if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>                         break;
> 
> So init always ran to the end of the function. Only r8153c_init(), the old
> r8156_init() and the two *_ups_en() loops returned early.
> 
> wait_autoload_done() returns -ENODEV whenever RTL8152_INACCESSIBLE is
> set, so both functions now return early instead.
> 
> The commit message says the early return costs nothing because
> generic_ocp_read()/generic_ocp_write() short-circuit. However, the skipped
> tail also does work that does not go through OCP accesses:
> 
>   r8153_init():
>         usb_enable_lpm(tp->udev);
>         ...
>         tp->coalesce = COALESCE_SUPER;   (or COALESCE_HIGH /
> COALESCE_SLOW)
> 
>   r8153b_init():
>         usb_enable_lpm(tp->udev);
>         ...
>         set_bit(GREEN_ETHERNET, &tp->flags);
>         ...
>         tp->coalesce = 15000;
> 
> usb_enable_lpm() is a counted USB core call. It decrements
> udev->lpm_disable_count and does not check the driver's
> RTL8152_INACCESSIBLE flag.
> 
> The polling interval also changes. The old loops used msleep(20), while
> read_poll_timeout() with a sleep_us of 20000 ends up in usleep_range(5001,
> 20000).
> 
> These differences only show up on error paths. Still, could the commit message
> describe the new early return in r8153_init() and r8153b_init()?
> Or should these two functions keep the old break-and-continue behaviour?

I should keep the original behavior, remove the early return in r8153_init() and r8153b_init().

> --
> pw-bot: cr

Best Regards,
Chih Kai