Re: [PATCH net-next v3 1/8] r8152: refactor r8156_init

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 2 (1) · Low: 0

- [High] wait_autoload_done() drops the per-iteration
  test_bit(RTL8152_INACCESSIBLE, &tp->flags) abort that every one of the…
- [High] On a genuine autoload timeout the new `if
  (wait_autoload_done(tp)) return;` aborts initialization mid-sequence…
- [Medium] Commit message mischaracterizes the change: a patch titled
  "refactor r8156_init" adds brand-new hardware programming and changes…
- [Medium] The new wait_autoload_done() helper is applied to five open-
  coded autoload-done loops that belong to chips outside the patch's…

Pre-existing issues:
- [Medium] `tp->coalesce = 15000;` carries the comment `/* 15 us */`, but
  tp->coalesce is exported verbatim to userspace as…

--

r8152: refactor r8156_init

This splits the shared r8156_init() into per-chip init functions for
RTL8156, RTL8156B, RTL8157 and RTL8159, and introduces a new
wait_autoload_done() helper that replaces the open-coded autoload-done
poll loops in several init and UPS paths.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f61686433031c..013e8d1abfc24 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -78,6 +78,7 @@
>  #define PLA_EEE_TXTWSYS_2P5G	0xe058
>  #define PLA_EEEP_CR		0xe080
>  #define PLA_MAC_PWR_CTRL	0xe0c0
> +#define PLA_RMT_WAKE		0xe0c8
>  #define PLA_MAC_PWR_CTRL2	0xe0ca
>  #define PLA_MAC_PWR_CTRL3	0xe0cc
>  #define PLA_MAC_PWR_CTRL4	0xe0ce
> @@ -371,6 +372,9 @@
>  #define MCU_CLK_RATIO_MASK	0x0f0f0f0f
>  #define ALDPS_SPDWN_RATIO	0x0f87
>  
> +/* PLA_RMT_WAKE */
> +#define RMT_WAKE_EN		BIT(0)
> +
>  /* PLA_MAC_PWR_CTRL2 */
>  #define EEE_SPDWN_RATIO		0x8007
>  #define MAC_CLK_SPDWN_EN	BIT(15)
> @@ -417,6 +421,7 @@
>  
>  /* PLA_INDICATE_FALG */
>  #define UPCOMING_RUNTIME_D3	BIT(0)
> +#define PREBOOT_OPTION		BIT(1)

[Severity: Medium]

The changelog says:

    Split r8156_init into per-chip functions:
    - r8156_init (VER_10/11)
    - r8156b_init (VER_12/13/15)
    - r8157_init (VER_16)
    - r8159_init (VER_17)

Is that a complete description of what the patch does?  These new register
and bit definitions have no user in the baseline, so the clears that use
them are new hardware programming rather than code motion:

  PLA_RMT_WAKE (0xe0c8) / RMT_WAKE_EN
  PLA_INDICATE_FALG / PREBOOT_OPTION

The same applies to several other writes that appear only after this patch:

  r8156b_u2phy_backup(): USB 0xd3ce/0xd3d0/0xd3d4/0xd3d8/0xd3dc
  r8157_init() and r8159_init(): USB 0xcd06 BIT(11) clear
  r8159_init(): USB 0xb9a6 BIT(0) clear and the write 0xb9a2 = 0x0448

A few existing sequences also change behaviour: AUTO_SPEEDUP is now cleared
for VER_16/VER_17 (previously only inside the VER_12..VER_15 block), the
0xcf04 interrupt-mitigation clear now also runs during init (previously
only in the unload path), and r8156b_wait_loading_flash() moved from before
the autoload poll to after it for VER_16/17.

Would it be possible to split the mechanical function split from the new
chip programming, so that a regression on RTL8156/8156B/8157/8159 does not
bisect to a commit that describes itself as a pure refactor?

[ ... ]

> @@ -4193,6 +4199,22 @@ static u16 r8153_phy_status(struct r8152 *tp, u16 desired)
>  	return data;
>  }
>  
> +static int wait_autoload_done(struct r8152 *tp)
> +{
> +	u16 ocp_data;
> +	int ret;
> +
> +	ret = read_poll_timeout(ocp_read_word, ocp_data,
> +				ocp_data & AUTOLOAD_DONE, 20000,
> +				10 * USEC_PER_SEC, false, tp, MCU_TYPE_PLA,
> +				PLA_BOOT_CTRL);
> +
> +	if (ret)
> +		dev_err(&tp->intf->dev, "autoload done timeout\n");
> +
> +	return ret;
> +}
> +

[Severity: High]

Every loop this helper replaces tested RTL8152_INACCESSIBLE on each
iteration, but the new poll has no exit other than the condition or the
10 second deadline.  Can this keep sleeping for 10 seconds under
tp->control after the device is gone?

Once RTL8152_INACCESSIBLE is set (rtl_set_unplug() on disconnect, pre-reset,
or a failed control transfer), the read never touches the caller's buffer:

drivers/net/usb/r8152.c:generic_ocp_read() {
	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
		return -ENODEV;
	...
}

and ocp_read_word() ignores that error while converting an uninitialized
variable:

drivers/net/usb/r8152.c:ocp_read_word() {
	u32 data;
	__le32 tmp;
	...
	generic_ocp_read(tp, index, sizeof(tmp), &tmp, type | byen);

	data = __le32_to_cpu(tmp);
	...
}

So is the poll condition ocp_data & AUTOLOAD_DONE decided by stack garbage
in that state?  If bit 1 happens to be set, wait_autoload_done() reports
success and the caller programs the whole bring-up sequence, including
tp->rtl_ops.hw_phy_cfg() from r8153b_ups_en()/r8153c_ups_en(), against a
device that is already gone.  If bit 1 is clear, the caller sleeps the full
10 seconds and then logs a timeout that did not really happen.

There is a related case on plain transfer failures:

drivers/net/usb/r8152.c:get_registers() {
	...
	if (ret < 0)
		memset(data, 0xff, size);
	...
}

0xffff & AUTOLOAD_DONE (0x0002) is non-zero, so the poll reports completion
on read errors too.

All the other wait loops in this file (r8153_phy_status(),
r8156b_wait_loading_flash(), rtl_phy_patch_request(), rtl_disable(),
r8153_aldps_en()) still honour RTL8152_INACCESSIBLE.  Should the poll
condition terminate on that flag as well, and should the read path preserve
the error status instead of returning an undefined value?

>  static void r8153b_ups_en(struct r8152 *tp, bool enable)
>  {
>  	if (enable) {
> @@ -4211,16 +4233,8 @@ static void r8153b_ups_en(struct r8152 *tp, bool enable)
>  				  UPS_FORCE_PWR_DOWN);
>  
>  		if (ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0) & PCUT_STATUS) {
> -			int i;
> -
> -			for (i = 0; i < 500; i++) {
> -				if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> -					return;
> -				if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> -				    AUTOLOAD_DONE)
> -					break;
> -				msleep(20);
> -			}
> +			if (wait_autoload_done(tp))
> +				return;
>  
>  			tp->rtl_ops.hw_phy_cfg(tp);
>  

[ ... ]

> @@ -7246,22 +7252,14 @@ static void r8152b_init(struct r8152 *tp)
>  static void r8153_init(struct r8152 *tp)
>  {
>  	u32 ocp_data;
> -	int i;
>  
>  	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>  		return;
>  
>  	r8153_u1u2en(tp, false);
>  
> -	for (i = 0; i < 500; i++) {
> -		if (ocp_read_word(tp, MCU_TYPE_PLA, PLA_BOOT_CTRL) &
> -		    AUTOLOAD_DONE)
> -			break;
> -
> -		msleep(20);
> -		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> -			break;
> -	}
> +	if (wait_autoload_done(tp))
> +		return;
>  
>  	r8153_phy_status(tp, 0);
>  

[Severity: Medium]

The helper is also applied to five call sites outside the chips named in
the changelog: r8153_init() (VER_03..06), r8153b_init() (VER_08/09),
r8153c_init() (VER_14), r8153b_ups_en() and r8153c_ups_en().

For r8153_init() and r8153b_init() the previous poll only did break on
expiry and then executed the rest of the bring-up (power-cut disable,
u1u2 re-enable, LPM, RX aggregation, rtl_tally_reset(), and the
tp->coalesce assignment at the tail of r8153_init).  With the new

	if (wait_autoload_done(tp))
		return;

all of that is skipped instead.  The helper also newly emits
dev_err("autoload done timeout") on paths that were previously silent, and
drops the per-iteration test_bit(RTL8152_INACCESSIBLE, &tp->flags) abort.

Is this change of failure semantics on the RTL8153/8153B/8153C families
intended?  Right now a regression there would bisect to a commit titled
"r8152: refactor r8156_init".

[ ... ]

> @@ -8351,90 +8331,132 @@ static void r8157_hw_phy_cfg(struct r8152 *tp)
>  	set_bit(PHY_RESET, &tp->flags);
>  }
>  
> -static int r8159_wait_backup_restore(struct r8152 *tp)
> +static void r8156_init(struct r8152 *tp)
>  {

[ ... ]

> +	r8156_mdio_force_mode(tp);
> +	rtl_tally_reset(tp);
> +
> +	tp->coalesce = 15000; /* 15 us */
>  }

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but the
comment and the value disagree with the unit that is exported to
userspace.  tp->coalesce is handed to ethtool verbatim as
ETHTOOL_COALESCE_USECS:

drivers/net/usb/r8152.c:rtl8152_get_coalesce() {
	...
	coalesce->rx_coalesce_usecs = tp->coalesce;
	...
}

drivers/net/usb/r8152.c:rtl8152_set_coalesce() {
	...
	tp->coalesce = coalesce->rx_coalesce_usecs;
	...
}

while the internal unit is nanoseconds, one register tick being 8 ns:

drivers/net/usb/r8152.c:r8153_set_rx_early_timeout() {
	u32 ocp_data = tp->coalesce / 8;
	...
	/* ... For USB_RX_EARLY_TIMEOUT, we fix it to 128ns. */
	ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_TIMEOUT, 128 / 8);
	...
}

So ethtool -c reports rx-usecs 15000 for a 15 us setting, and
ethtool -C rx-usecs 15 programs about 15 ns.  Since this patch replicates
the same constant and comment into r8156b_init(), r8157_init() and
r8159_init(), would it make sense to fix the unit (or at least the comment)
while these functions are being created?

> +
> +static void r8157_init(struct r8152 *tp)
> +{
> +	u16 data;
> +
> +	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
> +		return;
> +
> +	/* Enable SW reset */
> +	ocp_byte_set_bits(tp, MCU_TYPE_USB, 0xcffe, BIT(3));
> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, 0xd3ca, BIT(0));
> +
> +	ocp_byte_clr_bits(tp, MCU_TYPE_USB, USB_ECM_OP, EN_ALL_SPEED);
> +
> +	ocp_word_set_bits(tp, MCU_TYPE_USB, USB_ECM_OPTION, BYPASS_MAC_RESET);
> +
> +	r8153b_u1u2en(tp, false);
> +
> +	if (wait_autoload_done(tp))
> +		return;

[Severity: High]

On a real autoload timeout this returns in the middle of the sequence, after
the SW reset bits (0xcffe BIT(3), 0xd3ca BIT(0)) and the U1/U2 disable have
already been applied, and without marking the device inaccessible.

What state does the adapter end up in?  The rest of the function is skipped:
r8153b_u1u2en(tp, true), usb_enable_lpm(), r8156_mac_clk_spd(), the
PLA_MCU_SPDWN_EN clear, the PLA_EXTRA_STATUS link polling setup, the RX
aggregation / RX_DESC_16B / BMU programming, r8156_mdio_force_mode(),
rtl_tally_reset() and tp->coalesce.

tp->coalesce then stays 0 in the freshly allocated private area, and:

drivers/net/usb/r8152.c:r8153_set_rx_early_timeout() {
	u32 ocp_data = tp->coalesce / 8;
	...
}

programs USB_RX_EARLY_TIMEOUT / USB_RX_EXTRA_AGGR_TMR with 0.  The same
applies to r8153_init(), where the tp->coalesce = COALESCE_SUPER/HIGH/SLOW
switch sits at the very end of the function.

Since ops->init is void and the timeout path sets neither
RTL8152_INACCESSIBLE nor PROBE_SHOULD_RETRY, does anything upstream learn
about the failure?  rtl8152_probe_once() still registers the netdev and
rtl8152_reset_resume() still continues into the normal resume path.

The intended contract seems to be the one used a few lines below in the
same patch for the other failure of the same class:

> +	if (r8159_wait_backup_restore(tp)) {
> +		rtl_set_inaccessible(tp);
> +		dev_err(&tp->intf->dev,
> +			"init failed, backup-restore timed out\n");
> +		return;
>  	}

Should the autoload timeout do the same (rtl_set_inaccessible() plus an
error), so the device is not registered and used half-programmed?

The same early return in r8153b_ups_en()/r8153c_ups_en() skips
hw_phy_cfg() and rtl8152_set_speed() right after the chip left power-cut,
and in r8153c_ups_en() also the trailing PLA_CRWECR/PLA_CONFIG34 writes
that previously always ran.

[ ... ]

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