Re: [PATCH net-next v5 5/9] r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

[netdev-bot+sashiko]

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

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

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] r8159_hw_phy_cfg() (drivers/net/usb/r8152.c,
  RTL_VER_17_QFN68/QFN100) no longer calls rtl_eee_enable(tp, false),…
- [Low] The commit message says the split gives each function 'its own
  copy of the shared prologue/epilogue instead of switching on version…

--

r8152: split r8157_hw_phy_cfg into RTL8157 and RTL8159 variants

This patch splits r8157_hw_phy_cfg() into r8157_hw_phy_cfg() for
RTL_VER_16 and a new r8159_hw_phy_cfg() for RTL_VER_17_QFN68/QFN100. The
RTL8159 variant no longer calls rtl_eee_enable(tp, false), changes how the
EEE clock divider and 0xa5ea are programmed, and adds new PHY tuning
parameters. It also adds the sram2_set_bits()/sram2_clr_bits() helpers.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 1d7cca55104cc..13a59a1df37ea 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c

[ ... ]

> +static void r8159_hw_phy_cfg(struct r8152 *tp)
> +{
> +	u16 data;
> +
> +	r8156b_wait_loading_flash(tp);

[ ... ]

> +	r8152_mdio_test_and_clr_bit(tp, MII_BMCR, BMCR_PDOWN);
> +
> +	r8153_aldps_en(tp, false);
> +
> +	data = r8153_phy_status(tp, PHY_STAT_LAN_ON);

[Severity: Medium]
With rtl_eee_enable(tp, false) gone, does anything on this path still
write the EEE-disabled state to the chip when tp->eee_en is false?

The epilogue of r8159_hw_phy_cfg() only does this:

	if (tp->eee_en)
		rtl_eee_enable(tp, true);

Before this patch, RTL_VER_17 always called rtl_eee_enable(tp, false)
first. Through r8156_eee_en(tp, false), that call clears EEE_RX_EN and
EEE_TX_EN in PLA_EEE_CR, EEE10_EN in OCP_EEE_CFG, and the 2.5G/5G bits in
OCP_EEE_ADV2. It then writes 0 to OCP_EEE_ADV. So at the end of the
function the hardware always matched tp->eee_en.

Take a user who turns EEE off:

r8152_set_eee()
    tp->eee_en = eee->eee_enabled;   /* false */
    rtl_eee_enable(tp, tp->eee_en);

The device then loses power or is reset:

r8156_ups_en(tp, false)   /* PCUT_STATUS set */
    tp->rtl_ops.hw_phy_cfg(tp)   -> r8159_hw_phy_cfg()
    rtl8152_set_speed(...)

The same applies to rtl8152_reset_resume() -> hw_phy_work ->
r8159_hw_phy_cfg(). Neither path touches the EEE registers, so they keep
their post-reset or firmware defaults.

If those defaults have EEE advertised, could the RTL8159 advertise EEE
and enter LPI after the user disabled it? r8153_get_eee() would then
report eee_enabled from tp->eee_en, but the advertised mask from
OCP_EEE_ADV/OCP_EEE_ADV2.

The commit message says:

    RTL8159 doesn't need EEE disabled before reprogramming PHY
    parameters, so r8159_hw_phy_cfg() drops the rtl_eee_enable(tp,
    false) call that r8157_hw_phy_cfg() keeps.

That call was also the only place where eee_en == false got applied to
freshly reset hardware. The other hw_phy_cfg variants keep the
disable-then-conditionally-enable pattern, and r8152b_hw_phy_cfg() uses
rtl_eee_enable(tp, tp->eee_en).

Would calling rtl_eee_enable(tp, tp->eee_en) in the r8159_hw_phy_cfg()
epilogue respect the datasheet requirement and still apply the user's
setting?

> +	WARN_ON_ONCE(data != PHY_STAT_LAN_ON);
> +
> +	/* PFM mode */
> +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_PHY_PWR, PFM_PWM_SWITCH);

[ ... ]

> +	/* Disable bypass_turn_off_clk_in_aldps */
> +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, 0xd3c8, BIT(0));
> +
> +	switch (tp->version) {
>  	case RTL_VER_17_QFN68:
>  	case RTL_VER_17_QFN100:

[Severity: Low]
This isn't a bug, but the commit message says each function gets:

    its own copy of the shared prologue/epilogue instead of switching on
    version mid-function.

Both functions still switch on tp->version mid-function.
rtl_ops_init() assigns r8157_hw_phy_cfg() only for RTL_VER_16, and
r8159_hw_phy_cfg() only for RTL_VER_17_QFN68/QFN100.

That means the switch (tp->version) here and the one in
r8157_hw_phy_cfg() each have just one reachable case. The default
branches can never run.

Could these switches be dropped, or the commit message changed to match
the code?

[ ... ]

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