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

[Yibo Dong]

On Fri, Jun 05, 2026 at 01:32:59AM +0200, Andrew Lunn wrote:
> > +static int rnpgbe_process_link_event(struct mucse *mucse)
> > +{
> > +	struct net_device *netdev = mucse->netdev;
> > +	struct mucse_hw *hw = &mucse->hw;
> > +	unsigned long flags;
> > +	bool link;
> > +	int speed;
> > +	u8 duplex;
> > +
> > +	/* lockless fast-path ,serv_task retry handles race */
> > +	if (!(mucse->flags & M_FLAG_NEED_LINK_UPDATE))
> > +		return hw->link;
> > +
> > +	spin_lock_irqsave(&mucse->link_lock, flags);
> > +
> > +	link = hw->link;
> > +	speed = hw->speed;
> > +	duplex = hw->duplex;
> > +
> > +	mucse->flags &= ~M_FLAG_NEED_LINK_UPDATE;
> > +	spin_unlock_irqrestore(&mucse->link_lock, flags);
> > +
> > +	if (link) {
> > +		netdev_info(netdev, "NIC Link is Up %d Mbps, %s Duplex\n",
> > +			    speed,
> > +			    duplex ? "Full" : "Half");
> > +	}
> 
> I actually think down is more useful to report than up? Up is
> generally the normal state, you don't care about that too
> much. However if the link does down, you probably want to know so you
> can figure out why.
> 
I agree down is the more actionable signal. However most in-tree drivers
(igb, e1000e, nfp, etc.) report both directions, and the link-up message
carries speed/duplex information that is useful at boot or after renegotiation.
I'd prefer to keep both but move the up print to the right place.
> > + * rnpgbe_link_is_down - Update netif_carrier status and
> > + * print link down message
> > + * @mucse: pointer to the private structure
> > + **/
> > +static void rnpgbe_link_is_down(struct mucse *mucse)
> > +{
> > +	struct net_device *netdev = mucse->netdev;
> > +	struct mucse_hw *hw = &mucse->hw;
> > +
> > +	/* Only continue if link was up previously */
> > +	if (!netif_carrier_ok(netdev))
> > +		return;
> > +	netdev_info(netdev, "NIC Link is Down\n");
> > +	rnpgbe_set_link(hw, false);
> > +	netif_carrier_off(netdev);
> 
> Why asymmetric behaviour.  Report up in one place down somewhere else?
You're right — the up message in rnpgbe_process_link_event() is misplaced.
I should move it to rnpgbe_link_is_up which is symmetrically with the down path.
> 
> 	Andrew
> 
Thanks for you feedback.