Re: [PATCH net-next v8 01/11] r8152: fix RTL8156/RTL8156B/RTL8157/RTL8159 up and down settings

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "r8152: fix
  RTL8156/RTL8156B/RTL8157/RTL8159 up and down settings" does not say…

--

r8152: fix RTL8156/RTL8156B/RTL8157/RTL8159 up and down settings

This commit changes the register programming in rtl8156_up() and
rtl8156_down() for the RTL8156, RTL8156B, RTL8157 and RTL8159. It touches
PLA_RCR1, USB_SPEED_OPTION, Clear_SDR, the OOB entry bits, the OOB RX size
settings and PLA_MCU_SPDWN_EN, and says the datasheet requires these.

> Several settings the datasheet requires for these chips were either
> missing or gated on the wrong version range in the shared
> rtl8156_up()/rtl8156_down():

[ ... ]

> Fixes: 195aae321c82 ("r8152: support new chips")
> Fixes: fd3c7d080df5 ("r8152: Add support for the RTL8157 hardware")
> Fixes: ad2a55b63b41 ("r8152: Add support for the RTL8159 chip")

[Severity: Low]
Could the commit message say what goes wrong for users without these
changes?

The only reason given is that the datasheet requires the settings. No
symptom is described, such as WoL or OOB wake failing, RX stalls after
resume, link problems, or suspend and power regressions.

The three Fixes: tags mean stable will pick this up. It also bundles six
independent register changes across three chip generations. Without a
stated impact, it is hard to tell whether this is a user-visible fix for
net or vendor-recommended tuning for net-next. It is also unclear which
hunk fixes which problem.

This affects backports, because 195aae321c82 predates RTL_VER_16 and
RTL_VER_17, which fd3c7d080df5 added later. In older stable trees, the
hunks that touch the version >= RTL_VER_16 and version < RTL_VER_16
checks won't apply, and neither will the RTL_VER_16/RTL_VER_17 case arms.
The RTL8156/RTL8156B parts would have to be pulled out by hand, such as
these in rtl8156_up():

    ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR1, BIT(3));
    ...
    case RTL_VER_13:
    case RTL_VER_15:
        /* Enable Clear_SDR */
        ocp_word_set_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(15));

and these in rtl8156_down():

    ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_BDC_CR, ALDPS_PROXY_MODE);

    ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_OOB_CTRL,
                      NOW_IS_OOB | DIS_MCU_CLROOB);

By comparison, the later patch in this series, "r8152: fix flow control
for RTL8156/RTL8156B/RTL8157/RTL8159", does state its impact ("they could
keep sending pause frames indefinitely").

Could the message be updated to describe the user-visible impact of each
change, so maintainers and stable backporters can judge where each one
belongs?

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