Re: [PATCH net-next v5 6/9] r8152: add rtl8157_unload and rtl8157_change_mtu

[netdev-bot+sashiko]

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: add rtl8157_unload and
  rtl8157_change_mtu" has two Fixes: tags (fd3c7d080df5 for RTL8157,…

--

r8152: add rtl8157_unload and rtl8157_change_mtu

This adds unload and change_mtu callbacks that only RTL8157 and RTL8159
use. rtl8157_unload() calls r8157_power_cut_en() and always disables
interrupt mitigation. rtl8157_change_mtu() sets MTPS from the configured
MTU instead of using the fixed MTPS_JUMBO.

> 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 probably go to net and stable. The
message doesn't say what goes wrong on RTL8157/RTL8159 without the
patch, though. Could it state the user-visible symptom being fixed?

The patch changes several things.

In rtl8157_unload(), r8157_power_cut_en(tp, false) replaces
r8153_power_cut_en(tp, false). After this, unload no longer clears
PHASE2_EN in USB_POWER_CUT, and it now clears BIT(1) of USB_MISC_2:

drivers/net/usb/r8152.c: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));
	}
}

drivers/net/usb/r8152.c:r8153_power_cut_en() {
	...
	else
		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_POWER_CUT,
				  PWR_EN | PHASE2_EN);
	...
}

In rtl8157_change_mtu(), MTPS is no longer the fixed MTPS_JUMBO, which
is 12 * 1024 / 64 = 0xc0:

- At the default MTU of 1500, mtu_to_size() gives 1522, so MTPS drops
  from 0xc0 (12K) to 0xa0 (10K).
- Above about 12K, MTPS grows with the MTU.
- At max_mtu = size_to_mtu(16 * 1024), the value would be 0x100, so it
  is capped to 0xff.

max_mtu allows frames up to 16K. Did the fixed 12K MTPS make jumbo
frames between 12K and 16K fail? If so, could the message say that?

The subject also reads like a refactor rather than a fix. Could the
unload power-cut change and the MTPS change be sent as two separate
patches, each describing the failure it fixes?

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