[resend PATCH 1/1] b44: fix ethool link settings support

[resend PATCH 1/1] b44: fix ethool link settings support

[Gary Zambrano]

The b44 driver support for setting link speed/duplex using ethtool was broken. 
This patch contains the following driver changes for link settings:
-clear the b44 flags of the previous settings before setting the phy
-allow access to get/set_settings() if interface is down
-set "advertised autoneg" setting dependent on autoneg enabled 
-return unknown speed/duplex if the interface is down

Signed-off-by: Gary Zambrano <zambrano@broadcom.com>

diff --git a/drivers/net/b44.c b/drivers/net/b44.c
index 3d30668..7b3dfc4 100644
--- a/drivers/net/b44.c
+++ b/drivers/net/b44.c
@@ -1612,8 +1612,6 @@ static int b44_get_settings(struct net_d
 {
 	struct b44 *bp = netdev_priv(dev);
 
-	if (!netif_running(dev))
-		return -EAGAIN;
 	cmd->supported = (SUPPORTED_Autoneg);
 	cmd->supported |= (SUPPORTED_100baseT_Half |
 			  SUPPORTED_100baseT_Full |
@@ -1641,6 +1639,12 @@ static int b44_get_settings(struct net_d
 		XCVR_INTERNAL : XCVR_EXTERNAL;
 	cmd->autoneg = (bp->flags & B44_FLAG_FORCE_LINK) ?
 		AUTONEG_DISABLE : AUTONEG_ENABLE;
+	if (cmd->autoneg == AUTONEG_ENABLE)
+		cmd->advertising |= ADVERTISED_Autoneg;
+	if (!netif_running(dev)){
+		cmd->speed = 0; 
+		cmd->duplex = 0xff;
+	}
 	cmd->maxtxpkt = 0;
 	cmd->maxrxpkt = 0;
 	return 0;
@@ -1650,47 +1654,42 @@ static int b44_set_settings(struct net_d
 {
 	struct b44 *bp = netdev_priv(dev);
 
-	if (!netif_running(dev))
-		return -EAGAIN;
-
-	/* We do not support gigabit. */
-	if (cmd->autoneg == AUTONEG_ENABLE) {
-		if (cmd->advertising &
-		    (ADVERTISED_1000baseT_Half |
-		     ADVERTISED_1000baseT_Full))
-			return -EINVAL;
-	} else if ((cmd->speed != SPEED_100 &&
-		    cmd->speed != SPEED_10) ||
-		   (cmd->duplex != DUPLEX_HALF &&
-		    cmd->duplex != DUPLEX_FULL)) {
-			return -EINVAL;
-	}
-
 	spin_lock_irq(&bp->lock);
 
 	if (cmd->autoneg == AUTONEG_ENABLE) {
-		bp->flags &= ~B44_FLAG_FORCE_LINK;
-		bp->flags &= ~(B44_FLAG_ADV_10HALF |
+		bp->flags &= ~(B44_FLAG_FORCE_LINK |
+			       B44_FLAG_100_BASE_T |
+			       B44_FLAG_FULL_DUPLEX |
+			       B44_FLAG_ADV_10HALF |
 			       B44_FLAG_ADV_10FULL |
 			       B44_FLAG_ADV_100HALF |
 			       B44_FLAG_ADV_100FULL);
-		if (cmd->advertising & ADVERTISE_10HALF)
-			bp->flags |= B44_FLAG_ADV_10HALF;
-		if (cmd->advertising & ADVERTISE_10FULL)
-			bp->flags |= B44_FLAG_ADV_10FULL;
-		if (cmd->advertising & ADVERTISE_100HALF)
-			bp->flags |= B44_FLAG_ADV_100HALF;
-		if (cmd->advertising & ADVERTISE_100FULL)
-			bp->flags |= B44_FLAG_ADV_100FULL;
+		if (cmd->advertising & ADVERTISE_ALL) {
+			if (cmd->advertising & ADVERTISE_10HALF)
+				bp->flags |= B44_FLAG_ADV_10HALF;
+			if (cmd->advertising & ADVERTISE_10FULL)
+				bp->flags |= B44_FLAG_ADV_10FULL;
+			if (cmd->advertising & ADVERTISE_100HALF)
+				bp->flags |= B44_FLAG_ADV_100HALF;
+			if (cmd->advertising & ADVERTISE_100FULL)
+				bp->flags |= B44_FLAG_ADV_100FULL;
+		} else {
+			bp->flags |= (B44_FLAG_ADV_10HALF |
+				      B44_FLAG_ADV_10FULL |
+				      B44_FLAG_ADV_100HALF |
+				      B44_FLAG_ADV_100FULL);
+		}
 	} else {
 		bp->flags |= B44_FLAG_FORCE_LINK;
+		bp->flags &= ~(B44_FLAG_100_BASE_T | B44_FLAG_FULL_DUPLEX);
 		if (cmd->speed == SPEED_100)
 			bp->flags |= B44_FLAG_100_BASE_T;
 		if (cmd->duplex == DUPLEX_FULL)
 			bp->flags |= B44_FLAG_FULL_DUPLEX;
 	}
 
-	b44_setup_phy(bp);
+	if(netif_running(dev))
+		b44_setup_phy(bp);
 
 	spin_unlock_irq(&bp->lock);
 

Re: [resend PATCH 1/1] b44: fix ethool link settings support

[Jeff Garzik]

Gary Zambrano wrote:
> +		if (cmd->advertising & ADVERTISE_ALL) {
> +			if (cmd->advertising & ADVERTISE_10HALF)
> +				bp->flags |= B44_FLAG_ADV_10HALF;
> +			if (cmd->advertising & ADVERTISE_10FULL)
> +				bp->flags |= B44_FLAG_ADV_10FULL;
> +			if (cmd->advertising & ADVERTISE_100HALF)
> +				bp->flags |= B44_FLAG_ADV_100HALF;
> +			if (cmd->advertising & ADVERTISE_100FULL)
> +				bp->flags |= B44_FLAG_ADV_100FULL;
> +		} else {
> +			bp->flags |= (B44_FLAG_ADV_10HALF |
> +				      B44_FLAG_ADV_10FULL |
> +				      B44_FLAG_ADV_100HALF |
> +				      B44_FLAG_ADV_100FULL);
> +		}

The logic above is backwards.  I presume the first test should be
	if (!(cmd->advertising & ADVERTISE_ALL))

The rest looks OK.

	Jeff

Re: [resend PATCH 1/1] b44: fix ethool link settings support

[Gary Zambrano]

The first test is to check if any advertising value is given.
The else path is when advertising == 0.
The b44->flags do not have the same values as the given advertising bits.
When any (or all) of the ADVERTISE_ALL bits are passed to the driver the b44->flags 
are set accordingly.
I think it's ok. 

Thanks,
Gary


On Wed, 2006-05-24 at 02:41 -0400, Jeff Garzik wrote:
> Gary Zambrano wrote:
> > +		if (cmd->advertising & ADVERTISE_ALL) {
> > +			if (cmd->advertising & ADVERTISE_10HALF)
> > +				bp->flags |= B44_FLAG_ADV_10HALF;
> > +			if (cmd->advertising & ADVERTISE_10FULL)
> > +				bp->flags |= B44_FLAG_ADV_10FULL;
> > +			if (cmd->advertising & ADVERTISE_100HALF)
> > +				bp->flags |= B44_FLAG_ADV_100HALF;
> > +			if (cmd->advertising & ADVERTISE_100FULL)
> > +				bp->flags |= B44_FLAG_ADV_100FULL;
> > +		} else {
> > +			bp->flags |= (B44_FLAG_ADV_10HALF |
> > +				      B44_FLAG_ADV_10FULL |
> > +				      B44_FLAG_ADV_100HALF |
> > +				      B44_FLAG_ADV_100FULL);
> > +		}
> 
> The logic above is backwards.  I presume the first test should be
> 	if (!(cmd->advertising & ADVERTISE_ALL))
> 
> The rest looks OK.
> 
> 	Jeff
> 
> 
>