RE: [PATCH net-next v4 7/8] r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159

[Chih Kai Hsu]

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 3 · Low: 0
> 
> 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).
> 
> - [Medium] rtl_fc_pause_pkt_en() (drivers/net/usb/r8152.c, RTL_VER_16 /
>   RTL_VER_17_QFN68 / RTL_VER_17_QFN100 branch) derives the…
> - [Medium] The changelog misstates the pre-patch state and omits the
>   impact/Fixes: information for what is a functional change to already-…
> - [Medium] In the new RTL_VER_16/RTL_VER_17_* branch of
>   rtl_fc_pause_pkt_en() (drivers/net/usb/r8152.c:6206-6252) both…
> 
> --
> 
> r8152: extract rtl_fc_pause_pkt_en() and apply it to RTL8156/8157/8159
> 
> The flow-control setup that used to live inline in r8156b_init() is moved into a
> new rtl_fc_pause_pkt_en() helper, and cases are added for RTL8156A
> (VER_10/11) and for RTL8157/RTL8159 (VER_16/17), where a per-speed
> pause-packet count is programmed into the new PLA_PAUSE_LIMIT register.
> The helper is called from r8156_init(), r8156b_init() and rtl8157_enable().
> 
> A few questions about the changelog and about the new VER_16/17 branch.
> 
> [Severity: Medium]
> 
> The changelog says:
> 
>     RTL8156A (VER_10/11) and RTL8157/RTL8159 (VER_16/17)
>     never applied any flow-control patch at all.
> 
> Is the VER_10/11 half of that accurate?  The ops table maps RTL_VER_10 and
> RTL_VER_11 to rtl8156_enable(), and rtl8156_enable() already runs the
> FC_PATCH_TASK toggle on every enable:
> 
>         ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
>         usleep_range(1000, 2000);
>         ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_TASK,
> FC_PATCH_TASK);
> 
> What VER_10/11 appears to have been missing is just the USB_FC_TIMER
> programming (CTRL_TIMER_EN | 1000 / 8) and the USB_FW_CTRL
> FLOW_CTRL_PATCH_OPT bit that the new VER_10/11 case adds.  Could the
> message be narrowed to that?

Sure. Will do.

> 
> Also, the changelog does not describe an observable symptom (RX FIFO
> overflow, drops, throughput loss under back-pressure), and there is no
> Fixes: tag even though behaviour changes on already shipping hardware:
> r8156_init() gains rtl_fc_pause_pkt_en(tp, 0) for RTL_VER_10/11 (candidate
> 195aae321c82 "r8152: support new chips"), and rtl8157_enable() gains the
> PLA_PAUSE_LIMIT programming for RTL_VER_16 and both RTL_VER_17 variants
> (candidates fd3c7d080df5, ad2a55b63b41).
> 
> Could the impact be stated and the Fixes: tag(s) added, or the wording adjusted
> so it does not read as fixing a defect?

Ok, I will modify the commit message.

> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 3473061af129a..05b2b79ab3e31 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> 
> [ ... ]
> 
> > @@ -301,6 +302,10 @@
> >  #define MTPS_JUMBO           (12 * 1024 / 64)
> >  #define MTPS_DEFAULT         (6 * 1024 / 64)
> >
> > +/* PLA_PAUSE_LIMIT */
> > +#define PAUSE_LIMIT_EN               BIT(3)
> > +#define PAUSE_LIMIT_MASK     0xf0
> > +
> >  /* PLA_RSTTALLY */
> >  #define TALLY_RESET          0x0001
> >
> > @@ -6163,6 +6168,93 @@ static void r8152b_enter_oob(struct r8152 *tp)
> >                          RCR_APM | RCR_AM | RCR_AB);  }
> >
> > +static void rtl_fc_pause_pkt_en(struct r8152 *tp, u16 speed) {
> > +     int log2_ratio, ratio;
> > +     u16 num_pause_pkts;
> > +     u32 ocp_data;
> > +
> 
> [ ... ]
> 
> > +     case RTL_VER_16:
> > +     case RTL_VER_17_QFN68:
> > +     case RTL_VER_17_QFN100:
> > +             ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> > +
> > +             num_pause_pkts = 0xa;
> > +             ratio = 10000;
> > +
> > +             if (!(speed & LINK_STATUS)) {
> > +                     dev_dbg(&tp->intf->dev, "No link\n");
> > +                     goto no_link;
> > +             } else if (speed & _10bps) {
> > +                     ratio /= 10;
> > +             } else if (speed & _100bps) {
> > +                     ratio /= 100;
> > +             } else if (speed & _1000bps) {
> > +                     ratio /= 1000;
> > +             } else if (speed & _2500bps) {
> > +                     ratio /= 2500;
> > +             } else if (speed & _5000bps) {
> > +                     ratio /= 5000;
> > +             } else if (speed & _10000bps) {
> > +                     ratio /= 10000;
> > +             } else {
> > +                     dev_err(&tp->intf->dev, "Unknown link speed\n");
> > +                     goto no_link;
> > +             }
> > +
> > +             log2_ratio = ilog2(ratio);
> > +             num_pause_pkts -= log2_ratio;
> > +
> > +             /* Round up if ratio is more than halfway to the next power
> of 2.
> > +              * Floating-point is avoided by rewriting
> > +              * ratio > 1.5 * 2^log2_ratio as
> > +              * 2 * ratio > 3 * 2^log2_ratio
> > +              */
> > +             if (2 * ratio > 3 * (1 << log2_ratio))
> > +                     num_pause_pkts--;
> > +
> 
> [Severity: Medium]
> 
> Can the values this formula produces be spelled out?  The changelog says "a
> log2-ratio formula that reproduces that table", but the table itself is nowhere in
> the patch, so the programmed encoding cannot be checked against the
> datasheet by a reviewer or by whoever touches this next.
> 
> Evaluating the arithmetic gives:
> 
>         10 Mbit/s   ratio 1000  ilog2 9  0xa - 9 = 1, round-up hits -> 0
>         100 Mbit/s  ratio 100   ilog2 6  0xa - 6 = 4, round-up hits -> 3
>         1 Gbit/s    ratio 10    ilog2 3  0xa - 3 = 7, no round-up   -> 7
>         2.5 Gbit/s  ratio 4     ilog2 2  0xa - 2 = 8, no round-up   -> 8
>         5 Gbit/s    ratio 2     ilog2 1  0xa - 1 = 9, no round-up   -> 9
>         10 Gbit/s   ratio 1     ilog2 0  0xa - 0 = 10               -> 10
> 
> Is num_pause_pkts == 0 at 10 Mbit/s the intended encoding?  The write below
> clears PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN, ORs in 0 << 4, and then
> PAUSE_LIMIT_EN is set again, so the limiter is armed with a count field of zero.
> PLA_PAUSE_LIMIT was not written at all for these parts before this patch, so if
> zero means "no pause frames" this changes 10 Mbit/s behaviour.

The number of pause packets to be sent is 2^num_pause_pkts rather than num_pause_pkts.
Therefore, the device will send 1 pause packet at 10Mbit/s.

I will clarify this in the next version.

> 
> Would a plain switch or lookup table (or the datasheet table quoted in a
> comment) make the per-speed values auditable, and can the 10 Mbit/s
> boundary value be confirmed against the datasheet?

I will quote the table in a comment.

> 
> > +no_link:
> > +             ocp_word_w0w1(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> > +                           PAUSE_LIMIT_MASK | PAUSE_LIMIT_EN,
> > +                           num_pause_pkts << 4);
> > +
> > +             ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_PAUSE_LIMIT,
> > +                               PAUSE_LIMIT_EN);
> > +             break;
> 
> [Severity: Medium]
> 
> Both fallback paths reach this label with num_pause_pkts still at its initialiser
> 0xa, which is the 10 Gbit/s entry, and the label then writes that count and sets
> PAUSE_LIMIT_EN.  Should the no-link and unknown-speed cases instead leave
> PLA_PAUSE_LIMIT alone (the pre-patch behaviour), or clear PAUSE_LIMIT_EN?
> 
> The name no_link and the dev_dbg("No link") read as if nothing is programmed,
> but the limiter ends up armed with the fastest link's pause count while the MAC
> may be running at 10 or 100 Mbit/s, where the intended counts are 0 and 3.
> 
> How is the no-link path reached?  rtl8157_enable() re-reads PLA_PHYSTATUS
> after the caller already tested LINK_STATUS:
> 
> set_carrier()
>         speed = rtl8152_get_speed(tp);
>         if (speed & LINK_STATUS)
>                 ... tp->rtl_ops.enable(tp);
> 
> rtl8157_enable()
>         speed = rtl8152_get_speed(tp);
>         rtl_fc_pause_pkt_en(tp, speed);
> 
> A link drop between the two reads takes the !(speed & LINK_STATUS) arm.
> Since ->enable() only runs again on an observed carrier down/up transition, and
> nothing else re-programs PLA_PAUSE_LIMIT, can a later low-speed link then run
> for the whole session with the 10 Gbit/s pause budget?

Actually, I should drop the no-link path because it will never be reached.

> 
> Separately, the decode chain covers only _10bps, _100bps, _1000bps,
> _2500bps, _5000bps and _10000bps, while this driver's own enum
> rtl_register_content also defines:
> 
>         _2500bps        = BIT(10),
>         _1250bps        = BIT(9),
>         _500bps         = BIT(8),
> 
> and rtl8156_enable() treats _500bps as a real link state:
> 
>         if (speed & _1000bps)
>                 ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS,
> 0x11);
>         else if (speed & _500bps)
>                 ocp_write_word(tp, MCU_TYPE_PLA, PLA_EEE_TXTWSYS,
> 0x3d);
> 
> If RTL8157/RTL8159 can report one of those, this lands in the final else, emits
> dev_err("Unknown link speed") at KERN_ERR on every ops->enable() call
> (link-up, ndo_open, change_mtu, set_coalesce), and still arms the
> 10 Gbit/s limit.  Should the remaining encodings be decoded here too?

Yes, RTL8157/RTL8159 are capable of reporting _500bps and _1250bps in hardware. 
However, since the Linux kernel's EEE link-mode bitmask has no encoding for 
these 500/1250 Mbit/s sub-rates, the driver never advertises them, so link negotiation 
can never converge on that state — these bits should never actually be set.

That said, I agree the current else branch's side effect of unconditionally arming the 
10 Gbit/s pause-frame limit is a real concern regardless. Rather than adding explicit 
decode branches for states that can't occur, I think it's more robust to clear the 
PAUSE_LIMIT_EN bit so that even if this branch were ever reached, it wouldn't 
silently apply an incorrect flow-control limit.

> 
> > +     default:
> > +             break;
> > +     }
> > +}
> > +
> >  static int r8153_pre_firmware_1(struct r8152 *tp)  {
> >       int i;
> > @@ -6694,6 +6786,8 @@ static int rtl8157_enable(struct r8152 *tp)
> >       r8153_set_rx_early_size(tp);
> >
> >       speed = rtl8152_get_speed(tp);
> > +     rtl_fc_pause_pkt_en(tp, speed);
> > +
> >       rtl_set_ifg(tp, speed);
> >
> >       return rtl_enable(tp);
> > @@ -8842,6 +8936,8 @@ static void r8156_init(struct r8152 *tp)
> >
> >       usb_enable_lpm(tp->udev);
> >
> > +     rtl_fc_pause_pkt_en(tp, 0);
> > +
> >       r8156_mac_clk_spd(tp, true);
> >
> 
> [ ... ]
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917070637.62827-1
> -nic_swsd%40realtek.com

Best Regards,
Chih-Kai