Re: [PATCH net-next v7 01/10] r8152: split r8156_init by generation and factor out wait_autoload_done

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8152: split r8156_init by generation and factor out wait_autoload_done

The patch splits r8156_init() into two functions. r8156_init() now covers
RTL8156/RTL8156B, and the new r8157_init() covers RTL8157/RTL8159. It also
moves the repeated PLA_BOOT_CTRL AUTOLOAD_DONE polling loop into a new
wait_autoload_done() helper built on read_poll_timeout(), and converts the
r8153 init and ups_en paths to use it.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index af3d7dcb2f141..518a2b42c2200 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -4172,6 +4172,23 @@ 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,
> +				test_bit(RTL8152_INACCESSIBLE, &tp->flags) ||
> +				(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 test_bit(RTL8152_INACCESSIBLE, &tp->flags) ? -ENODEV : 0;
> +}
> +

[Severity: Medium]
Can this end up reading uninitialized stack memory in ocp_read_word()?

read_poll_timeout() runs op before it evaluates cond, on the first
iteration, after every sleep, and once more after the timeout expires.
So once RTL8152_INACCESSIBLE gets set, the helper still issues another
ocp_read_word() on PLA_BOOT_CTRL before the test_bit() in the condition
gets a chance to stop the loop.

In that case generic_ocp_read() returns -ENODEV without writing to the
local tmp. ocp_read_word() ignores that return value and still does
__le32_to_cpu(tmp), then hands the result back as ocp_data.

The loops being replaced did not do this. r8153b_ups_en() and
r8153c_ups_en() tested the flag before each read. The init functions
tested it right after msleep(20) and left the loop before the next
read.

The test_bit() short-circuit means the garbage is never acted on, but
the read itself is still undefined behaviour of the sort KMSAN is meant
to catch, and it now happens whenever the device goes away while we are
waiting for AUTOLOAD_DONE.

A flag check before the poll would only cover the entry case, since
the flag can be set during the sleep. Would it be better to initialize
tmp in ocp_read_word() and the other ocp_read_*() helpers, given that
plenty of existing callers already read without checking the flag?
Or is an open-coded loop that tests the flag first preferable here?

[ ... ]

> @@ -7225,22 +7226,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);
>  
> @@ -7341,22 +7334,13 @@ static void r8153_init(struct r8152 *tp)
>  
>  static void r8153b_init(struct r8152 *tp)
>  {
> -	int i;
> -
>  	if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
>  		return;
>  
>  	r8153b_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]
Is the commit message accurate for r8153_init() and r8153b_init()? It
says:

  a plain AUTOLOAD_DONE timeout is logged but otherwise treated as
  success, same as every one of the loops it replaces, which only bailed
  out early on RTL8152_INACCESSIBLE and otherwise fell through to the
  rest of the init sequence.

and:

  No functional change intended.

In these two functions the old loops did a break, not a return, when
RTL8152_INACCESSIBLE was set:

		msleep(20);
		if (test_bit(RTL8152_INACCESSIBLE, &tp->flags))
			break;

So init always ran to the end of the function. Only r8153c_init(), the
old r8156_init() and the two *_ups_en() loops returned early.

wait_autoload_done() returns -ENODEV whenever RTL8152_INACCESSIBLE is
set, so both functions now return early instead.

The commit message says the early return costs nothing because
generic_ocp_read()/generic_ocp_write() short-circuit. However, the
skipped tail also does work that does not go through OCP accesses:

  r8153_init():
	usb_enable_lpm(tp->udev);
	...
	tp->coalesce = COALESCE_SUPER;   (or COALESCE_HIGH / COALESCE_SLOW)

  r8153b_init():
	usb_enable_lpm(tp->udev);
	...
	set_bit(GREEN_ETHERNET, &tp->flags);
	...
	tp->coalesce = 15000;

usb_enable_lpm() is a counted USB core call. It decrements
udev->lpm_disable_count and does not check the driver's
RTL8152_INACCESSIBLE flag.

The polling interval also changes. The old loops used msleep(20), while
read_poll_timeout() with a sleep_us of 20000 ends up in
usleep_range(5001, 20000).

These differences only show up on error paths. Still, could the commit
message describe the new early return in r8153_init() and r8153b_init()?
Or should these two functions keep the old break-and-continue behaviour?
-- 
pw-bot: cr