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

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

[Linmao Li]

r8152 detects carrier from PLA_PHYSTATUS without reading BMSR, so
BMSR_LSTATUS can still be latched low when the carrier comes up.
Since commit f6f2e946aa4d ("net: mii: Fix the Speed display when the
network cable is not connected"), the first speed query after link up
can then report SPEED_UNKNOWN, leaving NetworkManager at 0 Mb/s until
the next carrier change.

Use BMSR_LSTATUS in set_carrier() and rtl8152_runtime_resume(), so the
driver consumes the latched link down itself. If the first read still
reports link down, the next link-up notification triggers another read
and brings the carrier up.

Tested on an RTL8153B with a 6.6-based kernel. In 5 rebinds and 6 cable
replugs, the first read returned LSTATUS=0, a second link-up
notification came about 32 ms later, the second read returned
LSTATUS=1 and the carrier went up; NetworkManager reported 1000 Mb/s.
Runtime suspend/resume with the link up did not change the carrier.

Fixes: f6f2e946aa4d ("net: mii: Fix the Speed display when the network cable is not connected")
Cc: stable@vger.kernel.org
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
v2:
- Fix it in r8152 instead of mii.c, detecting the link from BMSR as
  suggested by Andrew Lunn.
- Also use BMSR in rtl8152_runtime_resume().
v1: https://lore.kernel.org/netdev/20260930113842.2640928-1-lilinmao@kylinos.cn/

Only tested on an RTL8153B, with a 6.6-based kernel; the changed lines
are the same in net/main. I could not check whether BMSR_LSTATUS is
reliable on the 2.5G/5G/10G chips handled by this driver.
I could not reproduce a link drop that recovers while the device is
runtime suspended, so that case is only covered by code inspection.

r8152_mdio_read() cannot fail today. The pending RTL8157/RTL8159
series makes it return errors, so these two callers will need error
handling once that is merged.

 drivers/net/usb/r8152.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index f61686433031..e3947eb796c5 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);
@@ -8653,7 +8650,7 @@ static int rtl8152_runtime_resume(struct r8152 *tp)
 		set_bit(WORK_ENABLE, &tp->flags);
 
 		if (netif_carrier_ok(netdev)) {
-			if (rtl8152_get_speed(tp) & LINK_STATUS) {
+			if (r8152_mdio_read(tp, MII_BMSR) & BMSR_LSTATUS) {
 				rtl_start_rx(tp);
 			} else {
 				netif_carrier_off(netdev);
-- 
2.25.1

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

[Birger Koblitz]

Hi Linmao,

On 05/10/2026 12:52 pm, Linmao Li wrote:
> r8152 detects carrier from PLA_PHYSTATUS without reading BMSR, so
> BMSR_LSTATUS can still be latched low when the carrier comes up.
> Since commit f6f2e946aa4d ("net: mii: Fix the Speed display when the
> network cable is not connected"), the first speed query after link up
> can then report SPEED_UNKNOWN, leaving NetworkManager at 0 Mb/s until
> the next carrier change.
> 
> Use BMSR_LSTATUS in set_carrier() and rtl8152_runtime_resume(), so the
> driver consumes the latched link down itself. If the first read still
> reports link down, the next link-up notification triggers another read
> and brings the carrier up.
> 
> Tested on an RTL8153B with a 6.6-based kernel. In 5 rebinds and 6 cable
> replugs, the first read returned LSTATUS=0, a second link-up
> notification came about 32 ms later, the second read returned
> LSTATUS=1 and the carrier went up; NetworkManager reported 1000 Mb/s.
> Runtime suspend/resume with the link up did not change the carrier.

I have been trying to reproduce the issue with the following adapter:
driver: r8152
version: 7.1.8+deb13-amd64
firmware-version: rtl8153a-4 v2 02/07/20

But I am not able to do so. Both after a link-up after a plugin event 
and after a system resume, ethtool always reports that the correct link 
speed and state. Could you explain, what you actually do, exactly? Or is 
the wrong link information only there for 32ms and I am just too slow?

Birger

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

[Hayes Wang]

Linmao Li <lilinmao@kylinos.cn>
> Sent: Monday, October 5, 2026 6:53 PM
[...]
> r8152 detects carrier from PLA_PHYSTATUS without reading BMSR, so
> BMSR_LSTATUS can still be latched low when the carrier comes up.
> Since commit f6f2e946aa4d ("net: mii: Fix the Speed display when the network
> cable is not connected"), the first speed query after link up can then report
> SPEED_UNKNOWN, leaving NetworkManager at 0 Mb/s until the next carrier
> change.
> 
> Use BMSR_LSTATUS in set_carrier() and rtl8152_runtime_resume(), so the
> driver consumes the latched link down itself. If the first read still reports link
> down, the next link-up notification triggers another read and brings the carrier
> up.
> 
> Tested on an RTL8153B with a 6.6-based kernel. In 5 rebinds and 6 cable
> replugs, the first read returned LSTATUS=0, a second link-up notification came
> about 32 ms later, the second read returned
> LSTATUS=1 and the carrier went up; NetworkManager reported 1000 Mb/s.
> Runtime suspend/resume with the link up did not change the carrier.

I think this patch may introduce a new issue.

Our newer ICs do not generate periodic link-status notifications. They generate a
notification only when the link status changes. Therefore, with your patch, BMSR
will not be read a second time until the next link-status change.

Best Regards,
Hayes

> Fixes: f6f2e946aa4d ("net: mii: Fix the Speed display when the network cable is
> not connected")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
> v2:
> - Fix it in r8152 instead of mii.c, detecting the link from BMSR as
>   suggested by Andrew Lunn.
> - Also use BMSR in rtl8152_runtime_resume().
> v1:
> https://lore.kernel.org/netdev/20260930113842.2640928-1-lilinmao@kylinos.c
> n/
> 
> Only tested on an RTL8153B, with a 6.6-based kernel; the changed lines are the
> same in net/main. I could not check whether BMSR_LSTATUS is reliable on the
> 2.5G/5G/10G chips handled by this driver.
> I could not reproduce a link drop that recovers while the device is runtime
> suspended, so that case is only covered by code inspection.
> 
> r8152_mdio_read() cannot fail today. The pending RTL8157/RTL8159 series
> makes it return errors, so these two callers will need error handling once that
> is merged.
> 
>  drivers/net/usb/r8152.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c index
> f61686433031..e3947eb796c5 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); @@ -8653,7 +8650,7
> @@ static int rtl8152_runtime_resume(struct r8152 *tp)
>                 set_bit(WORK_ENABLE, &tp->flags);
> 
>                 if (netif_carrier_ok(netdev)) {
> -                       if (rtl8152_get_speed(tp) & LINK_STATUS) {
> +                       if (r8152_mdio_read(tp, MII_BMSR) &
> + BMSR_LSTATUS) {
>                                 rtl_start_rx(tp);
>                         } else {
>                                 netif_carrier_off(netdev);
> --
> 2.25.1

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

[Andrew Lunn]

On Tue, Oct 06, 2026 at 08:28:21AM +0000, Hayes Wang wrote:
> Linmao Li <lilinmao@kylinos.cn>
> > Sent: Monday, October 5, 2026 6:53 PM
> [...]
> > r8152 detects carrier from PLA_PHYSTATUS without reading BMSR, so
> > BMSR_LSTATUS can still be latched low when the carrier comes up.
> > Since commit f6f2e946aa4d ("net: mii: Fix the Speed display when the network
> > cable is not connected"), the first speed query after link up can then report
> > SPEED_UNKNOWN, leaving NetworkManager at 0 Mb/s until the next carrier
> > change.
> > 
> > Use BMSR_LSTATUS in set_carrier() and rtl8152_runtime_resume(), so the
> > driver consumes the latched link down itself. If the first read still reports link
> > down, the next link-up notification triggers another read and brings the carrier
> > up.
> > 
> > Tested on an RTL8153B with a 6.6-based kernel. In 5 rebinds and 6 cable
> > replugs, the first read returned LSTATUS=0, a second link-up notification came
> > about 32 ms later, the second read returned
> > LSTATUS=1 and the carrier went up; NetworkManager reported 1000 Mb/s.
> > Runtime suspend/resume with the link up did not change the carrier.
> 
> I think this patch may introduce a new issue.
> 
> Our newer ICs do not generate periodic link-status notifications. They generate a
> notification only when the link status changes. Therefore, with your patch, BMSR
> will not be read a second time until the next link-status change.

But the link status in BMSR does change.

You read it once and get the latched value. That clears the latch, so
the link status changes to the current version.

Now, 802.3 C22 has no support for interrupts, that is a vendor
extension. But if you are not generating an interrupt when BMSR
changes, i would say that is broken.

	 Andrew

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

[netdev-bot+sashiko]

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