RE: [PATCH net-next v3 5/8] r8152: support rtl8157_unload and rtl8157_change_mtu

[Chih Kai Hsu]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
> 
> - [Low] Commit message understates the impact of the change and carries
>   no Fixes: tag, even though it silently alters the register programming…
> 
> --
> 
> r8152: support rtl8157_unload and rtl8157_change_mtu
> 
> This adds rtl8157_unload() and rtl8157_change_mtu() and wires them into the
> RTL_VER_16 (RTL8157) and RTL_VER_17 (RTL8159) op tables, replacing
> rtl8153_unload() and rtl8156_change_mtu().  The interrupt mitigation clear
> of USB register 0xcf04 is moved out of rtl8153_unload() into the new callback.
> 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> > 679aead731f73..06fad895fce09 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -7024,6 +7024,28 @@ static void rtl8156_down(struct r8152 *tp)
> >       r8153_aldps_en(tp, true);
> >  }
> >
> > +static void rtl8157_change_mtu(struct r8152 *tp) {
> > +     u32 max_pkt_size = mtu_to_size(tp->netdev->mtu);
> > +     u32 ocp_data;
> > +
> > +     ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, max_pkt_size);
> > +
> > +     /* Use at least 10K for MTPS */
> > +     ocp_data = max_t(u32, max_pkt_size, 10 * 1024) / 64;
> > +
> > +     /* 16 * 1024 / 64 = 0x100, so the max is 0xff for 8 bits data */
> > +     ocp_data = min_t(u32, ocp_data, 0xff);
> > +
> > +     ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, ocp_data);
> > +     r8156_fc_parameter(tp);
> 
> [ ... ]
> 
> > @@ -10174,11 +10196,6 @@ static void rtl8153_unload(struct r8152 *tp)
> >               return;
> >
> >       r8153_power_cut_en(tp, false);
> > -
> > -     if (tp->version >= RTL_VER_16) {
> > -             /* Disable Interrupt Mitigation */
> > -             ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) | BIT(1)
> | BIT(2) | BIT(7));
> > -     }
> >  }
> 
> [ ... ]
> 
> > +static void rtl8157_unload(struct r8152 *tp) {
> > +     if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> > +             return;
> > +
> > +     r8157_power_cut_en(tp, false);
> > +     ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04,
> > +                       BIT(0) | BIT(1) | BIT(2) | BIT(7)); }
> 
> [Severity: Low]
> Could the changelog say what misbehaves today on these chips, and carry
> Fixes: tags?  RTL_VER_16 and RTL_VER_17 are already supported in-tree
> (added by fd3c7d080df531 and ad2a55b63b41af), and both assigned
> ops->unload = rtl8153_unload and ops->change_mtu = rtl8156_change_mtu,
> ops->so
> this changes the register programming of shipped hardware rather than
> enabling something new.
> 
> The commit message says the dedicated unload is needed "to disable interrupt
> mitigation", but rtl8153_unload() already did that for
> tp->version >= RTL_VER_16:
> 
>         if (tp->version >= RTL_VER_16) {
>                 /* Disable Interrupt Mitigation */
>                 ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xcf04, BIT(0) |
> BIT(1) | BIT(2) | BIT(7));
>         }
> 
> so that part is only relocated.  The behaviour that actually changes is the
> power cut disable sequence:
> 
> r8153_power_cut_en(tp, false):
>         ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN
> | PHASE2_EN);
>         ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
> 
> r8157_power_cut_en(tp, false):
>         ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT, PWR_EN);
>         ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_MISC_0, PCUT_STATUS);
>         ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_MISC_2, BIT(1));
> 
> PHASE2_EN is now left set on the unload/disconnect path for these chips.
> Is that intentional, and is there an observable symptom with the old sequence?

It's intentional. RTL8157 and RTL8159 have different sequences to disable power cut.

> 
> Similarly, rtl8157_change_mtu() replaces the fixed MTPS_JUMBO (12 * 1024 /
> 64 = 0xc0) that rtl8156_change_mtu() programmed with an MTU derived value:
> at the default 1500 byte MTU, max_t(u32, mtu_to_size(1500), 10 * 1024) / 64
> gives 0xa0, and it saturates at 0xff for a 16K MTU.  Both paths are live at
> runtime, via rtl8157_up() on every bring-up and via rtl8152_change_mtu() ->
> tp->rtl_ops.change_mtu() on every MTU change.  What does the old MTPS
> tp->value
> break on these chips?

RTL8157 and RTL8159 support a new formula to compute PLA_MTPS.

> 
> As written it is hard to tell whether this is net-next tuning or a fix for a user
> visible defect on released kernels, and there is nothing for stable backports to
> key on.  Note that this wording is unchanged at the end of the series, so no
> later patch adds the missing justification.
> 
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908075634.23740-1
> -nic_swsd%40realtek.com

Best,
Chih-Kai