RE: [PATCH net-next v4 2/8] r8152: split RTL_VER_17 into QFN68 and QFN100 package variants

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

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 2
> 
> 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).
> 
> - [High] A failed or unrecognised PLA_PKG_DET read makes an already-
>   identified RTL8159 (chip ID 0x2020) unusable.
> - [Low] The commit message states the patch splits the enum and updates
>   'every switch/case on RTL_VER_17', but it does not mention that the…
> - [Low] The new diagnostic in __rtl_get_hw_ver() uses
>   `dev_info(&udev->dev, "Unknown package %#02x\n", pkg_det)`.
> 
> --
> 
> r8152: split RTL_VER_17 into QFN68 and QFN100 package variants
> 
> This splits RTL_VER_17 into RTL_VER_17_QFN68 and RTL_VER_17_QFN100
> and updates the version switches accordingly. The package is detected by
> reading the new PLA_PKG_DET register through a new __rtl_get_pkg_det()
> helper called from __rtl_get_hw_ver() for chip ID 0x2020.
> 
> A couple of questions below.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 00b0fda306c30..9ce072c38b5b5 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -6404,8 +6419,15 @@ static int rtl8156_enable(struct r8152 *tp)
> >       set_tx_qlen(tp);
> >       rtl_set_eee_plus(tp);
> >
> > -     if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)
> > -             ocp_word_clr_bits(tp, MCU_TYPE_USB,
> USB_RX_AGGR_NUM, RX_AGGR_NUM_MASK);
> > +     switch (tp->version) {
> > +     case RTL_VER_10:
> > +     case RTL_VER_11:
> > +             break;
> > +     default:
> > +             ocp_word_clr_bits(tp, MCU_TYPE_USB,
> USB_RX_AGGR_NUM,
> > +                               RX_AGGR_NUM_MASK);
> > +             break;
> > +     }
> 
> [Severity: Low]
> This isn't a bug, but the changelog only says "update every switch/case on
> RTL_VER_17 in the driver to handle both", and this hunk does something
> else: the positive range test
> 
>         if (tp->version >= RTL_VER_12 && tp->version <= RTL_VER_17)
> 
> becomes an exclusion list, so every version other than RTL_VER_10 and
> RTL_VER_11 that reaches rtl8156_enable() now clears RX_AGGR_NUM_MASK
> by default.
> 
> With the versions rtl_ops_init() currently routes to rtl8156_enable() the
> resulting register writes are identical, and the later patch "r8152: split
> rtl8156_enable/up/down into per-chip-family functions" removes this switch
> again, so there is no behavioural change in the tree.
> 
> Could the changelog mention the inverted version gate, since it is not just an
> enum rename?
> 
> [ ... ]
> 
> > @@ -10182,9 +10211,33 @@ static int rtl_fw_init(struct r8152 *tp)
> >       return 0;
> >  }
> >
> > +static u32 __rtl_get_pkg_det(struct usb_device *udev) {
> > +     u32 pkg_det = 0;
> > +     __le32 *tmp;
> > +     int ret, i;
> > +
> > +     tmp = kmalloc_obj(*tmp);
> > +     if (!tmp)
> > +             return 0;
> > +
> > +     for (i = 0, ret = 0; i < 3 && ret != 4; i++)
> > +             ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> > +                                   RTL8152_REQ_GET_REGS,
> RTL8152_REQT_READ,
> > +                                   PLA_PKG_DET, MCU_TYPE_PLA,
> tmp,
> > +                                   sizeof(*tmp),
> > + USB_CTRL_GET_TIMEOUT);
> > +
> > +     if (ret > 0)
> > +             pkg_det = __le32_to_cpu(*tmp) & PKG_MASK;
> > +
> > +     kfree(tmp);
> > +     return pkg_det;
> > +}
> 
> [Severity: High]
> Can __rtl_get_pkg_det() report a read failure to its caller?
> 
> Both the allocation failure path and the "all three control transfers failed"
> path return 0, which is also a legal register value (PKG_DET bits [4:1] all zero),
> so the caller cannot tell an -EPIPE/-ETIMEDOUT/-ENOMEM from real strap
> data.

__rtl_get_pkg_det follows the behavior of __rtl_get_hw_ver.
Therefore, the function returns 0, which can be distinguished from one of pkg_det.

> 
> Two smaller points in the same helper:
> 
> The loop's success condition is ret == 4, but the value is consumed when ret >
> 0, so a short control transfer would be accepted and the unread bytes of the
> kmalloc'ed (non-zeroed) buffer would be folded into pkg_det.
> Should this test be ret == sizeof(*tmp), matching the loop?

Ok, I will fix it.

> 
> The version read a few lines above retries and complains with
> dev_warn("Needed %d retries to read version") when the transfer misbehaves.
> Would it be worth propagating an error here in the same spirit, e.g.
> returning the usb_control_msg() error via an out parameter?

I think adding a similar dev_warn() log is enough.

> 
> >  static u8 __rtl_get_hw_ver(struct usb_device *udev)  {
> >       u32 ocp_data = 0;
> > +     u32 pkg_det = 0;
> >       __le32 *tmp;
> >       u8 version;
> >       int ret;
> 
> [ ... ]
> 
> > @@ -10267,7 +10320,16 @@ static u8 __rtl_get_hw_ver(struct usb_device
> *udev)
> >               version = RTL_VER_16;
> >               break;
> >       case 0x2020:
> > -             version = RTL_VER_17;
> > +             pkg_det = __rtl_get_pkg_det(udev);
> > +             if (pkg_det == 0x1e || pkg_det == 0x1c) {
> > +                     version = RTL_VER_17_QFN68;
> > +             } else if (pkg_det == 0x18 || pkg_det == 0x1a) {
> > +                     version = RTL_VER_17_QFN100;
> > +             } else {
> > +                     version = RTL_VER_UNKNOWN;
> 
> [Severity: High]
> Following on from the helper above: does a single failed PLA_PKG_DET read
> make an already identified RTL8159 unusable?
> 
> The chip has been positively identified from PLA_TCR0/TCR1 as 0x2020 at this
> point, but a failed read yields pkg_det == 0, which lands in the else branch and
> becomes RTL_VER_UNKNOWN. In rtl8152_probe():
> 
>         version = rtl8152_get_version(intf);
>         if (version == RTL_VER_UNKNOWN)
>                 return -ENODEV;
> 
>         for (i = 0; i < RTL8152_PROBE_TRIES; i++) {
> 
> so the -ENODEV happens before the RTL8152_PROBE_TRIES retry loop and a
> transient control-transfer error is not retried at probe level either.
> 
> rtl8152_cfgselector_choose_configuration() has the same test:
> 
>         /* Switch the device to vendor mode, if and only if the vendor mode
>          * driver supports it.
>          */
>         if (__rtl_get_hw_ver(udev) == RTL_VER_UNKNOWN)
>                 return -ENODEV;
> 
> which leaves the device in its default CDC configuration, and
> rtl8153_ecm_probe() then treats the zero version as "r8152 does not support
> this device":
> 
> drivers/net/usb/r8153_ecm.c:
>         if (rtl8152_get_version(intf))
>                 return -ENODEV;
> 
> Before this patch chip ID 0x2020 unconditionally bound as RTL_VER_17.
> Would it make sense to keep binding when the PKG_DET read itself fails, and
> only fall back to RTL_VER_UNKNOWN for a value that was actually read?

No matter what failure occurs in __rtl_get_pkg_det, the version should 
be set as RTL_VER_UNKNOWN.

> 
> The same else branch is also taken for the twelve other encodings PKG_MASK
> = 0x1e can produce. Is rejecting the device the intended behaviour for those,
> or should one of the two packages be the default?

Rejecting the device makes sense. Making one of the two packages the default is
risky because they have different settings and firmware.

> 
> > +                     dev_info(&udev->dev, "Unknown package
> %#02x\n",
> > +                              pkg_det);
> 
> [Severity: Low]
> Does this format string produce the intended padding? The kernel
> vsnprintf() counts the "0x" emitted by the '#' flag inside the field width, so
> "%#02x" behaves like "%#x": pkg_det == 0 prints as 0x0 rather than 0x00. The
> sibling message in the default case of the same switch uses
> 
>         dev_info(&udev->dev, "Unknown version 0x%04x\n", ocp_data);
> 
> Would "%#04x" be what was meant here?
> 
> > +             }
> >               break;
> >       default:
> >               version = RTL_VER_UNKNOWN;
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-
> 1-nic_swsd%40realtek.com

Best Regards,
Chih-Kai