Re: [PATCH net-next v8 4/4] net: rnpgbe: Add link status handling support

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260731120322.895955-2-dong100@mucse.com?part=4
---

--- Patch [4]: [PATCH net-next v8 4/4] net: rnpgbe: Add link status handling support ---

commit aaed18098b5cbd048578a3cab86657b190d37796
Author: Dong Yibo [off-list ref]

net: rnpgbe: Add link status handling support

Add link status management infrastructure to the rnpgbe driver:
- Add link status related data structures (speed, duplex, link state)
- Implement firmware link event handling via mailbox
  (fw controls link states, does not support phylink)
- Add service task for periodic link status monitoring
- Implement carrier status management (netif_carrier_on/off)
- Add port up/down notification to firmware

This enables the driver to properly track and report link status changes.

diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_lib.c

[ ... ]

+static int rnpgbe_process_link_event(struct mucse *mucse)
+{
+	struct mucse_hw *hw = &mucse->hw;
+	unsigned long flags;
+	bool link;
+
+	/* Only event consumption needs a consistent link-state snapshot. */
+	if (!atomic_read(&mucse->link_pending) ||
+	    !atomic_xchg(&mucse->link_pending, 0))
+		return READ_ONCE(hw->link);
+
+	spin_lock_irqsave(&mucse->link_lock, flags);
+
+	link = hw->link;
+	spin_unlock_irqrestore(&mucse->link_lock, flags);
+
+	return link;
+}
+
+/**
+ * rnpgbe_link_is_up - Update netif_carrier status and
+ * print link up message
+ * @mucse: pointer to the device private structure
+ **/
+static void rnpgbe_link_is_up(struct mucse *mucse)
+{
+	struct net_device *netdev = mucse->netdev;
+	struct mucse_hw *hw = &mucse->hw;
+	unsigned long flags;
+	int speed;
+	u8 duplex;
+
+	/* Only continue if link was previously down */
+	if (netif_carrier_ok(netdev))
+		return;
+	spin_lock_irqsave(&mucse->link_lock, flags);
+	speed = hw->speed;
+	duplex = hw->duplex;
+	spin_unlock_irqrestore(&mucse->link_lock, flags);

[Severity: Medium]
Could this cause a torn link state snapshot if an interrupt fires between
these critical sections?

In rnpgbe_process_link_subtask(), the worker thread first reads the link state
in rnpgbe_process_link_event() under the lock, and then drops the lock.
If the link is true, it calls rnpgbe_link_is_up(), which re-acquires the lock
to read the speed and duplex values.

If a link-down event triggers an interrupt between these two lock acquisitions,
could the worker thread end up reading newly zeroed speed and duplex values,
incorrectly reporting "NIC Link is Up 0 Mbps", and turning the netif carrier
ON while the physical link is actually DOWN?

diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

[ ... ]

[Severity: Medium]
Does calling netif_carrier_off() after register_netdev() expose a transient
carrier up state to userspace?

By default, the networking core assumes the carrier state is OK when a new
network device is created. When register_netdev() is called earlier in
rnpgbe_add_adapter(), the device is exposed to userspace and an RTM_NEWLINK
netlink event is broadcast indicating the link is UP.

Immediately calling netif_carrier_off() right after will emit another event
indicating the carrier is DOWN. Can this spurious UP-then-DOWN transition
confuse userspace network management daemons?