Re: [PATCH net-next v2 5/8] r8152: support rtl8157_unload and rtl8157_change_mtu

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The commit is presented as adding new callback "support" for
  RTL8157/RTL8159, but it silently changes runtime register programming…

--

r8152: support rtl8157_unload and rtl8157_change_mtu

This adds rtl8157_unload(), which uses r8157_power_cut_en() and disables
interrupt mitigation, and rtl8157_change_mtu(), which scales PLA_MTPS with
the configured MTU.  Both are wired up for RTL_VER_16 and
RTL_VER_17_QFN68/QFN100 in rtl_ops_init(), replacing rtl8153_unload and
rtl8156_change_mtu.

(no Link: tags in the commit header)

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 679aead731f7..06fad895fce0 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);

[ ... ]

> @@ -10189,6 +10206,16 @@ static void rtl8153b_unload(struct r8152 *tp)
>  	r8153b_power_cut_en(tp, false);
>  }
>  
> +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));
> +}
> +

[ ... ]

> @@ -10367,13 +10394,13 @@ static int rtl_ops_init(struct r8152 *tp)
>  		ops->disable		= rtl8153_disable;
>  		ops->up			= rtl8157_up;
>  		ops->down		= rtl8157_down;
> -		ops->unload		= rtl8153_unload;
> +		ops->unload		= rtl8157_unload;
>  		ops->eee_get		= r8153_get_eee;
>  		ops->eee_set		= r8152_set_eee;
>  		ops->in_nway		= rtl8153_in_nway;
>  		ops->hw_phy_cfg		= r8157_hw_phy_cfg;
>  		ops->autosuspend_en	= rtl8157_runtime_enable;
> -		ops->change_mtu		= rtl8156_change_mtu;
> +		ops->change_mtu		= rtl8157_change_mtu;

[Severity: Medium]

The subject and changelog describe this as adding "support", but these two
ops assignments change register programming for hardware that released
kernels already drive.  RTL8157 (RTL_VER_16) was enabled by fd3c7d080df5
("r8152: Add support for the RTL8157 hardware") and RTL8159 (RTL_VER_17) by
ad2a55b63b41 ("r8152: Add support for the RTL8159 chip"), and the baseline
already assigns rtl8153_unload / rtl8156_change_mtu for both.

For the unload path, the old and new helpers are not equivalent:

	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));

So PHASE2_EN is now left set and USB_MISC_2 BIT(1) is now cleared on these
chips.  This runs both from the probe error path and from
rtl8152_disconnect() via tp->rtl_ops.unload(), so the state left on the
chip after unplug or module removal differs from released kernels.  Given
that r8157_init() and r8159_init() already use r8157_power_cut_en() for
these versions, was the earlier rtl8153_unload assignment simply wrong?
If so, should this carry a Fixes: tag against fd3c7d080df5 and
ad2a55b63b41, and name the observable misbehaviour (power cut, suspend or
resume, WOL) that it corrects?

The change_mtu switch has the same question.  rtl8156_change_mtu() writes a
fixed PLA_MTPS of MTPS_JUMBO (12 * 1024 / 64), while rtl8157_change_mtu()
writes max(mtu_to_size(mtu), 10K) / 64 clamped to 0xff.  netdev->max_mtu
for RTL_VER_16 and RTL_VER_17_* is size_to_mtu(16 * 1024), so on a released
kernel a user can already configure a frame size above the fixed 12K MTPS
threshold through rtl8152_change_mtu() -> tp->rtl_ops.change_mtu().  Was
that mismatch causing a visible TX problem, and can the changelog say what
it was instead of only "unlike rtl8156_change_mtu which uses a fixed MTPS"?

As submitted, it is hard for a stable backporter to tell whether this is a
fix for shipping RTL8157/RTL8159 adapters or a cleanup with no user impact.
The rest of the series does not add the missing rationale or a Fixes: tag
either.

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