Re: [PATCH net-next v6 07/10] r8152: add rtl8157_unload and rtl8157_change_mtu

[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 carries two Fixes: tags (fd3c7d080df5 for
  RTL8157 and ad2a55b63b41 for RTL8159), so it will be picked up for…

--

r8152: add rtl8157_unload and rtl8157_change_mtu

This patch adds two RTL8157/RTL8159-specific helpers. rtl8157_unload()
uses r8157_power_cut_en() and clears the 0xcf04 interrupt mitigation
bits. rtl8157_change_mtu() scales MTPS from the configured MTU instead of
writing a fixed MTPS_JUMBO. Both helpers replace the earlier
rtl8153_unload() and rtl8156_change_mtu() ops for RTL_VER_16 and
RTL_VER_17_QFN68/QFN100.

> r8152: add rtl8157_unload and rtl8157_change_mtu
>
> RTL8157 and RTL8159 used rtl8153_unload() and rtl8156_change_mtu(),
> which program power-cut and MTPS the way earlier chips require. Per
> the datasheet, RTL8157/RTL8159 need r8157_power_cut_en() (already
> used by r8157_init()/r8159_init()) instead of r8153_power_cut_en(),
> and an MTPS scaled from the configured MTU (10K-16K, max_pkt_size /
> 64 capped at 0xff) instead of a fixed MTPS_JUMBO.

[ ... ]

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

[Severity: Low]

With two Fixes: tags this will likely be picked up for stable. The only
reason the changelog gives is "Per the datasheet". What do users actually
see go wrong on RTL8157/RTL8159 with the old unload sequence or the fixed
MTPS? Could the changelog describe that symptom?

The unload change is not just a rename. Compared with
r8153_power_cut_en(tp, false), the new path no longer clears PHASE2_EN,
and it now also clears USB_MISC_2 BIT(1):

r8157_power_cut_en() {
    ...
	} else {
		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));
	}
}

On the MTPS side, RTL8156B (RTL_VER_12/13/15) uses the same
max_mtu = size_to_mtu(16 * 1024) as RTL_VER_16/17. It still goes through
rtl8156_change_mtu(), which writes the fixed value:

rtl8156_change_mtu() {
    ...
	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RMS, rx_max_size);
	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_MTPS, MTPS_JUMBO);
    ...
}

So the commit text alone does not show what MTPS_JUMBO breaks on
RTL8157/RTL8159.

The subject also reads like a feature addition rather than a fix.

The unload power-cut sequence and the MTPS change are independent. Would
it be better to split them into two patches, each with its own symptom
and Fixes: tags? That way each change could be backported or reverted on
its own.

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