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/[email protected]?part=4
---
--- Patch [4]: [PATCH net-next v8 4/4] net: rnpgbe: Add link status handling support ---
commit aaed18098b5cbd048578a3cab86657b190d37796
Author: Dong Yibo <[email protected]>

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
[ ... ]
> @@ -243,6 +249,8 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
>  	if (err)
>  		goto err_remove_mbx;
>  
> +	netif_carrier_off(netdev);
> +
>  	return 0;

[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?