[PATCH] Rewrite e100_phys_id

[PATCH] Rewrite e100_phys_id

[Matthew Wilcox]

The motivator for this was to fix the sparse warning:

drivers/net/e100.c:2418:48: warning: cast truncates bits from constant
value (83126e978d4fdf becomes 978d4fdf)
drivers/net/e100.c:2419:37: warning: cast truncates bits from constant
value (83126e978d4fdf becomes 978d4fdf)

Initially, I tried a quick fix, but when it ran into difficulties, I
looked at tg3.c to see how it does it.  I liked their way better, so I
rewrote e100.c to be similar.  It shaves ~700 bytes off the size of the
driver, and a few bytes off the size of struct nic, so I think it's a
win all round.  Tested on the internal interface of an HP Integrity rx2600.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index a3a08a5..aade1e9 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -556,7 +556,6 @@ struct nic {
 	struct params params;
 	struct net_device_stats net_stats;
 	struct timer_list watchdog;
-	struct timer_list blink_timer;
 	struct mii_if_info mii;
 	struct work_struct tx_timeout_task;
 	enum loopback loopback;
@@ -581,7 +580,6 @@ struct nic {
 	u32 rx_over_length_errors;
 
 	u8 rev_id;
-	u16 leds;
 	u16 eeprom_wc;
 	u16 eeprom[256];
 	spinlock_t mdio_lock;
@@ -2168,23 +2166,6 @@ err_clean_rx:
 	return err;
 }
 
-#define MII_LED_CONTROL	0x1B
-static void e100_blink_led(unsigned long data)
-{
-	struct nic *nic = (struct nic *)data;
-	enum led_state {
-		led_on     = 0x01,
-		led_off    = 0x04,
-		led_on_559 = 0x05,
-		led_on_557 = 0x07,
-	};
-
-	nic->leds = (nic->leds & led_on) ? led_off :
-		(nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559;
-	mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, nic->leds);
-	mod_timer(&nic->blink_timer, jiffies + HZ / 4);
-}
-
 static int e100_get_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
 {
 	struct nic *nic = netdev_priv(netdev);
@@ -2411,16 +2392,32 @@ static void e100_diag_test(struct net_de
 	msleep_interruptible(4 * 1000);
 }
 
+#define MII_LED_CONTROL	0x1B
 static int e100_phys_id(struct net_device *netdev, u32 data)
 {
 	struct nic *nic = netdev_priv(netdev);
+	int i;
+
+	enum led_state {
+		led_off    = 0x04,
+		led_on_559 = 0x05,
+		led_on_557 = 0x07,
+	};
+	u16 leds = led_off;
+
+	if (data == 0)
+		data = 2;
+
+	for (i = 0; i < (data * 2); i++) {
+		leds = (leds == led_off) ?
+			(nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559 :
+			led_off;
+		mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, leds);
+		if (msleep_interruptible(500))
+			break;
+	}
 
-	if(!data || data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ))
-		data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
-	mod_timer(&nic->blink_timer, jiffies);
-	msleep_interruptible(data * 1000);
-	del_timer_sync(&nic->blink_timer);
-	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, 0);
+	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, led_off);
 
 	return 0;
 }
@@ -2633,9 +2630,6 @@ #endif
 	init_timer(&nic->watchdog);
 	nic->watchdog.function = e100_watchdog;
 	nic->watchdog.data = (unsigned long)nic;
-	init_timer(&nic->blink_timer);
-	nic->blink_timer.function = e100_blink_led;
-	nic->blink_timer.data = (unsigned long)nic;
 
 	INIT_WORK(&nic->tx_timeout_task,
 		(void (*)(void *))e100_tx_timeout_task, netdev);

Re: [PATCH] Rewrite e100_phys_id

[Jeff Garzik]

On Thu, Oct 26, 2006 at 01:11:55PM -0600, Matthew Wilcox wrote:
> 
> The motivator for this was to fix the sparse warning:
> 
> drivers/net/e100.c:2418:48: warning: cast truncates bits from constant
> value (83126e978d4fdf becomes 978d4fdf)
> drivers/net/e100.c:2419:37: warning: cast truncates bits from constant
> value (83126e978d4fdf becomes 978d4fdf)
> 
> Initially, I tried a quick fix, but when it ran into difficulties, I
> looked at tg3.c to see how it does it.  I liked their way better, so I
> rewrote e100.c to be similar.  It shaves ~700 bytes off the size of the
> driver, and a few bytes off the size of struct nic, so I think it's a
> win all round.  Tested on the internal interface of an HP Integrity rx2600.
> 
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>

Seems sane to me...  I'll pick it up, if Auke doesn't...

	Jeff

Re: [PATCH] Rewrite e100_phys_id

[Auke Kok]

Jeff Garzik wrote:
> On Thu, Oct 26, 2006 at 01:11:55PM -0600, Matthew Wilcox wrote:
>> The motivator for this was to fix the sparse warning:
>>
>> drivers/net/e100.c:2418:48: warning: cast truncates bits from constant
>> value (83126e978d4fdf becomes 978d4fdf)
>> drivers/net/e100.c:2419:37: warning: cast truncates bits from constant
>> value (83126e978d4fdf becomes 978d4fdf)
>>
>> Initially, I tried a quick fix, but when it ran into difficulties, I
>> looked at tg3.c to see how it does it.  I liked their way better, so I
>> rewrote e100.c to be similar.  It shaves ~700 bytes off the size of the
>> driver, and a few bytes off the size of struct nic, so I think it's a
>> win all round.  Tested on the internal interface of an HP Integrity rx2600.
>>
>> Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> 
> Seems sane to me...  I'll pick it up, if Auke doesn't...

no objections, so I'll ACK it with the notion that I'm going to let our labs do some 
more testing on it with all the latest changes to it.

Jeff, I will stack it on the patches I have for 2.6.20 and push those out before the 
weekend.

Cheers,

Auke

Re: [PATCH] Rewrite e100_phys_id

[Matthew Wilcox]

On Thu, Oct 26, 2006 at 01:04:32PM -0700, Auke Kok wrote:
> no objections, so I'll ACK it with the notion that I'm going to let our 
> labs do some more testing on it with all the latest changes to it.

Thanks, Auke.  Here's the equivalent patch for e1000.  I don't have a
convenient machine to test it on, but it reduces the size of the driver
by 1.5k.

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 7ecce43..1e22da6 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -257,9 +257,6 @@ #endif
 	struct work_struct reset_task;
 	uint8_t fc_autoneg;
 
-	struct timer_list blink_timer;
-	unsigned long led_status;
-
 	/* TX */
 	struct e1000_tx_ring *tx_ring;      /* One per active queue */
 	unsigned long tx_queue_len;
diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
index 773821e..620afa5 100644
--- a/drivers/net/e1000/e1000_ethtool.c
+++ b/drivers/net/e1000/e1000_ethtool.c
@@ -1819,61 +1819,15 @@ e1000_set_wol(struct net_device *netdev,
 	return 0;
 }
 
-/* toggle LED 4 times per second = 2 "blinks" per second */
-#define E1000_ID_INTERVAL	(HZ/4)
-
-/* bit defines for adapter->led_status */
-#define E1000_LED_ON		0
-
-static void
-e1000_led_blink_callback(unsigned long data)
-{
-	struct e1000_adapter *adapter = (struct e1000_adapter *) data;
-
-	if (test_and_change_bit(E1000_LED_ON, &adapter->led_status))
-		e1000_led_off(&adapter->hw);
-	else
-		e1000_led_on(&adapter->hw);
-
-	mod_timer(&adapter->blink_timer, jiffies + E1000_ID_INTERVAL);
-}
-
 static int
 e1000_phys_id(struct net_device *netdev, uint32_t data)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 
-	if (!data || data > (uint32_t)(MAX_SCHEDULE_TIMEOUT / HZ))
-		data = (uint32_t)(MAX_SCHEDULE_TIMEOUT / HZ);
-
-	if (adapter->hw.mac_type < e1000_82571) {
-		if (!adapter->blink_timer.function) {
-			init_timer(&adapter->blink_timer);
-			adapter->blink_timer.function = e1000_led_blink_callback;
-			adapter->blink_timer.data = (unsigned long) adapter;
-		}
-		e1000_setup_led(&adapter->hw);
-		mod_timer(&adapter->blink_timer, jiffies);
-		msleep_interruptible(data * 1000);
-		del_timer_sync(&adapter->blink_timer);
-	} else if (adapter->hw.phy_type == e1000_phy_ife) {
-		if (!adapter->blink_timer.function) {
-			init_timer(&adapter->blink_timer);
-			adapter->blink_timer.function = e1000_led_blink_callback;
-			adapter->blink_timer.data = (unsigned long) adapter;
-		}
-		mod_timer(&adapter->blink_timer, jiffies);
-		msleep_interruptible(data * 1000);
-		del_timer_sync(&adapter->blink_timer);
-		e1000_write_phy_reg(&(adapter->hw), IFE_PHY_SPECIAL_CONTROL_LED, 0);
-	} else {
-		e1000_blink_led_start(&adapter->hw);
-		msleep_interruptible(data * 1000);
-	}
+	if (data == 0)
+		data = 2;
 
-	e1000_led_off(&adapter->hw);
-	clear_bit(E1000_LED_ON, &adapter->led_status);
-	e1000_cleanup_led(&adapter->hw);
+	e1000_blink_led(&adapter->hw, data);
 
 	return 0;
 }
diff --git a/drivers/net/e1000/e1000_hw.c b/drivers/net/e1000/e1000_hw.c
index 65077f3..db5e999 100644
--- a/drivers/net/e1000/e1000_hw.c
+++ b/drivers/net/e1000/e1000_hw.c
@@ -6071,7 +6071,7 @@ e1000_id_led_init(struct e1000_hw * hw)
  *
  * hw - Struct containing variables accessed by shared code
  *****************************************************************************/
-int32_t
+static int32_t
 e1000_setup_led(struct e1000_hw *hw)
 {
     uint32_t ledctl;
@@ -6123,50 +6123,11 @@ e1000_setup_led(struct e1000_hw *hw)
 
 
 /******************************************************************************
- * Used on 82571 and later Si that has LED blink bits.
- * Callers must use their own timer and should have already called
- * e1000_id_led_init()
- * Call e1000_cleanup led() to stop blinking
- *
- * hw - Struct containing variables accessed by shared code
- *****************************************************************************/
-int32_t
-e1000_blink_led_start(struct e1000_hw *hw)
-{
-    int16_t  i;
-    uint32_t ledctl_blink = 0;
-
-    DEBUGFUNC("e1000_id_led_blink_on");
-
-    if (hw->mac_type < e1000_82571) {
-        /* Nothing to do */
-        return E1000_SUCCESS;
-    }
-    if (hw->media_type == e1000_media_type_fiber) {
-        /* always blink LED0 for PCI-E fiber */
-        ledctl_blink = E1000_LEDCTL_LED0_BLINK |
-                     (E1000_LEDCTL_MODE_LED_ON << E1000_LEDCTL_LED0_MODE_SHIFT);
-    } else {
-        /* set the blink bit for each LED that's "on" (0x0E) in ledctl_mode2 */
-        ledctl_blink = hw->ledctl_mode2;
-        for (i=0; i < 4; i++)
-            if (((hw->ledctl_mode2 >> (i * 8)) & 0xFF) ==
-                E1000_LEDCTL_MODE_LED_ON)
-                ledctl_blink |= (E1000_LEDCTL_LED0_BLINK << (i * 8));
-    }
-
-    E1000_WRITE_REG(hw, LEDCTL, ledctl_blink);
-
-    return E1000_SUCCESS;
-}
-
-/******************************************************************************
  * Restores the saved state of the SW controlable LED.
  *
  * hw - Struct containing variables accessed by shared code
  *****************************************************************************/
-int32_t
-e1000_cleanup_led(struct e1000_hw *hw)
+static int32_t e1000_cleanup_led(struct e1000_hw *hw)
 {
     int32_t ret_val = E1000_SUCCESS;
 
@@ -6207,8 +6168,7 @@ e1000_cleanup_led(struct e1000_hw *hw)
  *
  * hw - Struct containing variables accessed by shared code
  *****************************************************************************/
-int32_t
-e1000_led_on(struct e1000_hw *hw)
+static int32_t e1000_led_on(struct e1000_hw *hw)
 {
     uint32_t ctrl = E1000_READ_REG(hw, CTRL);
 
@@ -6258,8 +6218,7 @@ e1000_led_on(struct e1000_hw *hw)
  *
  * hw - Struct containing variables accessed by shared code
  *****************************************************************************/
-int32_t
-e1000_led_off(struct e1000_hw *hw)
+static int32_t e1000_led_off(struct e1000_hw *hw)
 {
     uint32_t ctrl = E1000_READ_REG(hw, CTRL);
 
@@ -6304,6 +6263,69 @@ e1000_led_off(struct e1000_hw *hw)
     return E1000_SUCCESS;
 }
 
+static void e1000_blink_led_auto(struct e1000_hw *hw, unsigned int seconds)
+{
+    int16_t  i;
+    uint32_t ledctl_blink = 0;
+
+    DEBUGFUNC("e1000_blink_led");
+
+    if (hw->media_type == e1000_media_type_fiber) {
+        /* always blink LED0 for PCI-E fiber */
+        ledctl_blink = E1000_LEDCTL_LED0_BLINK |
+                     (E1000_LEDCTL_MODE_LED_ON << E1000_LEDCTL_LED0_MODE_SHIFT);
+    } else {
+        /* set the blink bit for each LED that's "on" (0x0E) in ledctl_mode2 */
+        ledctl_blink = hw->ledctl_mode2;
+        for (i=0; i < 4; i++)
+            if (((hw->ledctl_mode2 >> (i * 8)) & 0xFF) ==
+                E1000_LEDCTL_MODE_LED_ON)
+                ledctl_blink |= (E1000_LEDCTL_LED0_BLINK << (i * 8));
+    }
+
+    E1000_WRITE_REG(hw, LEDCTL, ledctl_blink);
+
+    msleep_interruptible(seconds * HZ);
+
+    e1000_led_off(hw);
+    e1000_cleanup_led(hw);
+}
+
+static void e1000_blink_led_manually(struct e1000_hw *hw, unsigned int seconds)
+{
+	unsigned int i;
+	e1000_setup_led(hw);
+
+	for (i = 0; i < (seconds * 4); i++) {
+		if (i & 1)
+			e1000_led_off(hw);
+		else
+			e1000_led_on(hw);
+		if (msleep_interruptible(250))
+			break;
+	}
+
+	e1000_led_off(hw);
+	e1000_cleanup_led(hw);
+}
+
+/******************************************************************************
+ * Blink the LED for the given number of seconds. 
+ *
+ * hw - Struct containing variables accessed by shared code
+ * seconds - Length of time to blink LED for
+ *****************************************************************************/
+void e1000_blink_led(struct e1000_hw *hw, unsigned int seconds)
+{
+	if (hw->mac_type < e1000_82571) {
+		e1000_blink_led_manually(hw, seconds);
+	} else if (hw->phy_type == e1000_phy_ife) {
+		e1000_blink_led_manually(hw, seconds);
+	} else {
+		e1000_blink_led_auto(hw, seconds);
+	}
+}
+
 /******************************************************************************
  * Clears all hardware statistics counters.
  *
diff --git a/drivers/net/e1000/e1000_hw.h b/drivers/net/e1000/e1000_hw.h
index 112447f..1efedb7 100644
--- a/drivers/net/e1000/e1000_hw.h
+++ b/drivers/net/e1000/e1000_hw.h
@@ -404,11 +404,7 @@ void e1000_rar_set(struct e1000_hw *hw, 
 void e1000_write_vfta(struct e1000_hw *hw, uint32_t offset, uint32_t value);
 
 /* LED functions */
-int32_t e1000_setup_led(struct e1000_hw *hw);
-int32_t e1000_cleanup_led(struct e1000_hw *hw);
-int32_t e1000_led_on(struct e1000_hw *hw);
-int32_t e1000_led_off(struct e1000_hw *hw);
-int32_t e1000_blink_led_start(struct e1000_hw *hw);
+void e1000_blink_led(struct e1000_hw *hw, unsigned int seconds);
 
 /* Adaptive IFS Functions */
 

Re: [PATCH] Rewrite e100_phys_id

[Auke Kok]

Matthew Wilcox wrote:
> On Thu, Oct 26, 2006 at 01:04:32PM -0700, Auke Kok wrote:
>> no objections, so I'll ACK it with the notion that I'm going to let our 
>> labs do some more testing on it with all the latest changes to it.
> 
> Thanks, Auke.  Here's the equivalent patch for e1000.  I don't have a
> convenient machine to test it on, but it reduces the size of the driver
> by 1.5k.

this is a bit (!) more complex than e100, so I'm going to take a bit of time to review 
this patch.

thanks,

Auke



> 
> diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> index 7ecce43..1e22da6 100644
> --- a/drivers/net/e1000/e1000.h
> +++ b/drivers/net/e1000/e1000.h
> @@ -257,9 +257,6 @@ #endif
>  	struct work_struct reset_task;
>  	uint8_t fc_autoneg;
>  
> -	struct timer_list blink_timer;
> -	unsigned long led_status;
> -
>  	/* TX */
>  	struct e1000_tx_ring *tx_ring;      /* One per active queue */
>  	unsigned long tx_queue_len;
> diff --git a/drivers/net/e1000/e1000_ethtool.c b/drivers/net/e1000/e1000_ethtool.c
> index 773821e..620afa5 100644
> --- a/drivers/net/e1000/e1000_ethtool.c
> +++ b/drivers/net/e1000/e1000_ethtool.c
> @@ -1819,61 +1819,15 @@ e1000_set_wol(struct net_device *netdev,
>  	return 0;
>  }
>  
> -/* toggle LED 4 times per second = 2 "blinks" per second */
> -#define E1000_ID_INTERVAL	(HZ/4)
> -
> -/* bit defines for adapter->led_status */
> -#define E1000_LED_ON		0
> -
> -static void
> -e1000_led_blink_callback(unsigned long data)
> -{
> -	struct e1000_adapter *adapter = (struct e1000_adapter *) data;
> -
> -	if (test_and_change_bit(E1000_LED_ON, &adapter->led_status))
> -		e1000_led_off(&adapter->hw);
> -	else
> -		e1000_led_on(&adapter->hw);
> -
> -	mod_timer(&adapter->blink_timer, jiffies + E1000_ID_INTERVAL);
> -}
> -
>  static int
>  e1000_phys_id(struct net_device *netdev, uint32_t data)
>  {
>  	struct e1000_adapter *adapter = netdev_priv(netdev);
>  
> -	if (!data || data > (uint32_t)(MAX_SCHEDULE_TIMEOUT / HZ))
> -		data = (uint32_t)(MAX_SCHEDULE_TIMEOUT / HZ);
> -
> -	if (adapter->hw.mac_type < e1000_82571) {
> -		if (!adapter->blink_timer.function) {
> -			init_timer(&adapter->blink_timer);
> -			adapter->blink_timer.function = e1000_led_blink_callback;
> -			adapter->blink_timer.data = (unsigned long) adapter;
> -		}
> -		e1000_setup_led(&adapter->hw);
> -		mod_timer(&adapter->blink_timer, jiffies);
> -		msleep_interruptible(data * 1000);
> -		del_timer_sync(&adapter->blink_timer);
> -	} else if (adapter->hw.phy_type == e1000_phy_ife) {
> -		if (!adapter->blink_timer.function) {
> -			init_timer(&adapter->blink_timer);
> -			adapter->blink_timer.function = e1000_led_blink_callback;
> -			adapter->blink_timer.data = (unsigned long) adapter;
> -		}
> -		mod_timer(&adapter->blink_timer, jiffies);
> -		msleep_interruptible(data * 1000);
> -		del_timer_sync(&adapter->blink_timer);
> -		e1000_write_phy_reg(&(adapter->hw), IFE_PHY_SPECIAL_CONTROL_LED, 0);
> -	} else {
> -		e1000_blink_led_start(&adapter->hw);
> -		msleep_interruptible(data * 1000);
> -	}
> +	if (data == 0)
> +		data = 2;
>  
> -	e1000_led_off(&adapter->hw);
> -	clear_bit(E1000_LED_ON, &adapter->led_status);
> -	e1000_cleanup_led(&adapter->hw);
> +	e1000_blink_led(&adapter->hw, data);
>  
>  	return 0;
>  }
> diff --git a/drivers/net/e1000/e1000_hw.c b/drivers/net/e1000/e1000_hw.c
> index 65077f3..db5e999 100644
> --- a/drivers/net/e1000/e1000_hw.c
> +++ b/drivers/net/e1000/e1000_hw.c
> @@ -6071,7 +6071,7 @@ e1000_id_led_init(struct e1000_hw * hw)
>   *
>   * hw - Struct containing variables accessed by shared code
>   *****************************************************************************/
> -int32_t
> +static int32_t
>  e1000_setup_led(struct e1000_hw *hw)
>  {
>      uint32_t ledctl;
> @@ -6123,50 +6123,11 @@ e1000_setup_led(struct e1000_hw *hw)
>  
>  
>  /******************************************************************************
> - * Used on 82571 and later Si that has LED blink bits.
> - * Callers must use their own timer and should have already called
> - * e1000_id_led_init()
> - * Call e1000_cleanup led() to stop blinking
> - *
> - * hw - Struct containing variables accessed by shared code
> - *****************************************************************************/
> -int32_t
> -e1000_blink_led_start(struct e1000_hw *hw)
> -{
> -    int16_t  i;
> -    uint32_t ledctl_blink = 0;
> -
> -    DEBUGFUNC("e1000_id_led_blink_on");
> -
> -    if (hw->mac_type < e1000_82571) {
> -        /* Nothing to do */
> -        return E1000_SUCCESS;
> -    }
> -    if (hw->media_type == e1000_media_type_fiber) {
> -        /* always blink LED0 for PCI-E fiber */
> -        ledctl_blink = E1000_LEDCTL_LED0_BLINK |
> -                     (E1000_LEDCTL_MODE_LED_ON << E1000_LEDCTL_LED0_MODE_SHIFT);
> -    } else {
> -        /* set the blink bit for each LED that's "on" (0x0E) in ledctl_mode2 */
> -        ledctl_blink = hw->ledctl_mode2;
> -        for (i=0; i < 4; i++)
> -            if (((hw->ledctl_mode2 >> (i * 8)) & 0xFF) ==
> -                E1000_LEDCTL_MODE_LED_ON)
> -                ledctl_blink |= (E1000_LEDCTL_LED0_BLINK << (i * 8));
> -    }
> -
> -    E1000_WRITE_REG(hw, LEDCTL, ledctl_blink);
> -
> -    return E1000_SUCCESS;
> -}
> -
> -/******************************************************************************
>   * Restores the saved state of the SW controlable LED.
>   *
>   * hw - Struct containing variables accessed by shared code
>   *****************************************************************************/
> -int32_t
> -e1000_cleanup_led(struct e1000_hw *hw)
> +static int32_t e1000_cleanup_led(struct e1000_hw *hw)
>  {
>      int32_t ret_val = E1000_SUCCESS;
>  
> @@ -6207,8 +6168,7 @@ e1000_cleanup_led(struct e1000_hw *hw)
>   *
>   * hw - Struct containing variables accessed by shared code
>   *****************************************************************************/
> -int32_t
> -e1000_led_on(struct e1000_hw *hw)
> +static int32_t e1000_led_on(struct e1000_hw *hw)
>  {
>      uint32_t ctrl = E1000_READ_REG(hw, CTRL);
>  
> @@ -6258,8 +6218,7 @@ e1000_led_on(struct e1000_hw *hw)
>   *
>   * hw - Struct containing variables accessed by shared code
>   *****************************************************************************/
> -int32_t
> -e1000_led_off(struct e1000_hw *hw)
> +static int32_t e1000_led_off(struct e1000_hw *hw)
>  {
>      uint32_t ctrl = E1000_READ_REG(hw, CTRL);
>  
> @@ -6304,6 +6263,69 @@ e1000_led_off(struct e1000_hw *hw)
>      return E1000_SUCCESS;
>  }
>  
> +static void e1000_blink_led_auto(struct e1000_hw *hw, unsigned int seconds)
> +{
> +    int16_t  i;
> +    uint32_t ledctl_blink = 0;
> +
> +    DEBUGFUNC("e1000_blink_led");
> +
> +    if (hw->media_type == e1000_media_type_fiber) {
> +        /* always blink LED0 for PCI-E fiber */
> +        ledctl_blink = E1000_LEDCTL_LED0_BLINK |
> +                     (E1000_LEDCTL_MODE_LED_ON << E1000_LEDCTL_LED0_MODE_SHIFT);
> +    } else {
> +        /* set the blink bit for each LED that's "on" (0x0E) in ledctl_mode2 */
> +        ledctl_blink = hw->ledctl_mode2;
> +        for (i=0; i < 4; i++)
> +            if (((hw->ledctl_mode2 >> (i * 8)) & 0xFF) ==
> +                E1000_LEDCTL_MODE_LED_ON)
> +                ledctl_blink |= (E1000_LEDCTL_LED0_BLINK << (i * 8));
> +    }
> +
> +    E1000_WRITE_REG(hw, LEDCTL, ledctl_blink);
> +
> +    msleep_interruptible(seconds * HZ);
> +
> +    e1000_led_off(hw);
> +    e1000_cleanup_led(hw);
> +}
> +
> +static void e1000_blink_led_manually(struct e1000_hw *hw, unsigned int seconds)
> +{
> +	unsigned int i;
> +	e1000_setup_led(hw);
> +
> +	for (i = 0; i < (seconds * 4); i++) {
> +		if (i & 1)
> +			e1000_led_off(hw);
> +		else
> +			e1000_led_on(hw);
> +		if (msleep_interruptible(250))
> +			break;
> +	}
> +
> +	e1000_led_off(hw);
> +	e1000_cleanup_led(hw);
> +}
> +
> +/******************************************************************************
> + * Blink the LED for the given number of seconds. 
> + *
> + * hw - Struct containing variables accessed by shared code
> + * seconds - Length of time to blink LED for
> + *****************************************************************************/
> +void e1000_blink_led(struct e1000_hw *hw, unsigned int seconds)
> +{
> +	if (hw->mac_type < e1000_82571) {
> +		e1000_blink_led_manually(hw, seconds);
> +	} else if (hw->phy_type == e1000_phy_ife) {
> +		e1000_blink_led_manually(hw, seconds);
> +	} else {
> +		e1000_blink_led_auto(hw, seconds);
> +	}
> +}
> +
>  /******************************************************************************
>   * Clears all hardware statistics counters.
>   *
> diff --git a/drivers/net/e1000/e1000_hw.h b/drivers/net/e1000/e1000_hw.h
> index 112447f..1efedb7 100644
> --- a/drivers/net/e1000/e1000_hw.h
> +++ b/drivers/net/e1000/e1000_hw.h
> @@ -404,11 +404,7 @@ void e1000_rar_set(struct e1000_hw *hw, 
>  void e1000_write_vfta(struct e1000_hw *hw, uint32_t offset, uint32_t value);
>  
>  /* LED functions */
> -int32_t e1000_setup_led(struct e1000_hw *hw);
> -int32_t e1000_cleanup_led(struct e1000_hw *hw);
> -int32_t e1000_led_on(struct e1000_hw *hw);
> -int32_t e1000_led_off(struct e1000_hw *hw);
> -int32_t e1000_blink_led_start(struct e1000_hw *hw);
> +void e1000_blink_led(struct e1000_hw *hw, unsigned int seconds);
>  
>  /* Adaptive IFS Functions */
>  
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Rewrite e100_phys_id

[Auke Kok]

Matthew Wilcox wrote:
> The motivator for this was to fix the sparse warning:
> 
> drivers/net/e100.c:2418:48: warning: cast truncates bits from constant
> value (83126e978d4fdf becomes 978d4fdf)
> drivers/net/e100.c:2419:37: warning: cast truncates bits from constant
> value (83126e978d4fdf becomes 978d4fdf)
> 
> Initially, I tried a quick fix, but when it ran into difficulties, I
> looked at tg3.c to see how it does it.  I liked their way better, so I
> rewrote e100.c to be similar.  It shaves ~700 bytes off the size of the
> driver, and a few bytes off the size of struct nic, so I think it's a
> win all round.  Tested on the internal interface of an HP Integrity rx2600.

bad news, it's completely hosed. The adapter does some indistinguishable blinking for a 
second, then stops blinking alltogether.

I might revert the code to the old situation. I guess I should have tested it initially 
right away.

I'm not even going to touch the e1000 patch for now ;)

Auke

> 
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> 
> diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> index a3a08a5..aade1e9 100644
> --- a/drivers/net/e100.c
> +++ b/drivers/net/e100.c
> @@ -556,7 +556,6 @@ struct nic {
>  	struct params params;
>  	struct net_device_stats net_stats;
>  	struct timer_list watchdog;
> -	struct timer_list blink_timer;
>  	struct mii_if_info mii;
>  	struct work_struct tx_timeout_task;
>  	enum loopback loopback;
> @@ -581,7 +580,6 @@ struct nic {
>  	u32 rx_over_length_errors;
>  
>  	u8 rev_id;
> -	u16 leds;
>  	u16 eeprom_wc;
>  	u16 eeprom[256];
>  	spinlock_t mdio_lock;
> @@ -2168,23 +2166,6 @@ err_clean_rx:
>  	return err;
>  }
>  
> -#define MII_LED_CONTROL	0x1B
> -static void e100_blink_led(unsigned long data)
> -{
> -	struct nic *nic = (struct nic *)data;
> -	enum led_state {
> -		led_on     = 0x01,
> -		led_off    = 0x04,
> -		led_on_559 = 0x05,
> -		led_on_557 = 0x07,
> -	};
> -
> -	nic->leds = (nic->leds & led_on) ? led_off :
> -		(nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559;
> -	mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, nic->leds);
> -	mod_timer(&nic->blink_timer, jiffies + HZ / 4);
> -}
> -
>  static int e100_get_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
>  {
>  	struct nic *nic = netdev_priv(netdev);
> @@ -2411,16 +2392,32 @@ static void e100_diag_test(struct net_de
>  	msleep_interruptible(4 * 1000);
>  }
>  
> +#define MII_LED_CONTROL	0x1B
>  static int e100_phys_id(struct net_device *netdev, u32 data)
>  {
>  	struct nic *nic = netdev_priv(netdev);
> +	int i;
> +
> +	enum led_state {
> +		led_off    = 0x04,
> +		led_on_559 = 0x05,
> +		led_on_557 = 0x07,
> +	};
> +	u16 leds = led_off;
> +
> +	if (data == 0)
> +		data = 2;
> +
> +	for (i = 0; i < (data * 2); i++) {
> +		leds = (leds == led_off) ?
> +			(nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559 :
> +			led_off;
> +		mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, leds);
> +		if (msleep_interruptible(500))
> +			break;
> +	}
>  
> -	if(!data || data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ))
> -		data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
> -	mod_timer(&nic->blink_timer, jiffies);
> -	msleep_interruptible(data * 1000);
> -	del_timer_sync(&nic->blink_timer);
> -	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, 0);
> +	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, led_off);
>  
>  	return 0;
>  }
> @@ -2633,9 +2630,6 @@ #endif
>  	init_timer(&nic->watchdog);
>  	nic->watchdog.function = e100_watchdog;
>  	nic->watchdog.data = (unsigned long)nic;
> -	init_timer(&nic->blink_timer);
> -	nic->blink_timer.function = e100_blink_led;
> -	nic->blink_timer.data = (unsigned long)nic;
>  
>  	INIT_WORK(&nic->tx_timeout_task,
>  		(void (*)(void *))e100_tx_timeout_task, netdev);

Re: [PATCH] Rewrite e100_phys_id

[Matthew Wilcox]

On Tue, Nov 07, 2006 at 10:33:14AM -0800, Auke Kok wrote:
> Matthew Wilcox wrote:
> >Tested on the internal interface of an HP Integrity rx2600.
> 
> bad news, it's completely hosed. The adapter does some indistinguishable 
> blinking for a second, then stops blinking alltogether.

Weird.  I tested it on the only e100 I have access to, and it worked.
I've just reviewed the patch you quoted below, and I don't see what the
problem is.  

I wonder if this is wrong:

> >-    mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, 0);
> >+    mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, led_off);

but everything else seems pretty straight-forward.

> I might revert the code to the old situation. I guess I should have tested 
> it initially right away.
> 
> I'm not even going to touch the e1000 patch for now ;)
> 
> Auke
> 
> >
> >Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> >
> >diff --git a/drivers/net/e100.c b/drivers/net/e100.c
> >index a3a08a5..aade1e9 100644
> >--- a/drivers/net/e100.c
> >+++ b/drivers/net/e100.c
> >@@ -556,7 +556,6 @@ struct nic {
> > 	struct params params;
> > 	struct net_device_stats net_stats;
> > 	struct timer_list watchdog;
> >-	struct timer_list blink_timer;
> > 	struct mii_if_info mii;
> > 	struct work_struct tx_timeout_task;
> > 	enum loopback loopback;
> >@@ -581,7 +580,6 @@ struct nic {
> > 	u32 rx_over_length_errors;
> > 
> > 	u8 rev_id;
> >-	u16 leds;
> > 	u16 eeprom_wc;
> > 	u16 eeprom[256];
> > 	spinlock_t mdio_lock;
> >@@ -2168,23 +2166,6 @@ err_clean_rx:
> > 	return err;
> > }
> > 
> >-#define MII_LED_CONTROL	0x1B
> >-static void e100_blink_led(unsigned long data)
> >-{
> >-	struct nic *nic = (struct nic *)data;
> >-	enum led_state {
> >-		led_on     = 0x01,
> >-		led_off    = 0x04,
> >-		led_on_559 = 0x05,
> >-		led_on_557 = 0x07,
> >-	};
> >-
> >-	nic->leds = (nic->leds & led_on) ? led_off :
> >-		(nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559;
> >-	mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, nic->leds);
> >-	mod_timer(&nic->blink_timer, jiffies + HZ / 4);
> >-}
> >-
> > static int e100_get_settings(struct net_device *netdev, struct 
> > ethtool_cmd *cmd)
> > {
> > 	struct nic *nic = netdev_priv(netdev);
> >@@ -2411,16 +2392,32 @@ static void e100_diag_test(struct net_de
> > 	msleep_interruptible(4 * 1000);
> > }
> > 
> >+#define MII_LED_CONTROL	0x1B
> > static int e100_phys_id(struct net_device *netdev, u32 data)
> > {
> > 	struct nic *nic = netdev_priv(netdev);
> >+	int i;
> >+
> >+	enum led_state {
> >+		led_off    = 0x04,
> >+		led_on_559 = 0x05,
> >+		led_on_557 = 0x07,
> >+	};
> >+	u16 leds = led_off;
> >+
> >+	if (data == 0)
> >+		data = 2;
> >+
> >+	for (i = 0; i < (data * 2); i++) {
> >+		leds = (leds == led_off) ?
> >+			(nic->mac < mac_82559_D101M) ? led_on_557 : 
> >led_on_559 :
> >+			led_off;
> >+		mdio_write(nic->netdev, nic->mii.phy_id, MII_LED_CONTROL, 
> >leds);
> >+		if (msleep_interruptible(500))
> >+			break;
> >+	}
> > 
> >-	if(!data || data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ))
> >-		data = (u32)(MAX_SCHEDULE_TIMEOUT / HZ);
> >-	mod_timer(&nic->blink_timer, jiffies);
> >-	msleep_interruptible(data * 1000);
> >-	del_timer_sync(&nic->blink_timer);
> >-	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, 0);
> >+	mdio_write(netdev, nic->mii.phy_id, MII_LED_CONTROL, led_off);
> > 
> > 	return 0;
> > }
> >@@ -2633,9 +2630,6 @@ #endif
> > 	init_timer(&nic->watchdog);
> > 	nic->watchdog.function = e100_watchdog;
> > 	nic->watchdog.data = (unsigned long)nic;
> >-	init_timer(&nic->blink_timer);
> >-	nic->blink_timer.function = e100_blink_led;
> >-	nic->blink_timer.data = (unsigned long)nic;
> > 
> > 	INIT_WORK(&nic->tx_timeout_task,
> > 		(void (*)(void *))e100_tx_timeout_task, netdev);

Re: [PATCH] Rewrite e100_phys_id

[Auke Kok]

Matthew Wilcox wrote:
> On Tue, Nov 07, 2006 at 10:33:14AM -0800, Auke Kok wrote:
>> Matthew Wilcox wrote:
>>> Tested on the internal interface of an HP Integrity rx2600.
>> bad news, it's completely hosed. The adapter does some indistinguishable 
>> blinking for a second, then stops blinking alltogether.
> 
> Weird.  I tested it on the only e100 I have access to, and it worked.
> I've just reviewed the patch you quoted below, and I don't see what the
> problem is.  

I don't understand it either, and will dig into this after I get more coffee.

point is that `ethtool -p` now exits immediately after 500ms. it should loop until ^C is 
pressed. Somehow msleep_interruptable is always returning 0 on my platform? very strange.

Auke