[PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Stephen Hemminger]

Did a bunch of the easy drivers to convert.

[PATCH net-next-2.6 01/12] sky2: support ethtool set_phys_id

[Stephen Hemminger]

[-- Attachment #1: sky2-phys-set.patch --]
[-- Type: text/plain, Size: 1446 bytes --]

Use ethtool set_phys_id to control LED. Fixes issues with RTNL being
held for extended periods.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/sky2.c	2011-04-04 09:24:59.413162054 -0700
+++ b/drivers/net/sky2.c	2011-04-04 10:48:48.338000416 -0700
@@ -3826,23 +3826,24 @@ static void sky2_led(struct sky2_port *s
 }
 
 /* blink LED's for finding board */
-static int sky2_phys_id(struct net_device *dev, u32 data)
+static int sky2_set_phys_id(struct net_device *dev,
+			    enum ethtool_phys_id_state state)
 {
 	struct sky2_port *sky2 = netdev_priv(dev);
-	unsigned int i;
-
-	if (data == 0)
-		data = UINT_MAX;
 
-	for (i = 0; i < data; i++) {
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
+	case ETHTOOL_ID_INACTIVE:
+		sky2_led(sky2, MO_LED_NORM);
+		break;
+	case ETHTOOL_ID_ON:
 		sky2_led(sky2, MO_LED_ON);
-		if (msleep_interruptible(500))
-			break;
+		break;
+	case ETHTOOL_ID_OFF:
 		sky2_led(sky2, MO_LED_OFF);
-		if (msleep_interruptible(500))
-			break;
+		break;
 	}
-	sky2_led(sky2, MO_LED_NORM);
 
 	return 0;
 }
@@ -4269,7 +4270,7 @@ static const struct ethtool_ops sky2_eth
 	.set_ringparam	= sky2_set_ringparam,
 	.get_pauseparam = sky2_get_pauseparam,
 	.set_pauseparam = sky2_set_pauseparam,
-	.phys_id	= sky2_phys_id,
+	.set_phys_id	= sky2_set_phys_id,
 	.get_sset_count = sky2_get_sset_count,
 	.get_ethtool_stats = sky2_get_ethtool_stats,
 	.set_flags	= sky2_set_flags,

[PATCH net-next-2.6 02/12] skge: implement set_phys_id

[Stephen Hemminger]

[-- Attachment #1: skge-phys-id.patch --]
[-- Type: text/plain, Size: 1664 bytes --]

Implement set_phys_id led control on SysKonnect board.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/skge.c	2011-04-04 10:48:15.713539204 -0700
+++ b/drivers/net/skge.c	2011-04-04 10:50:07.387094723 -0700
@@ -786,29 +786,28 @@ static void skge_led(struct skge_port *s
 }
 
 /* blink LED's for finding board */
-static int skge_phys_id(struct net_device *dev, u32 data)
+static int skge_set_phys_id(struct net_device *dev,
+			    enum ethtool_phys_id_state state)
 {
 	struct skge_port *skge = netdev_priv(dev);
-	unsigned long ms;
-	enum led_mode mode = LED_MODE_TST;
 
-	if (!data || data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ))
-		ms = jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT / HZ) * 1000;
-	else
-		ms = data * 1000;
-
-	while (ms > 0) {
-		skge_led(skge, mode);
-		mode ^= LED_MODE_TST;
-
-		if (msleep_interruptible(BLINK_MS))
-			break;
-		ms -= BLINK_MS;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -1;
+
+	case ETHTOOL_ID_ON:
+		skge_led(skge, LED_MODE_TST);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		skge_led(skge, LED_MODE_OFF);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		/* back to regular LED state */
+		skge_led(skge, netif_running(dev) ? LED_MODE_ON : LED_MODE_OFF);
 	}
 
-	/* back to regular LED state */
-	skge_led(skge, netif_running(dev) ? LED_MODE_ON : LED_MODE_OFF);
-
 	return 0;
 }
 
@@ -930,7 +929,7 @@ static const struct ethtool_ops skge_eth
 	.get_rx_csum	= skge_get_rx_csum,
 	.set_rx_csum	= skge_set_rx_csum,
 	.get_strings	= skge_get_strings,
-	.phys_id	= skge_phys_id,
+	.set_phys_id	= skge_set_phys_id,
 	.get_sset_count = skge_get_sset_count,
 	.get_ethtool_stats = skge_get_ethtool_stats,
 };

[PATCH net-next-2.6 03/12] e100: implemenet set_phys_id

[Stephen Hemminger]

[-- Attachment #1: e100-set-phys-id.patch --]
[-- Type: text/plain, Size: 3579 bytes --]

Implement set_phys_id to control LED. 
Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/e100.c	2011-04-04 10:51:19.340063286 -0700
+++ b/drivers/net/e100.c	2011-04-04 11:04:55.117587486 -0700
@@ -593,7 +593,6 @@ struct nic {
 	enum phy phy;
 	struct params params;
 	struct timer_list watchdog;
-	struct timer_list blink_timer;
 	struct mii_if_info mii;
 	struct work_struct tx_timeout_task;
 	enum loopback loopback;
@@ -618,7 +617,6 @@ struct nic {
 	u32 rx_tco_frames;
 	u32 rx_over_length_errors;
 
-	u16 leds;
 	u16 eeprom_wc;
 	__le16 eeprom[256];
 	spinlock_t mdio_lock;
@@ -2351,30 +2349,6 @@ err_clean_rx:
 #define E100_82552_LED_OVERRIDE 0x19
 #define E100_82552_LED_ON       0x000F /* LEDTX and LED_RX both on */
 #define E100_82552_LED_OFF      0x000A /* LEDTX and LED_RX both off */
-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,
-	};
-	u16 led_reg = MII_LED_CONTROL;
-
-	if (nic->phy == phy_82552_v) {
-		led_reg = E100_82552_LED_OVERRIDE;
-
-		nic->leds = (nic->leds == E100_82552_LED_ON) ?
-		            E100_82552_LED_OFF : E100_82552_LED_ON;
-	} else {
-		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, led_reg, nic->leds);
-	mod_timer(&nic->blink_timer, jiffies + HZ / 4);
-}
 
 static int e100_get_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
 {
@@ -2598,19 +2572,38 @@ static void e100_diag_test(struct net_de
 	msleep_interruptible(4 * 1000);
 }
 
-static int e100_phys_id(struct net_device *netdev, u32 data)
+static int e100_set_phys_id(struct net_device *netdev,
+			    enum ethtool_phys_id_state state)
 {
 	struct nic *nic = netdev_priv(netdev);
+	enum led_state {
+		led_on     = 0x01,
+		led_off    = 0x04,
+		led_on_559 = 0x05,
+		led_on_557 = 0x07,
+	};
 	u16 led_reg = (nic->phy == phy_82552_v) ? E100_82552_LED_OVERRIDE :
-	              MII_LED_CONTROL;
+		MII_LED_CONTROL;
+	u16 leds = 0;
+
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		leds = (nic->phy == phy_82552_v) ? E100_82552_LED_ON :
+		       (nic->mac < mac_82559_D101M) ? led_on_557 : led_on_559;
+		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, led_reg, 0);
+	case ETHTOOL_ID_OFF:
+		leds = (nic->phy == phy_82552_v) ? E100_82552_LED_OFF : led_off;
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		break;
+	}
 
+	mdio_write(netdev, nic->mii.phy_id, led_reg, leds);
 	return 0;
 }
 
@@ -2691,7 +2684,7 @@ static const struct ethtool_ops e100_eth
 	.set_ringparam		= e100_set_ringparam,
 	.self_test		= e100_diag_test,
 	.get_strings		= e100_get_strings,
-	.phys_id		= e100_phys_id,
+	.set_phys_id		= e100_set_phys_id,
 	.get_ethtool_stats	= e100_get_ethtool_stats,
 	.get_sset_count		= e100_get_sset_count,
 };
@@ -2832,9 +2825,6 @@ static int __devinit e100_probe(struct p
 	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, e100_tx_timeout_task);
 

[PATCH net-next-2.6 04/12] e1000: convert to set_phys_id

[Stephen Hemminger]

[-- Attachment #1: e1000-set-phys-id.patch --]
[-- Type: text/plain, Size: 2972 bytes --]

Convert to new LED control infrastucture and remove no longer
necessary bits.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 drivers/net/e1000/e1000.h         |    3 --
 drivers/net/e1000/e1000_ethtool.c |   50 ++++++++++++--------------------------
 2 files changed, 16 insertions(+), 37 deletions(-)

--- a/drivers/net/e1000/e1000.h	2011-04-04 11:22:23.951992540 -0700
+++ b/drivers/net/e1000/e1000.h	2011-04-04 11:22:32.832071659 -0700
@@ -238,9 +238,6 @@ struct e1000_adapter {
 	struct work_struct reset_task;
 	u8 fc_autoneg;
 
-	struct timer_list blink_timer;
-	unsigned long led_status;
-
 	/* TX */
 	struct e1000_tx_ring *tx_ring;      /* One per active queue */
 	unsigned int restart_queue;
--- a/drivers/net/e1000/e1000_ethtool.c	2011-04-04 11:21:31.663536366 -0700
+++ b/drivers/net/e1000/e1000_ethtool.c	2011-04-04 11:26:51.978552805 -0700
@@ -1753,46 +1753,28 @@ static int e1000_set_wol(struct net_devi
 	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)
+static int e1000_set_phys_id(struct net_device *netdev,
+			     enum ethtool_phys_id_state state)
 {
-	struct e1000_adapter *adapter = (struct e1000_adapter *) data;
+	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 
-	if (test_and_change_bit(E1000_LED_ON, &adapter->led_status))
-		e1000_led_off(hw);
-	else
-		e1000_led_on(hw);
-
-	mod_timer(&adapter->blink_timer, jiffies + E1000_ID_INTERVAL);
-}
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		e1000_setup_led(hw);
+		return -EINVAL;
 
-static int e1000_phys_id(struct net_device *netdev, u32 data)
-{
-	struct e1000_adapter *adapter = netdev_priv(netdev);
-	struct e1000_hw *hw = &adapter->hw;
+	case ETHTOOL_ID_ON:
+		e1000_led_on(hw);
+		break;
 
-	if (!data)
-		data = INT_MAX;
+	case ETHTOOL_ID_OFF:
+		e1000_led_off(hw);
+		break;
 
-	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;
+	case ETHTOOL_ID_INACTIVE:
+		e1000_cleanup_led(hw);
 	}
-	e1000_setup_led(hw);
-	mod_timer(&adapter->blink_timer, jiffies);
-	msleep_interruptible(data * 1000);
-	del_timer_sync(&adapter->blink_timer);
-
-	e1000_led_off(hw);
-	clear_bit(E1000_LED_ON, &adapter->led_status);
-	e1000_cleanup_led(hw);
 
 	return 0;
 }
@@ -1929,7 +1911,7 @@ static const struct ethtool_ops e1000_et
 	.set_tso                = e1000_set_tso,
 	.self_test              = e1000_diag_test,
 	.get_strings            = e1000_get_strings,
-	.phys_id                = e1000_phys_id,
+	.set_phys_id            = e1000_set_phys_id,
 	.get_ethtool_stats      = e1000_get_ethtool_stats,
 	.get_sset_count         = e1000_get_sset_count,
 	.get_coalesce           = e1000_get_coalesce,

[PATCH net-next-2.6 05/12] e1000e: implement ethtool set_phys_id

[Stephen Hemminger]

[-- Attachment #1: e1000e-phys-id.patch --]
[-- Type: text/plain, Size: 5037 bytes --]

The new ethtool set_phys_id takes over controlling the LED for identifying
boards. This fixes the lockout during that period.
For this device lots of extra infrastructure can also be removed by using
set_phys_id.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/e1000e/e1000.h	2011-04-04 10:48:18.801583226 -0700
+++ b/drivers/net/e1000e/e1000.h	2011-04-04 10:49:31.254596880 -0700
@@ -389,13 +389,10 @@ struct e1000_adapter {
 
 	bool fc_autoneg;
 
-	unsigned long led_status;
-
 	unsigned int flags;
 	unsigned int flags2;
 	struct work_struct downshift_task;
 	struct work_struct update_phy_task;
-	struct work_struct led_blink_task;
 	struct work_struct print_hang_task;
 
 	bool idle_check;
@@ -484,7 +481,6 @@ extern const char e1000e_driver_version[
 
 extern void e1000e_check_options(struct e1000_adapter *adapter);
 extern void e1000e_set_ethtool_ops(struct net_device *netdev);
-extern void e1000e_led_blink_task(struct work_struct *work);
 
 extern int e1000e_up(struct e1000_adapter *adapter);
 extern void e1000e_down(struct e1000_adapter *adapter);
--- a/drivers/net/e1000e/ethtool.c	2011-04-04 10:48:18.789583047 -0700
+++ b/drivers/net/e1000e/ethtool.c	2011-04-04 10:49:48.798839237 -0700
@@ -1851,64 +1851,39 @@ static int e1000_set_wol(struct net_devi
 	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
-
-void e1000e_led_blink_task(struct work_struct *work)
-{
-	struct e1000_adapter *adapter = container_of(work,
-	                                struct e1000_adapter, led_blink_task);
-
-	if (test_and_change_bit(E1000_LED_ON, &adapter->led_status))
-		adapter->hw.mac.ops.led_off(&adapter->hw);
-	else
-		adapter->hw.mac.ops.led_on(&adapter->hw);
-}
-
-static void e1000_led_blink_callback(unsigned long data)
-{
-	struct e1000_adapter *adapter = (struct e1000_adapter *) data;
-
-	schedule_work(&adapter->led_blink_task);
-	mod_timer(&adapter->blink_timer, jiffies + E1000_ID_INTERVAL);
-}
-
-static int e1000_phys_id(struct net_device *netdev, u32 data)
+static int e1000_set_phys_id(struct net_device *netdev,
+			     enum ethtool_phys_id_state state)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
 
-	if (!data)
-		data = INT_MAX;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		if ((hw->phy.type == e1000_phy_ife) ||
+		    (hw->mac.type == e1000_pchlan) ||
+		    (hw->mac.type == e1000_pch2lan) ||
+		    (hw->mac.type == e1000_82583) ||
+		    (hw->mac.type == e1000_82574))
+			return -EINVAL;	/* blink in software */
 
-	if ((hw->phy.type == e1000_phy_ife) ||
-	    (hw->mac.type == e1000_pchlan) ||
-	    (hw->mac.type == e1000_pch2lan) ||
-	    (hw->mac.type == e1000_82583) ||
-	    (hw->mac.type == e1000_82574)) {
-		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);
+		e1000e_blink_led(hw);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
 		if (hw->phy.type == e1000_phy_ife)
 			e1e_wphy(hw, IFE_PHY_SPECIAL_CONTROL_LED, 0);
-	} else {
-		e1000e_blink_led(hw);
-		msleep_interruptible(data * 1000);
-	}
+		hw->mac.ops.led_off(hw);
+		hw->mac.ops.cleanup_led(hw);
+		break;
 
-	hw->mac.ops.led_off(hw);
-	clear_bit(E1000_LED_ON, &adapter->led_status);
-	hw->mac.ops.cleanup_led(hw);
+	case ETHTOOL_ID_ON:
+		adapter->hw.mac.ops.led_on(&adapter->hw);
+		break;
 
+	case ETHTOOL_ID_OFF:
+		adapter->hw.mac.ops.led_off(&adapter->hw);
+		break;
+	}
 	return 0;
 }
 
@@ -2049,7 +2024,7 @@ static const struct ethtool_ops e1000_et
 	.set_tso		= e1000_set_tso,
 	.self_test		= e1000_diag_test,
 	.get_strings		= e1000_get_strings,
-	.phys_id		= e1000_phys_id,
+	.set_phys_id		= e1000_set_phys_id,
 	.get_ethtool_stats	= e1000_get_ethtool_stats,
 	.get_sset_count		= e1000e_get_sset_count,
 	.get_coalesce		= e1000_get_coalesce,
--- a/drivers/net/e1000e/netdev.c	2011-04-04 10:48:18.821583507 -0700
+++ b/drivers/net/e1000e/netdev.c	2011-04-04 10:49:31.258596933 -0700
@@ -5991,7 +5991,6 @@ static int __devinit e1000_probe(struct
 	INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
 	INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
 	INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang);
-	INIT_WORK(&adapter->led_blink_task, e1000e_led_blink_task);
 
 	/* Initialize link parameters. User can change them with ethtool */
 	adapter->hw.mac.autoneg = 1;
@@ -6124,7 +6123,6 @@ static void __devexit e1000_remove(struc
 	cancel_work_sync(&adapter->watchdog_task);
 	cancel_work_sync(&adapter->downshift_task);
 	cancel_work_sync(&adapter->update_phy_task);
-	cancel_work_sync(&adapter->led_blink_task);
 	cancel_work_sync(&adapter->print_hang_task);
 
 	if (!(netdev->flags & IFF_UP))

[PATCH net-next-2.6 06/12] igb: convert to ethtool set_phys_id

[Stephen Hemminger]

[-- Attachment #1: igb-set-phys-id.patch --]
[-- Type: text/plain, Size: 1782 bytes --]

Convert igb driver to use new set_phys_id ethtool interface.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/igb/igb_ethtool.c	2011-04-04 10:48:11.725482218 -0700
+++ b/drivers/net/igb/igb_ethtool.c	2011-04-04 10:50:38.243512749 -0700
@@ -1963,29 +1963,26 @@ static int igb_set_wol(struct net_device
 /* bit defines for adapter->led_status */
 #define IGB_LED_ON		0
 
-static int igb_phys_id(struct net_device *netdev, u32 data)
+static int igb_set_phys_id(struct net_device *netdev,
+			   enum ethtool_phys_id_state state)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	struct e1000_hw *hw = &adapter->hw;
-	unsigned long timeout;
 
-	timeout = data * 1000;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		igb_blink_led(hw);
+		return 0;
+
+	case ETHTOOL_ID_INACTIVE:
+		igb_led_off(hw);
+		clear_bit(IGB_LED_ON, &adapter->led_status);
+		igb_cleanup_led(hw);
+		return 0;
 
-	/*
-	 *  msleep_interruptable only accepts unsigned int so we are limited
-	 * in how long a duration we can wait
-	 */
-	if (!timeout || timeout > UINT_MAX)
-		timeout = UINT_MAX;
-
-	igb_blink_led(hw);
-	msleep_interruptible(timeout);
-
-	igb_led_off(hw);
-	clear_bit(IGB_LED_ON, &adapter->led_status);
-	igb_cleanup_led(hw);
-
-	return 0;
+	default:
+		return -EINVAL;
+	}
 }
 
 static int igb_set_coalesce(struct net_device *netdev,
@@ -2215,7 +2212,7 @@ static const struct ethtool_ops igb_etht
 	.set_tso                = igb_set_tso,
 	.self_test              = igb_diag_test,
 	.get_strings            = igb_get_strings,
-	.phys_id                = igb_phys_id,
+	.set_phys_id            = igb_set_phys_id,
 	.get_sset_count         = igb_get_sset_count,
 	.get_ethtool_stats      = igb_get_ethtool_stats,
 	.get_coalesce           = igb_get_coalesce,

[PATCH net-next-2.6 07/12] igbvf: remove bogus phys_id

[Stephen Hemminger]

[-- Attachment #1: igvf-set-phys.patch --]
[-- Type: text/plain, Size: 927 bytes --]

This device lies about supporting phys_id. Remove it and just
let the upper layer report not supported.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/igbvf/ethtool.c	2011-04-04 11:15:50.477019338 -0700
+++ b/drivers/net/igbvf/ethtool.c	2011-04-04 11:16:12.661202774 -0700
@@ -391,11 +391,6 @@ static int igbvf_set_wol(struct net_devi
 	return -EOPNOTSUPP;
 }
 
-static int igbvf_phys_id(struct net_device *netdev, u32 data)
-{
-	return 0;
-}
-
 static int igbvf_get_coalesce(struct net_device *netdev,
                               struct ethtool_coalesce *ec)
 {
@@ -527,7 +522,6 @@ static const struct ethtool_ops igbvf_et
 	.self_test		= igbvf_diag_test,
 	.get_sset_count		= igbvf_get_sset_count,
 	.get_strings		= igbvf_get_strings,
-	.phys_id		= igbvf_phys_id,
 	.get_ethtool_stats	= igbvf_get_ethtool_stats,
 	.get_coalesce		= igbvf_get_coalesce,
 	.set_coalesce		= igbvf_set_coalesce,

[PATCH net-next-2.6 08/12] ixgbe: convert to ethtool set_phys_id

[Stephen Hemminger]

[-- Attachment #1: ixgbe-set-phys-id.patch --]
[-- Type: text/plain, Size: 1702 bytes --]

Convert ixgbe driver to use new set_phys_id ethtool interface.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/ixgbe/ixgbe_ethtool.c	2011-04-04 10:48:11.641481015 -0700
+++ b/drivers/net/ixgbe/ixgbe_ethtool.c	2011-04-04 10:50:57.979778947 -0700
@@ -1999,25 +1999,29 @@ static int ixgbe_nway_reset(struct net_d
 	return 0;
 }
 
-static int ixgbe_phys_id(struct net_device *netdev, u32 data)
+static int ixgbe_set_phys_id(struct net_device *netdev,
+			     enum ethtool_phys_id_state state)
 {
 	struct ixgbe_adapter *adapter = netdev_priv(netdev);
 	struct ixgbe_hw *hw = &adapter->hw;
 	u32 led_reg = IXGBE_READ_REG(hw, IXGBE_LEDCTL);
-	u32 i;
 
-	if (!data || data > 300)
-		data = 300;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -1;
 
-	for (i = 0; i < (data * 1000); i += 400) {
+	case ETHTOOL_ID_ON:
 		hw->mac.ops.led_on(hw, IXGBE_LED_ON);
-		msleep_interruptible(200);
+		break;
+
+	case ETHTOOL_ID_OFF:
 		hw->mac.ops.led_off(hw, IXGBE_LED_ON);
-		msleep_interruptible(200);
-	}
+		break;
 
-	/* Restore LED settings */
-	IXGBE_WRITE_REG(&adapter->hw, IXGBE_LEDCTL, led_reg);
+	case ETHTOOL_ID_INACTIVE:
+		/* Restore LED settings */
+		IXGBE_WRITE_REG(&adapter->hw, IXGBE_LEDCTL, led_reg);
+	}
 
 	return 0;
 }
@@ -2465,7 +2469,7 @@ static const struct ethtool_ops ixgbe_et
 	.set_tso                = ixgbe_set_tso,
 	.self_test              = ixgbe_diag_test,
 	.get_strings            = ixgbe_get_strings,
-	.phys_id                = ixgbe_phys_id,
+	.set_phys_id            = ixgbe_set_phys_id,
 	.get_sset_count         = ixgbe_get_sset_count,
 	.get_ethtool_stats      = ixgbe_get_ethtool_stats,
 	.get_coalesce           = ixgbe_get_coalesce,

[PATCH net-next-2.6 09/12] ixgb: convert to set_phys_id

[Stephen Hemminger]

[-- Attachment #1: ixgb-set-phys.patch --]
[-- Type: text/plain, Size: 2366 bytes --]

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/ixgb/ixgb.h	2011-04-04 11:38:54.837945742 -0700
+++ b/drivers/net/ixgb/ixgb.h	2011-04-04 11:38:59.441992978 -0700
@@ -157,9 +157,6 @@ struct ixgb_adapter {
 	u16 link_duplex;
 	struct work_struct tx_timeout_task;
 
-	struct timer_list blink_timer;
-	unsigned long led_status;
-
 	/* TX */
 	struct ixgb_desc_ring tx_ring ____cacheline_aligned_in_smp;
 	unsigned int restart_queue;
--- a/drivers/net/ixgb/ixgb_ethtool.c	2011-04-04 11:38:04.089428786 -0700
+++ b/drivers/net/ixgb/ixgb_ethtool.c	2011-04-04 11:40:39.458950195 -0700
@@ -610,45 +610,23 @@ err_setup_rx:
 	return err;
 }
 
-/* toggle LED 4 times per second = 2 "blinks" per second */
-#define IXGB_ID_INTERVAL	(HZ/4)
-
-/* bit defines for adapter->led_status */
-#define IXGB_LED_ON		0
-
-static void
-ixgb_led_blink_callback(unsigned long data)
-{
-	struct ixgb_adapter *adapter = (struct ixgb_adapter *)data;
-
-	if (test_and_change_bit(IXGB_LED_ON, &adapter->led_status))
-		ixgb_led_off(&adapter->hw);
-	else
-		ixgb_led_on(&adapter->hw);
-
-	mod_timer(&adapter->blink_timer, jiffies + IXGB_ID_INTERVAL);
-}
-
 static int
-ixgb_phys_id(struct net_device *netdev, u32 data)
+ixgb_set_phys_id(struct net_device *netdev, enum ethtool_phys_id_state state)
 {
 	struct ixgb_adapter *adapter = netdev_priv(netdev);
 
-	if (!data)
-		data = INT_MAX;
-
-	if (!adapter->blink_timer.function) {
-		init_timer(&adapter->blink_timer);
-		adapter->blink_timer.function = ixgb_led_blink_callback;
-		adapter->blink_timer.data = (unsigned long)adapter;
-	}
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
 
-	mod_timer(&adapter->blink_timer, jiffies);
+	case ETHTOOL_ID_ON:
+		ixgb_led_on(&adapter->hw);
+		break;
 
-	msleep_interruptible(data * 1000);
-	del_timer_sync(&adapter->blink_timer);
-	ixgb_led_off(&adapter->hw);
-	clear_bit(IXGB_LED_ON, &adapter->led_status);
+	case ETHTOOL_ID_OFF:
+	case ETHTOOL_ID_INACTIVE:
+		ixgb_led_off(&adapter->hw);
+	}
 
 	return 0;
 }
@@ -766,7 +744,7 @@ static const struct ethtool_ops ixgb_eth
 	.set_msglevel = ixgb_set_msglevel,
 	.set_tso = ixgb_set_tso,
 	.get_strings = ixgb_get_strings,
-	.phys_id = ixgb_phys_id,
+	.set_phys_id = ixgb_set_phys_id,
 	.get_sset_count = ixgb_get_sset_count,
 	.get_ethtool_stats = ixgb_get_ethtool_stats,
 	.get_flags = ethtool_op_get_flags,

[PATCH net-next-2.6 10/12] tg3: implement ethtool set_phys_id

[Stephen Hemminger]

[-- Attachment #1: tg3-set-phys-id.patch --]
[-- Type: text/plain, Size: 2073 bytes --]

Implement control of LED via set_phys_id. 
Note: since PHY is powered off if device is down, this board
only allows blinking if device is up.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/tg3.c	2011-04-04 11:04:20.373199183 -0700
+++ b/drivers/net/tg3.c	2011-04-04 11:05:19.485863394 -0700
@@ -10317,35 +10317,38 @@ static void tg3_get_strings(struct net_d
 	}
 }
 
-static int tg3_phys_id(struct net_device *dev, u32 data)
+static int tg3_set_phys_id(struct net_device *dev,
+			    enum ethtool_phys_id_state state)
 {
 	struct tg3 *tp = netdev_priv(dev);
-	int i;
 
 	if (!netif_running(tp->dev))
 		return -EAGAIN;
 
-	if (data == 0)
-		data = UINT_MAX / 2;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
 
-	for (i = 0; i < (data * 2); i++) {
-		if ((i % 2) == 0)
-			tw32(MAC_LED_CTRL, LED_CTRL_LNKLED_OVERRIDE |
-					   LED_CTRL_1000MBPS_ON |
-					   LED_CTRL_100MBPS_ON |
-					   LED_CTRL_10MBPS_ON |
-					   LED_CTRL_TRAFFIC_OVERRIDE |
-					   LED_CTRL_TRAFFIC_BLINK |
-					   LED_CTRL_TRAFFIC_LED);
+	case ETHTOOL_ID_ON:
+		tw32(MAC_LED_CTRL, LED_CTRL_LNKLED_OVERRIDE |
+		     LED_CTRL_1000MBPS_ON |
+		     LED_CTRL_100MBPS_ON |
+		     LED_CTRL_10MBPS_ON |
+		     LED_CTRL_TRAFFIC_OVERRIDE |
+		     LED_CTRL_TRAFFIC_BLINK |
+		     LED_CTRL_TRAFFIC_LED);
+		break;
 
-		else
-			tw32(MAC_LED_CTRL, LED_CTRL_LNKLED_OVERRIDE |
-					   LED_CTRL_TRAFFIC_OVERRIDE);
+	case ETHTOOL_ID_OFF:
+		tw32(MAC_LED_CTRL, LED_CTRL_LNKLED_OVERRIDE |
+		     LED_CTRL_TRAFFIC_OVERRIDE);
+		break;
 
-		if (msleep_interruptible(500))
-			break;
+	case ETHTOOL_ID_INACTIVE:
+		tw32(MAC_LED_CTRL, tp->led_ctrl);
+		break;
 	}
-	tw32(MAC_LED_CTRL, tp->led_ctrl);
+
 	return 0;
 }
 
@@ -11371,7 +11374,7 @@ static const struct ethtool_ops tg3_etht
 	.set_tso		= tg3_set_tso,
 	.self_test		= tg3_self_test,
 	.get_strings		= tg3_get_strings,
-	.phys_id		= tg3_phys_id,
+	.set_phys_id		= tg3_set_phys_id,
 	.get_ethtool_stats	= tg3_get_ethtool_stats,
 	.get_coalesce		= tg3_get_coalesce,
 	.set_coalesce		= tg3_set_coalesce,

[PATCH net-next-2.6 11/12] cxgb3: implement set_phys_id

[Stephen Hemminger]

[-- Attachment #1: chelsio-set-phys.patch --]
[-- Type: text/plain, Size: 1459 bytes --]

Implement new ethtool set_phys_id on Chelsio cxgb3 board.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/cxgb3/cxgb3_main.c	2011-04-04 11:05:44.658151093 -0700
+++ b/drivers/net/cxgb3/cxgb3_main.c	2011-04-04 11:08:37.060177688 -0700
@@ -1749,23 +1749,26 @@ static int restart_autoneg(struct net_de
 	return 0;
 }
 
-static int cxgb3_phys_id(struct net_device *dev, u32 data)
+static int set_phys_id(struct net_device *dev,
+		       enum ethtool_phys_id_state state)
 {
 	struct port_info *pi = netdev_priv(dev);
 	struct adapter *adapter = pi->adapter;
-	int i;
 
-	if (data == 0)
-		data = 2;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
+
+	case ETHTOOL_ID_OFF:
+		t3_set_reg_field(adapter, A_T3DBG_GPIO_EN, F_GPIO0_OUT_VAL, 0);
+		break;
 
-	for (i = 0; i < data * 2; i++) {
+	case ETHTOOL_ID_ON:
+	case ETHTOOL_ID_INACTIVE:
 		t3_set_reg_field(adapter, A_T3DBG_GPIO_EN, F_GPIO0_OUT_VAL,
-				 (i & 1) ? F_GPIO0_OUT_VAL : 0);
-		if (msleep_interruptible(500))
-			break;
-	}
-	t3_set_reg_field(adapter, A_T3DBG_GPIO_EN, F_GPIO0_OUT_VAL,
 			 F_GPIO0_OUT_VAL);
+	}
+
 	return 0;
 }
 
@@ -2107,7 +2110,7 @@ static const struct ethtool_ops cxgb_eth
 	.set_sg = ethtool_op_set_sg,
 	.get_link = ethtool_op_get_link,
 	.get_strings = get_strings,
-	.phys_id = cxgb3_phys_id,
+	.set_phys_id = set_phys_id,
 	.nway_reset = restart_autoneg,
 	.get_sset_count = get_sset_count,
 	.get_ethtool_stats = get_stats,

[PATCH net-next-2.6 12/12] qlcnic: convert to set_phys_id

[Stephen Hemminger]

[-- Attachment #1: qlcnic-set-phys.patch --]
[-- Type: text/plain, Size: 2635 bytes --]

Convert driver to use new ethtool set_phys_id.
Not completely sure that this is correct for all cases of device
up/down and doing operation. Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/qlcnic/qlcnic_ethtool.c	2011-04-04 11:30:54.689092370 -0700
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c	2011-04-04 11:36:55.776744627 -0700
@@ -831,48 +831,51 @@ static int qlcnic_set_tso(struct net_dev
 	return 0;
 }
 
-static int qlcnic_blink_led(struct net_device *dev, u32 val)
+static int qlcnic_set_led(struct net_device *dev,
+			  enum ethtool_phys_id_state state)
 {
 	struct qlcnic_adapter *adapter = netdev_priv(dev);
 	int max_sds_rings = adapter->max_sds_rings;
-	int dev_down = 0;
 	int ret;
 
-	if (!test_bit(__QLCNIC_DEV_UP, &adapter->state)) {
-		dev_down = 1;
-		if (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
-			return -EIO;
-
-		ret = qlcnic_diag_alloc_res(dev, QLCNIC_LED_TEST);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		if (!test_bit(__QLCNIC_DEV_UP, &adapter->state)) {
+			if (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
+				return -EIO;
+
+			ret = qlcnic_diag_alloc_res(dev, QLCNIC_LED_TEST);
+			if (ret) {
+				clear_bit(__QLCNIC_RESETTING, &adapter->state);
+				return ret;
+			}
+		}
+
+		ret = adapter->nic_ops->config_led(adapter, 1, 0xf);
 		if (ret) {
-			clear_bit(__QLCNIC_RESETTING, &adapter->state);
-			return ret;
+			dev_err(&adapter->pdev->dev,
+				"Failed to set LED blink state.\n");
+			break;
 		}
-	}
-
-	ret = adapter->nic_ops->config_led(adapter, 1, 0xf);
-	if (ret) {
-		dev_err(&adapter->pdev->dev,
-			"Failed to set LED blink state.\n");
-		goto done;
-	}
+		return 0;
 
-	msleep_interruptible(val * 1000);
+	case ETHTOOL_ID_INACTIVE:
+		ret = adapter->nic_ops->config_led(adapter, 0, 0xf);
+		if (ret)
+			dev_err(&adapter->pdev->dev,
+				"Failed to reset LED blink state.\n");
+		break;
 
-	ret = adapter->nic_ops->config_led(adapter, 0, 0xf);
-	if (ret) {
-		dev_err(&adapter->pdev->dev,
-			"Failed to reset LED blink state.\n");
-		goto done;
+	default:
+		return -EINVAL;
 	}
 
-done:
-	if (dev_down) {
+	if (!netif_running(dev)) {
 		qlcnic_diag_free_res(dev, max_sds_rings);
 		clear_bit(__QLCNIC_RESETTING, &adapter->state);
 	}
-	return ret;
 
+	return ret;
 }
 
 static void
@@ -1080,7 +1083,7 @@ const struct ethtool_ops qlcnic_ethtool_
 	.set_coalesce = qlcnic_set_intr_coalesce,
 	.get_flags = ethtool_op_get_flags,
 	.set_flags = qlcnic_set_flags,
-	.phys_id = qlcnic_blink_led,
+	.set_phys_id = qlcnic_set_led,
 	.set_msglevel = qlcnic_set_msglevel,
 	.get_msglevel = qlcnic_get_msglevel,
 };

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Jeff Kirsher]

On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> Did a bunch of the easy drivers to convert.
>
>

I have added patches 3-9 (Intel driver conversions) to my queue of
patches.  Thanks Stephen.

-- 
Cheers,
Jeff

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 4 Apr 2011 12:53:14 -0700

> On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
>> Did a bunch of the easy drivers to convert.
>>
>>
> 
> I have added patches 3-9 (Intel driver conversions) to my queue of
> patches.  Thanks Stephen.

Careful, Ben's commit to add the interface still isn't in the tree.

He needs to respin his patch set based upon feedback that came in
the other day.

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Stephen Hemminger]

On Mon, 4 Apr 2011 12:53:14 -0700
Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:

> On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > Did a bunch of the easy drivers to convert.
> >
> >
> 
> I have added patches 3-9 (Intel driver conversions) to my queue of
> patches.  Thanks Stephen.

I changed the behaviour of igbvf for 'ethtool -p' to return not supported.
This seems correct, but it may break somebody who has some weird script running
in a virt environment.

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Ben Hutchings]

On Mon, 2011-04-04 at 13:14 -0700, Stephen Hemminger wrote:
> On Mon, 4 Apr 2011 12:53:14 -0700
> Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> 
> > On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > > Did a bunch of the easy drivers to convert.
> > >
> > >
> > 
> > I have added patches 3-9 (Intel driver conversions) to my queue of
> > patches.  Thanks Stephen.
> 
> I changed the behaviour of igbvf for 'ethtool -p' to return not supported.
> This seems correct, but it may break somebody who has some weird script running
> in a virt environment.

Surely it should be connected to a virtual LED.  Doesn't virt-manager
have those yet? ;-)

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

On Mon, 2011-04-04 at 13:14 -0700, Stephen Hemminger wrote:
> On Mon, 4 Apr 2011 12:53:14 -0700
> Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> 
> > On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > > Did a bunch of the easy drivers to convert.
> > >
> > >
> > 
> > I have added patches 3-9 (Intel driver conversions) to my queue of
> > patches.  Thanks Stephen.
> 
> I changed the behaviour of igbvf for 'ethtool -p' to return not supported.
> This seems correct, but it may break somebody who has some weird script running
> in a virt environment.

I also noticed you did not convert ixgbevf, so we will take a look at
your changes for igbvf and come up with a patch for ixgbevf.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 688 bytes --]

On Mon, 2011-04-04 at 13:06 -0700, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Mon, 4 Apr 2011 12:53:14 -0700
> 
> > On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> >> Did a bunch of the easy drivers to convert.
> >>
> >>
> > 
> > I have added patches 3-9 (Intel driver conversions) to my queue of
> > patches.  Thanks Stephen.
> 
> Careful, Ben's commit to add the interface still isn't in the tree.
> 
> He needs to respin his patch set based upon feedback that came in
> the other day.

Yeah, I caught that.  I will be adding the dependent patches to my
internal tree for validation purposes only.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Stephen Hemminger]

On Mon, 04 Apr 2011 13:32:44 -0700
Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:

> On Mon, 2011-04-04 at 13:14 -0700, Stephen Hemminger wrote:
> > On Mon, 4 Apr 2011 12:53:14 -0700
> > Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
> > 
> > > On Mon, Apr 4, 2011 at 11:43, Stephen Hemminger <shemminger@vyatta.com> wrote:
> > > > Did a bunch of the easy drivers to convert.
> > > >
> > > >
> > > 
> > > I have added patches 3-9 (Intel driver conversions) to my queue of
> > > patches.  Thanks Stephen.
> > 
> > I changed the behaviour of igbvf for 'ethtool -p' to return not supported.
> > This seems correct, but it may break somebody who has some weird script running
> > in a virt environment.
> 
> I also noticed you did not convert ixgbevf, so we will take a look at
> your changes for igbvf and come up with a patch for ixgbevf.

ixgbevf has no hook for phys_id and therefore no change is needed.

RE: [PATCH net-next-2.6 07/12] igbvf: remove bogus phys_id

[Rose, Gregory V]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Stephen Hemminger
> Sent: Monday, April 04, 2011 11:44 AM
> To: David S. Miller; Ben Hutchings; Kirsher, Jeffrey T; Brandeburg, Jesse;
> Allan, Bruce W; Wyborny, Carolyn; Skidmore, Donald C; Ronciak, John
> Cc: netdev@vger.kernel.org
> Subject: [PATCH net-next-2.6 07/12] igbvf: remove bogus phys_id
> 
> This device lies about supporting phys_id. Remove it and just
> let the upper layer report not supported.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/drivers/net/igbvf/ethtool.c	2011-04-04 11:15:50.477019338 -0700
> +++ b/drivers/net/igbvf/ethtool.c	2011-04-04 11:16:12.661202774 -0700
> @@ -391,11 +391,6 @@ static int igbvf_set_wol(struct net_devi
>  	return -EOPNOTSUPP;
>  }
> 
> -static int igbvf_phys_id(struct net_device *netdev, u32 data)
> -{
> -	return 0;
> -}
> -
>  static int igbvf_get_coalesce(struct net_device *netdev,
>                                struct ethtool_coalesce *ec)
>  {
> @@ -527,7 +522,6 @@ static const struct ethtool_ops igbvf_et
>  	.self_test		= igbvf_diag_test,
>  	.get_sset_count		= igbvf_get_sset_count,
>  	.get_strings		= igbvf_get_strings,
> -	.phys_id		= igbvf_phys_id,
>  	.get_ethtool_stats	= igbvf_get_ethtool_stats,
>  	.get_coalesce		= igbvf_get_coalesce,
>  	.set_coalesce		= igbvf_set_coalesce,

I would just like to note that many virtual devices lie about their underlying capabilities.  Many systems report having an Realtek adapter when in fact they don't.  I'm agnostic about this change but at one time I had an internal bug report because my VF driver *didn't* report phys_id.  I'll keep this email around if my tester writes up a big for me.

Regards,

- Greg

Re: [PATCH net-next-2.6 02/12] skge: implement set_phys_id

[Ben Hutchings]

On Mon, 2011-04-04 at 11:43 -0700, Stephen Hemminger wrote:
> Implement set_phys_id led control on SysKonnect board.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> --- a/drivers/net/skge.c	2011-04-04 10:48:15.713539204 -0700
> +++ b/drivers/net/skge.c	2011-04-04 10:50:07.387094723 -0700
> @@ -786,29 +786,28 @@ static void skge_led(struct skge_port *s
>  }
>  
>  /* blink LED's for finding board */
> -static int skge_phys_id(struct net_device *dev, u32 data)
> +static int skge_set_phys_id(struct net_device *dev,
> +			    enum ethtool_phys_id_state state)
>  {
>  	struct skge_port *skge = netdev_priv(dev);
> -	unsigned long ms;
> -	enum led_mode mode = LED_MODE_TST;
>  
> -	if (!data || data > (u32)(MAX_SCHEDULE_TIMEOUT / HZ))
> -		ms = jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT / HZ) * 1000;
> -	else
> -		ms = data * 1000;
> -
> -	while (ms > 0) {
> -		skge_led(skge, mode);
> -		mode ^= LED_MODE_TST;
> -
> -		if (msleep_interruptible(BLINK_MS))
> -			break;
> -		ms -= BLINK_MS;
> +	switch (state) {
> +	case ETHTOOL_ID_ACTIVE:
> +		return -1;
[...]

Must be -EINVAL.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 00/12] Convert more drivers to ethtool set_phys_id

[Ben Hutchings]

On Mon, 2011-04-04 at 11:43 -0700, Stephen Hemminger wrote:
> Did a bunch of the easy drivers to convert.

Thanks Stephen.  I would have done some of these but I'm pretty snowed
under at the moment.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [PATCH net-next-2.6 12/12] qlcnic: convert to set_phys_id

[Amit Salecha]

> +             ret = adapter->nic_ops->config_led(adapter, 1, 0xf);
>               if (ret) {
> -                     clear_bit(__QLCNIC_RESETTING, &adapter->state);
> -                     return ret;
> +                     dev_err(&adapter->pdev->dev,
> +                             "Failed to set LED blink state.\n");
> +                     break;
>               }
> -     }
> -
> -     ret = adapter->nic_ops->config_led(adapter, 1, 0xf);
> -     if (ret) {
> -             dev_err(&adapter->pdev->dev,
> -                     "Failed to set LED blink state.\n");
> -             goto done;
> -     }
> +             return 0;
>
> -     msleep_interruptible(val * 1000);
> +     case ETHTOOL_ID_INACTIVE:
> +             ret = adapter->nic_ops->config_led(adapter, 0, 0xf);
> +             if (ret)
> +                     dev_err(&adapter->pdev->dev,
> +                             "Failed to reset LED blink state.\n");
> +             break;
>
> -     ret = adapter->nic_ops->config_led(adapter, 0, 0xf);
> -     if (ret) {
> -             dev_err(&adapter->pdev->dev,
> -                     "Failed to reset LED blink state.\n");
> -             goto done;
> +     default:
> +             return -EINVAL;
>       }
>
> -done:
> -     if (dev_down) {
> +     if (!netif_running(dev)) {
>               qlcnic_diag_free_res(dev, max_sds_rings);
>               clear_bit(__QLCNIC_RESETTING, &adapter->state);
>       }
> -     return ret;
>
> +     return ret;
>  }
>
"return ret" is return value of adapter->nic_ops->config_led, which can return any Error value.
And you have special check for -EINVAL. Though currently config_led doesn't return -EINVAL, but in future it may break (as 'ret' comes from config_led() -> send_cmd_desc()).
So it will be better if you return some hard code error value instead of 'ret'.

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

Re: [PATCH net-next-2.6 01/12] sky2: support ethtool set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:41 -0700

> Use ethtool set_phys_id to control LED. Fixes issues with RTNL being
> held for extended periods.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH net-next-2.6 02/12] skge: implement set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:42 -0700

> Implement set_phys_id led control on SysKonnect board.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, although I had to fix:

> +	case ETHTOOL_ID_ACTIVE:
> +		return -1;

to return -EINVAL.

Re: [PATCH net-next-2.6 03/12] e100: implemenet set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:43 -0700

> Implement set_phys_id to control LED. 
> Compile tested only.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 04/12] e1000: convert to set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:44 -0700

> Convert to new LED control infrastucture and remove no longer
> necessary bits.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 05/12] e1000e: implement ethtool set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:45 -0700

> The new ethtool set_phys_id takes over controlling the LED for identifying
> boards. This fixes the lockout during that period.
> For this device lots of extra infrastructure can also be removed by using
> set_phys_id.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 06/12] igb: convert to ethtool set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:46 -0700

> Convert igb driver to use new set_phys_id ethtool interface.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 07/12] igbvf: remove bogus phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:47 -0700

> This device lies about supporting phys_id. Remove it and just
> let the upper layer report not supported.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 08/12] ixgbe: convert to ethtool set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:48 -0700

> Convert ixgbe driver to use new set_phys_id ethtool interface.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 09/12] ixgb: convert to set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:49 -0700

> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Deferred to Intel driver maintainers.

Re: [PATCH net-next-2.6 10/12] tg3: implement ethtool set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:50 -0700

> Implement control of LED via set_phys_id. 
> Note: since PHY is powered off if device is down, this board
> only allows blinking if device is up.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH net-next-2.6 12/12] qlcnic: convert to set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:52 -0700

> Convert driver to use new ethtool set_phys_id.
> Not completely sure that this is correct for all cases of device
> up/down and doing operation. Compile tested only.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Please address the feedback given by qlcnic developers.

Thanks.

Re: [PATCH net-next-2.6 11/12] cxgb3: implement set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 04 Apr 2011 11:43:51 -0700

> Implement new ethtool set_phys_id on Chelsio cxgb3 board.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.

Re: [PATCH net-next-2.6 02/12] skge: implement set_phys_id

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 04 Apr 2011 23:57:32 +0100

> On Mon, 2011-04-04 at 11:43 -0700, Stephen Hemminger wrote:
>> -		ms -= BLINK_MS;
>> +	switch (state) {
>> +	case ETHTOOL_ID_ACTIVE:
>> +		return -1;
> [...]
> 
> Must be -EINVAL.

I took care of this when I applied Stephen's patch.

Re: [PATCH net-next-2.6 12/12] qlcnic: convert to set_phys_id

[Stephen Hemminger]

Convert driver to use new ethtool set_phys_id.
Not completely sure that this is correct for all cases of device
up/down and doing operation. Compile tested only.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
v2 -use adapter variable to hold state across active/inactive

--- a/drivers/net/qlcnic/qlcnic_ethtool.c	2011-04-06 12:30:35.673116770 -0700
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c	2011-04-06 12:52:06.250784682 -0700
@@ -831,48 +831,51 @@ static int qlcnic_set_tso(struct net_dev
 	return 0;
 }
 
-static int qlcnic_blink_led(struct net_device *dev, u32 val)
+static int qlcnic_set_led(struct net_device *dev,
+			  enum ethtool_phys_id_state state)
 {
 	struct qlcnic_adapter *adapter = netdev_priv(dev);
 	int max_sds_rings = adapter->max_sds_rings;
-	int dev_down = 0;
-	int ret;
 
-	if (!test_bit(__QLCNIC_DEV_UP, &adapter->state)) {
-		dev_down = 1;
-		if (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
-			return -EIO;
-
-		ret = qlcnic_diag_alloc_res(dev, QLCNIC_LED_TEST);
-		if (ret) {
-			clear_bit(__QLCNIC_RESETTING, &adapter->state);
-			return ret;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		adapter->blink_was_down = false;
+		if (!test_bit(__QLCNIC_DEV_UP, &adapter->state)) {
+			if (test_and_set_bit(__QLCNIC_RESETTING, &adapter->state))
+				return -EIO;
+
+			if (qlcnic_diag_alloc_res(dev, QLCNIC_LED_TEST)) {
+				clear_bit(__QLCNIC_RESETTING, &adapter->state);
+				return -EIO;
+			}
+			adapter->blink_was_down = true;
 		}
-	}
 
-	ret = adapter->nic_ops->config_led(adapter, 1, 0xf);
-	if (ret) {
+		if (adapter->nic_ops->config_led(adapter, 1, 0xf) == 0)
+			return 0;
+
 		dev_err(&adapter->pdev->dev,
 			"Failed to set LED blink state.\n");
-		goto done;
-	}
+		break;
 
-	msleep_interruptible(val * 1000);
+	case ETHTOOL_ID_INACTIVE:
+		if (adapter->nic_ops->config_led(adapter, 0, 0xf) == 0)
+			return 0;
 
-	ret = adapter->nic_ops->config_led(adapter, 0, 0xf);
-	if (ret) {
 		dev_err(&adapter->pdev->dev,
 			"Failed to reset LED blink state.\n");
-		goto done;
+		break;
+
+	default:
+		return -EINVAL;
 	}
 
-done:
-	if (dev_down) {
+	if (adapter->blink_was_down) {
 		qlcnic_diag_free_res(dev, max_sds_rings);
 		clear_bit(__QLCNIC_RESETTING, &adapter->state);
 	}
-	return ret;
 
+	return -EIO;
 }
 
 static void
@@ -1080,7 +1083,7 @@ const struct ethtool_ops qlcnic_ethtool_
 	.set_coalesce = qlcnic_set_intr_coalesce,
 	.get_flags = ethtool_op_get_flags,
 	.set_flags = qlcnic_set_flags,
-	.phys_id = qlcnic_blink_led,
+	.set_phys_id = qlcnic_set_led,
 	.set_msglevel = qlcnic_set_msglevel,
 	.get_msglevel = qlcnic_get_msglevel,
 };
--- a/drivers/net/qlcnic/qlcnic.h	2011-04-06 12:32:53.622576568 -0700
+++ b/drivers/net/qlcnic/qlcnic.h	2011-04-06 12:51:55.698672781 -0700
@@ -1056,6 +1056,7 @@ struct qlcnic_adapter {
 
 	struct qlcnic_filter_hash fhash;
 
+	bool 		blink_was_down;
 	unsigned long state;
 	__le32 file_prd_off;	/*File fw product offset*/
 	u32 fw_version;

Re: [PATCH net-next-2.6 12/12] qlcnic: convert to set_phys_id

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 6 Apr 2011 14:47:23 -0700

> Convert driver to use new ethtool set_phys_id.
> Not completely sure that this is correct for all cases of device
> up/down and doing operation. Compile tested only.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied.