Re: [PATCH]: Third (final?) release of Sun Neptune driver

[David Miller <davem@davemloft.net>]

From: Oliver Hartkopp <oliver@hartkopp.net>
Date: Mon, 08 Oct 2007 18:01:20 +0200

> David Miller wrote:
> > +
> > +static int link_status_1g(struct niu *np, int *link_up_p)
 ...
> Should *link_up_p be set to any value before returning?
 ...
> > +out:
> > +	spin_unlock_irqrestore(&np->lock, flags);
> > +
> > +	*link_up_p = link_up;
> > +	return 0;
> > +}
> > +
> >   
> Although you added a proper return value in link_status_10g() while 
> fixing the locking issue, you should think about providing a similar 
> return value for link_status_1g() to be constistent here.

Thanks for finding these two issues.  I've fixed it in the patch
below.

Luckily, this case was harmless.  When we enable a loopback mode,
niu_open() forces the link up by calling netif_carrier_on().
Then, niu_timer() always sees -EINVAL coming back from
niu_link_status() because of these loopback checks, and in such
cases never uses the link_up value because niu_link_status_common()
will not be invoked.

Anyways, thanks again!

commit ec3ec3566a8ee3c8c262606e06299c8212ebd53f
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Mon Oct 8 16:08:17 2007 -0700

    [NIU]: Make sure link_up status is set to something in link_status_{1,10}g().
    
    Noticed by Oliver Hartkopp.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 2a69cee..7c09243 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -1082,13 +1082,14 @@ static int link_status_10g(struct niu *np, int *link_up_p)
 	unsigned long flags;
 	int err, link_up;
 
-	if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
-		return -EINVAL;
-
 	link_up = 0;
 
 	spin_lock_irqsave(&np->lock, flags);
 
+	err = -EINVAL;
+	if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
+		return out;
+
 	err = mdio_read(np, np->phy_addr, BCM8704_PMA_PMD_DEV_ADDR,
 			BCM8704_PMD_RCV_SIGDET);
 	if (err < 0)
@@ -1119,15 +1120,12 @@ static int link_status_10g(struct niu *np, int *link_up_p)
 	link_up = 1;
 	np->link_config.active_speed = SPEED_10000;
 	np->link_config.active_duplex = DUPLEX_FULL;
+	err = 0;
 
 out:
 	spin_unlock_irqrestore(&np->lock, flags);
 
 	*link_up_p = link_up;
-	return 0;
-
-out_err:
-	spin_unlock_irqrestore(&np->lock, flags);
 	return err;
 }
 
@@ -1138,15 +1136,16 @@ static int link_status_1g(struct niu *np, int *link_up_p)
 	u8 current_duplex;
 	int err, link_up;
 
-	if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
-		return -EINVAL;
-
 	link_up = 0;
 	current_speed = SPEED_INVALID;
 	current_duplex = DUPLEX_INVALID;
 
 	spin_lock_irqsave(&np->lock, flags);
 
+	err = -EINVAL;
+	if (np->link_config.loopback_mode != LOOPBACK_DISABLED)
+		goto out;
+
 	err = mii_read(np, np->phy_addr, MII_BMSR);
 	if (err < 0)
 		goto out;
@@ -1172,6 +1171,7 @@ static int link_status_1g(struct niu *np, int *link_up_p)
 			goto out;
 		estat = err;
 
+		link_up = 1;
 		if (estat & (ESTATUS_1000_TFULL | ESTATUS_1000_THALF)) {
 			current_speed = SPEED_1000;
 			if (estat & ESTATUS_1000_TFULL)
@@ -1195,16 +1195,16 @@ static int link_status_1g(struct niu *np, int *link_up_p)
 				current_speed = SPEED_10;
 				current_duplex = DUPLEX_HALF;
 			} else
-				goto out;
+				link_up = 0;
 		}
-		link_up = 1;
 	}
+	err = 0;
 
 out:
 	spin_unlock_irqrestore(&np->lock, flags);
 
 	*link_up_p = link_up;
-	return 0;
+	return err;
 }
 
 static int niu_link_status(struct niu *np, int *link_up_p)