Re: [PATCH net v2] r8152: Use BMSR to detect the link state

[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

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] set_carrier() in drivers/net/usb/r8152.c (line 6983) now
  decides carrier-up from a single read of MII_BMSR.

--

r8152: Use BMSR to detect the link state

This patch changes how set_carrier() and rtl8152_runtime_resume() check
the link state. They used PLA_PHYSTATUS (rtl8152_get_speed() &
LINK_STATUS) and now use MII_BMSR & BMSR_LSTATUS. The goal is for the
driver to consume the latched link-down bit itself, so the first speed
query after link up no longer reports SPEED_UNKNOWN.

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index f61686433031c..e3947eb796c5d 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -6979,11 +6979,8 @@ static void set_carrier(struct r8152 *tp)
>  {
>  	struct net_device *netdev = tp->netdev;
>  	struct napi_struct *napi = &tp->napi;
> -	u16 speed;
> -
> -	speed = rtl8152_get_speed(tp);
>  
> -	if (speed & LINK_STATUS) {
> +	if (r8152_mdio_read(tp, MII_BMSR) & BMSR_LSTATUS) {
>  		if (!netif_carrier_ok(netdev)) {
>  			tp->rtl_ops.enable(tp);
>  			netif_stop_queue(netdev);

[Severity: Medium]

Can a single read of BMSR here leave the carrier stuck off after the link
comes back up?

BMSR_LSTATUS is latched low. After any link-down period, the first read
returns 0 even if the link is already up again. The commit message says
the first read returned LSTATUS=0 in all 5 rebinds and 6 cable replugs.

set_carrier() only runs from rtl_work_func_t() when RTL8152_LINK_CHG is
set, and that flag is cleared before the BMSR read:

drivers/net/usb/r8152.c:rtl_work_func_t() {
    ...
	if (test_and_clear_bit(RTL8152_LINK_CHG, &tp->flags))
		set_carrier(tp);
    ...
}

The only place that sets the flag is intr_callback():

	if (INTR_LINK & __le16_to_cpu(d[0])) {
		if (!netif_carrier_ok(tp->netdev)) {
			set_bit(RTL8152_LINK_CHG, &tp->flags);
			schedule_delayed_work(&tp->schedule, 0);
		}
	}

So on a down->up transition:

1. The event is consumed.
2. The stale BMSR read returns 0.
3. The else branch does nothing because the carrier is already off.
4. Nothing schedules another check.

The commit message says:

  If the first read still reports link down, the next link-up
  notification triggers another read and brings the carrier up.

Can the driver rely on that second notification? On RTL8153B it happened
to arrive about 32 ms later, but this path was not tested on the other
chips this driver handles. If one of them only sends an interrupt report
when the link changes, the carrier would stay off until ifdown/ifup or a
replug.

The second report can also be lost. If usb_submit_urb() fails in
intr_callback() after the work was scheduled, the error is only logged:

	res = usb_submit_urb(urb, GFP_ATOMIC);
	...
	} else if (res) {
		netif_err(tp, intr, tp->netdev,
			  "can't resubmit intr, status %d\n", res);
	}

delay_autosuspend() still reads PLA_PHYSTATUS:

	bool hw_linking = !!(rtl8152_get_speed(tp) & LINK_STATUS);

In the stuck state it would see carrier off and hardware link up, so it
blocks autosuspend. It never calls set_carrier() again, though.

Before this patch, the check read PLA_PHYSTATUS, which is the same live
source as INTR_LINK, so one notification was enough.

Would it work better to do a dummy read when the carrier is off, the way
mii_link_ok() does in drivers/net/mii.c:

	/* first, a dummy read, needed to latch some MII phys */
	mii->mdio_read(mii->dev, mii->phy_id, MII_BMSR);

Another option is to read twice when the link was previously down, as
genphy_update_link() does.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005105249.1281648-1-lilinmao%40kylinos.cn