Skip to contentSkip to conversation

Convert even more drivers to set_phys_id

PATCH Applied Stephen Hemminger 4 participants 25 messages Apr 4, 2011 – Apr 6, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Convert even more drivers to set_phys_id

This converts most drivers to the new set_phys_id ethtool ops. The three remaining drivers do things that are harder to convert since they all use a mailbox model. These should all compile, but not tested.

#1 lore raw

PATCH 1/8 vxge: convert to set_phys_id

Also fix up incorrect docbook comment

drivers/net/vxge/vxge-ethtool.c +16 -9
@@ -134,22 +134,29 @@ static void vxge_ethtool_gregs(struct ne
 /**
  * vxge_ethtool_idnic - To physically identify the nic on the system.
  * @dev : device pointer.
- * @id : pointer to the structure with identification parameters given by
- * ethtool.
+ * @state : requested LED state
  *
  * Used to physically identify the NIC on the system.
- * The Link LED will blink for a time specified by the user.
- * Return value:
  * 0 on success
  */
-static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
+static int vxge_ethtool_idnic(struct net_device *dev,
+			      enum ethtool_phys_id_state state)
 {
 	struct vxgedev *vdev = netdev_priv(dev);
 	struct __vxge_hw_device *hldev = vdev->devh;
 
-	vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_ON);
-	msleep_interruptible(data ? (data * HZ) : VXGE_MAX_FLICKER_TIME);
-	vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_OFF);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_ON);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_OFF);
+		break;
+
+	default:
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -1183,7 +1190,7 @@ static const struct ethtool_ops vxge_eth
 	.get_tso		= ethtool_op_get_tso,
 	.set_tso		= vxge_ethtool_op_set_tso,
 	.get_strings		= vxge_ethtool_get_strings,
-	.phys_id		= vxge_ethtool_idnic,
+	.set_phys_id		= vxge_ethtool_idnic,
 	.get_sset_count		= vxge_ethtool_get_sset_count,
 	.get_ethtool_stats	= vxge_get_ethtool_stats,
 	.set_flags		= vxge_set_flags,
#3 lore raw

PATCH 2/8 bnx2: convert to set_phys_id

In reply to Stephen Hemminger

In this case, need to add element to device private to hold original led state.

---
 drivers/net/bnx2.c |   58 +++++++++++++++++++++++++----------------------------
 drivers/net/bnx2.h |    1 
 2 files changed, 29 insertions(+), 30 deletions(-)
drivers/net/bnx2.c +28 -30
@@ -7495,41 +7495,39 @@ bnx2_get_ethtool_stats(struct net_device
 }
 
 static int
-bnx2_phys_id(struct net_device *dev, u32 data)
+bnx2_set_phys_id(struct net_device *dev, enum ethtool_phys_id_state state)
 {
 	struct bnx2 *bp = netdev_priv(dev);
-	int i;
-	u32 save;
 
-	bnx2_set_power_state(bp, PCI_D0);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		bnx2_set_power_state(bp, PCI_D0);
 
-	if (data == 0)
-		data = 2;
+		bp->leds_save = REG_RD(bp, BNX2_MISC_CFG);
+		REG_WR(bp, BNX2_MISC_CFG, BNX2_MISC_CFG_LEDMODE_MAC);
+		return -EINVAL;
 
-	save = REG_RD(bp, BNX2_MISC_CFG);
-	REG_WR(bp, BNX2_MISC_CFG, BNX2_MISC_CFG_LEDMODE_MAC);
-
-	for (i = 0; i < (data * 2); i++) {
-		if ((i % 2) == 0) {
-			REG_WR(bp, BNX2_EMAC_LED, BNX2_EMAC_LED_OVERRIDE);
-		}
-		else {
-			REG_WR(bp, BNX2_EMAC_LED, BNX2_EMAC_LED_OVERRIDE |
-				BNX2_EMAC_LED_1000MB_OVERRIDE |
-				BNX2_EMAC_LED_100MB_OVERRIDE |
-				BNX2_EMAC_LED_10MB_OVERRIDE |
-				BNX2_EMAC_LED_TRAFFIC_OVERRIDE |
-				BNX2_EMAC_LED_TRAFFIC);
-		}
-		msleep_interruptible(500);
-		if (signal_pending(current))
-			break;
+	case ETHTOOL_ID_ON:
+		REG_WR(bp, BNX2_EMAC_LED, BNX2_EMAC_LED_OVERRIDE |
+		       BNX2_EMAC_LED_1000MB_OVERRIDE |
+		       BNX2_EMAC_LED_100MB_OVERRIDE |
+		       BNX2_EMAC_LED_10MB_OVERRIDE |
+		       BNX2_EMAC_LED_TRAFFIC_OVERRIDE |
+		       BNX2_EMAC_LED_TRAFFIC);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		REG_WR(bp, BNX2_EMAC_LED, BNX2_EMAC_LED_OVERRIDE);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		REG_WR(bp, BNX2_EMAC_LED, 0);
+		REG_WR(bp, BNX2_MISC_CFG, bp->leds_save);
+
+		if (!netif_running(dev))
+			bnx2_set_power_state(bp, PCI_D3hot);
+		break;
 	}
-	REG_WR(bp, BNX2_EMAC_LED, 0);
-	REG_WR(bp, BNX2_MISC_CFG, save);
-
-	if (!netif_running(dev))
-		bnx2_set_power_state(bp, PCI_D3hot);
 
 	return 0;
 }
@@ -7602,7 +7600,7 @@ static const struct ethtool_ops bnx2_eth
 	.set_tso		= bnx2_set_tso,
 	.self_test		= bnx2_self_test,
 	.get_strings		= bnx2_get_strings,
-	.phys_id		= bnx2_phys_id,
+	.set_phys_id		= bnx2_set_phys_id,
 	.get_ethtool_stats	= bnx2_get_ethtool_stats,
 	.get_sset_count		= bnx2_get_sset_count,
 	.set_flags		= bnx2_set_flags,
drivers/net/bnx2.h +1 -0
@@ -6922,6 +6922,7 @@ struct bnx2 {
 	u8			num_tx_rings;
 	u8			num_rx_rings;
 
+	u32 			leds_save;
 	u32			idle_chk_status_idx;
 
 #ifdef BCM_CNIC
#5 lore raw

PATCH 3/8 bnx2x: convert to set_phys_id

In reply to Stephen Hemminger

Also cleanup error codes to no lie about things that driver doesn't support. If device is down report -EAGAIN (same as Broadcom), and if port doesn't blink then error as well.

drivers/net/bnx2x/bnx2x_ethtool.c +23 -21
@@ -2097,36 +2097,38 @@ static void bnx2x_get_ethtool_stats(stru
 	}
 }
 
-static int bnx2x_phys_id(struct net_device *dev, u32 data)
+static int bnx2x_set_phys_id(struct net_device *dev,
+			     enum ethtool_phys_id_state state)
 {
 	struct bnx2x *bp = netdev_priv(dev);
-	int i;
 
 	if (!netif_running(dev))
-		return 0;
+		return -EAGAIN;
 
 	if (!bp->port.pmf)
-		return 0;
+		return -EOPNOTSUPP;
 
-	if (data == 0)
-		data = 2;
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
 
-	for (i = 0; i < (data * 2); i++) {
-		if ((i % 2) == 0)
-			bnx2x_set_led(&bp->link_params, &bp->link_vars,
-				      LED_MODE_OPER, SPEED_1000);
-		else
-			bnx2x_set_led(&bp->link_params, &bp->link_vars,
-				      LED_MODE_OFF, 0);
+	case ETHTOOL_ID_ON:
+		bnx2x_set_led(&bp->link_params, &bp->link_vars,
+			      LED_MODE_OPER, SPEED_1000);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		bnx2x_set_led(&bp->link_params, &bp->link_vars,
+			      LED_MODE_OFF, 0);
 
-		msleep_interruptible(500);
-		if (signal_pending(current))
-			break;
-	}
+		break;
 
-	if (bp->link_vars.link_up)
-		bnx2x_set_led(&bp->link_params, &bp->link_vars, LED_MODE_OPER,
-			      bp->link_vars.line_speed);
+	case ETHTOOL_ID_INACTIVE:
+		if (bp->link_vars.link_up)
+			bnx2x_set_led(&bp->link_params, &bp->link_vars,
+				      LED_MODE_OPER,
+				      bp->link_vars.line_speed);
+	}
 
 	return 0;
 }
@@ -2218,7 +2220,7 @@ static const struct ethtool_ops bnx2x_et
 	.self_test		= bnx2x_self_test,
 	.get_sset_count		= bnx2x_get_sset_count,
 	.get_strings		= bnx2x_get_strings,
-	.phys_id		= bnx2x_phys_id,
+	.set_phys_id		= bnx2x_set_phys_id,
 	.get_ethtool_stats	= bnx2x_get_ethtool_stats,
 	.get_rxnfc		= bnx2x_get_rxnfc,
 	.get_rxfh_indir		= bnx2x_get_rxfh_indir,
maintainer Applied
#6 lore raw

Re: [PATCH 3/8] bnx2x: convert to set_phys_id

From: Stephen Hemminger <[email protected]>
Date: Mon, 04 Apr 2011 14:06:37 -0700

Quoted 5 lines

Also cleanup error codes to no lie about things that driver doesn't support. If device is down report -EAGAIN (same as Broadcom), and if port doesn't blink then error as well.

Applied.

#7 lore raw

PATCH 4/8 s2io: convert to set_phys_id

In reply to Stephen Hemminger

Convert to new ethtool set physical id model. Remove no longer used timer, and fix docbook comment.

drivers/net/s2io.c +40 -40
@@ -5484,53 +5484,50 @@ static void s2io_ethtool_gregs(struct ne
 	}
 }
 
-/**
- *  s2io_phy_id  - timer function that alternates adapter LED.
- *  @data : address of the private member of the device structure, which
- *  is a pointer to the s2io_nic structure, provided as an u32.
- * Description: This is actually the timer function that alternates the
- * adapter LED bit of the adapter control bit to set/reset every time on
- * invocation. The timer is set for 1/2 a second, hence tha NIC blinks
- *  once every second.
+/*
+ *  s2io_set_led - control NIC led
  */
-static void s2io_phy_id(unsigned long data)
+static void s2io_set_led(struct s2io_nic *sp, bool on)
 {
-	struct s2io_nic *sp = (struct s2io_nic *)data;
 	struct XENA_dev_config __iomem *bar0 = sp->bar0;
-	u64 val64 = 0;
-	u16 subid;
+	u16 subid = sp->pdev->subsystem_device;
+	u64 val64;
 
-	subid = sp->pdev->subsystem_device;
 	if ((sp->device_type == XFRAME_II_DEVICE) ||
 	    ((subid & 0xFF) >= 0x07)) {
 		val64 = readq(&bar0->gpio_control);
-		val64 ^= GPIO_CTRL_GPIO_0;
+		if (on)
+			val64 |= GPIO_CTRL_GPIO_0;
+		else
+			val64 &= ~GPIO_CTRL_GPIO_0;
+
 		writeq(val64, &bar0->gpio_control);
 	} else {
 		val64 = readq(&bar0->adapter_control);
-		val64 ^= ADAPTER_LED_ON;
+		if (on)
+			val64 |= ADAPTER_LED_ON;
+		else
+			val64 &= ~ADAPTER_LED_ON;
+
 		writeq(val64, &bar0->adapter_control);
 	}
 
-	mod_timer(&sp->id_timer, jiffies + HZ / 2);
 }
 
 /**
- * s2io_ethtool_idnic - To physically identify the nic on the system.
- * @sp : private member of the device structure, which is a pointer to the
- * s2io_nic structure.
- * @id : pointer to the structure with identification parameters given by
- * ethtool.
+ * s2io_ethtool_set_led - To physically identify the nic on the system.
+ * @dev : network device
+ * @state: led setting
+ *
  * Description: Used to physically identify the NIC on the system.
  * The Link LED will blink for a time specified by the user for
  * identification.
  * NOTE: The Link has to be Up to be able to blink the LED. Hence
  * identification is possible only if it's link is up.
- * Return value:
- * int , returns 0 on success
  */
 
-static int s2io_ethtool_idnic(struct net_device *dev, u32 data)
+static int s2io_ethtool_set_led(struct net_device *dev,
+				enum ethtool_phys_id_state state)
 {
 	u64 val64 = 0, last_gpio_ctrl_val;
 	struct s2io_nic *sp = netdev_priv(dev);
@@ -5543,24 +5540,27 @@ static int s2io_ethtool_idnic(struct net
 		val64 = readq(&bar0->adapter_control);
 		if (!(val64 & ADAPTER_CNTL_EN)) {
 			pr_err("Adapter Link down, cannot blink LED\n");
-			return -EFAULT;
+			return -EAGAIN;
 		}
 	}
-	if (sp->id_timer.function == NULL) {
-		init_timer(&sp->id_timer);
-		sp->id_timer.function = s2io_phy_id;
-		sp->id_timer.data = (unsigned long)sp;
-	}
-	mod_timer(&sp->id_timer, jiffies);
-	if (data)
-		msleep_interruptible(data * HZ);
-	else
-		msleep_interruptible(MAX_FLICKER_TIME);
-	del_timer_sync(&sp->id_timer);
 
-	if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid)) {
-		writeq(last_gpio_ctrl_val, &bar0->gpio_control);
-		last_gpio_ctrl_val = readq(&bar0->gpio_control);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		s2io_set_led(sp, true);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		s2io_set_led(sp, false);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid)) {
+			writeq(last_gpio_ctrl_val, &bar0->gpio_control);
+			last_gpio_ctrl_val = readq(&bar0->gpio_control);
+		}
 	}
 
 	return 0;
@@ -6776,7 +6776,7 @@ static const struct ethtool_ops netdev_e
 	.set_ufo = ethtool_op_set_ufo,
 	.self_test = s2io_ethtool_test,
 	.get_strings = s2io_ethtool_get_strings,
-	.phys_id = s2io_ethtool_idnic,
+	.set_phys_id = s2io_ethtool_set_led,
 	.get_ethtool_stats = s2io_get_ethtool_stats,
 	.get_sset_count = s2io_get_sset_count,
 };
drivers/net/s2io.h +0 -3
@@ -893,9 +893,6 @@ struct s2io_nic {
 	u16 all_multi_pos;
 	u16 promisc_flg;
 
-	/*  Id timer, used to blink NIC to physically identify NIC. */
-	struct timer_list id_timer;
-
 	/*  Restart timer, used to restart NIC if the device is stuck and
 	 *  a schedule task that will set the correct Link state once the
 	 *  NIC's PHY has stabilized after a state change.
#8 lore raw

Re: [PATCH 4/8] s2io: convert to set_phys_id

Quoted 113 lines · Stephen Hemminger

plain text document attachment (s2io-set-phys.patch) Convert to new ethtool set physical id model. Remove no longer used timer, and fix docbook comment.

drivers/net/s2io.c +28 -39
@@ -5484,53 +5484,50 @@ static void s2io_ethtool_gregs(struct ne
 	}
 }
 
-/**
- *  s2io_phy_id  - timer function that alternates adapter LED.
- *  @data : address of the private member of the device structure, which
- *  is a pointer to the s2io_nic structure, provided as an u32.
- * Description: This is actually the timer function that alternates the
- * adapter LED bit of the adapter control bit to set/reset every time on
- * invocation. The timer is set for 1/2 a second, hence tha NIC blinks
- *  once every second.
+/*
+ *  s2io_set_led - control NIC led
  */
-static void s2io_phy_id(unsigned long data)
+static void s2io_set_led(struct s2io_nic *sp, bool on)
 {
-	struct s2io_nic *sp = (struct s2io_nic *)data;
 	struct XENA_dev_config __iomem *bar0 = sp->bar0;
-	u64 val64 = 0;
-	u16 subid;
+	u16 subid = sp->pdev->subsystem_device;
+	u64 val64;
 
-	subid = sp->pdev->subsystem_device;
 	if ((sp->device_type == XFRAME_II_DEVICE) ||
 	    ((subid & 0xFF) >= 0x07)) {
 		val64 = readq(&bar0->gpio_control);
-		val64 ^= GPIO_CTRL_GPIO_0;
+		if (on)
+			val64 |= GPIO_CTRL_GPIO_0;
+		else
+			val64 &= ~GPIO_CTRL_GPIO_0;
+
 		writeq(val64, &bar0->gpio_control);
 	} else {
 		val64 = readq(&bar0->adapter_control);
-		val64 ^= ADAPTER_LED_ON;
+		if (on)
+			val64 |= ADAPTER_LED_ON;
+		else
+			val64 &= ~ADAPTER_LED_ON;
+
 		writeq(val64, &bar0->adapter_control);
 	}
 
-	mod_timer(&sp->id_timer, jiffies + HZ / 2);
 }
 
 /**
- * s2io_ethtool_idnic - To physically identify the nic on the system.
- * @sp : private member of the device structure, which is a pointer to the
- * s2io_nic structure.
- * @id : pointer to the structure with identification parameters given by
- * ethtool.
+ * s2io_ethtool_set_led - To physically identify the nic on the system.
+ * @dev : network device
+ * @state: led setting
+ *
  * Description: Used to physically identify the NIC on the system.
  * The Link LED will blink for a time specified by the user for
  * identification.
  * NOTE: The Link has to be Up to be able to blink the LED. Hence
  * identification is possible only if it's link is up.
- * Return value:
- * int , returns 0 on success
  */
 
-static int s2io_ethtool_idnic(struct net_device *dev, u32 data)
+static int s2io_ethtool_set_led(struct net_device *dev,
+				enum ethtool_phys_id_state state)
 {
 	u64 val64 = 0, last_gpio_ctrl_val;
 	struct s2io_nic *sp = netdev_priv(dev);
@@ -5543,24 +5540,27 @@ static int s2io_ethtool_idnic(struct net
 		val64 = readq(&bar0->adapter_control);
 		if (!(val64 & ADAPTER_CNTL_EN)) {
 			pr_err("Adapter Link down, cannot blink LED\n");
-			return -EFAULT;
+			return -EAGAIN;
 		}
 	}
-	if (sp->id_timer.function == NULL) {
-		init_timer(&sp->id_timer);
-		sp->id_timer.function = s2io_phy_id;
-		sp->id_timer.data = (unsigned long)sp;
-	}
-	mod_timer(&sp->id_timer, jiffies);
-	if (data)
-		msleep_interruptible(data * HZ);
-	else
-		msleep_interruptible(MAX_FLICKER_TIME);
-	del_timer_sync(&sp->id_timer);
 
-	if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid)) {
-		writeq(last_gpio_ctrl_val, &bar0->gpio_control);
-		last_gpio_ctrl_val = readq(&bar0->gpio_control);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		s2io_set_led(sp, true);

drivers/net/s2io.c static int s2io_ethtool_idnic(struct net · line 5562

Quoted 10 lines
drivers/net/s2io.c +10 -0
+		break;
+
+	case ETHTOOL_ID_OFF:
+		s2io_set_led(sp, false);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid)) {
+			writeq(last_gpio_ctrl_val, &bar0->gpio_control);
+			last_gpio_ctrl_val = readq(&bar0->gpio_control);

[...]

I think last_gpio_ctrl_val needs to be moved to struct s2io_nic and initialised only in the ETHTOOL_ID_ACTIVE case.

Ben.

Signature · 3 lines
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.
#9 lore raw

Re: [PATCH 4/8] s2io: convert to set_phys_id

		last_gpio_ctrl_val = readq(&bar0->gpio_control);
Quoted 4 lines

[...]

I think last_gpio_ctrl_val needs to be moved to struct s2io_nic and initialised only in the ETHTOOL_ID_ACTIVE case.

Strange there is a value already there and unused?
sp->adapt_ctrl_org

#10 lore raw

PATCH 4/8 s2io: convert to set_phys_id (v2)

In reply to Ben Hutchings

Convert to new ethtool set physical id model. Remove no longer used timer, and fix docbook comment.

---
v2 - store last gpio value in unused part of s2io_nic

drivers/net/s2io.c +41 -45
@@ -5484,83 +5484,79 @@ static void s2io_ethtool_gregs(struct ne
 	}
 }
 
-/**
- *  s2io_phy_id  - timer function that alternates adapter LED.
- *  @data : address of the private member of the device structure, which
- *  is a pointer to the s2io_nic structure, provided as an u32.
- * Description: This is actually the timer function that alternates the
- * adapter LED bit of the adapter control bit to set/reset every time on
- * invocation. The timer is set for 1/2 a second, hence tha NIC blinks
- *  once every second.
+/*
+ *  s2io_set_led - control NIC led
  */
-static void s2io_phy_id(unsigned long data)
+static void s2io_set_led(struct s2io_nic *sp, bool on)
 {
-	struct s2io_nic *sp = (struct s2io_nic *)data;
 	struct XENA_dev_config __iomem *bar0 = sp->bar0;
-	u64 val64 = 0;
-	u16 subid;
+	u16 subid = sp->pdev->subsystem_device;
+	u64 val64;
 
-	subid = sp->pdev->subsystem_device;
 	if ((sp->device_type == XFRAME_II_DEVICE) ||
 	    ((subid & 0xFF) >= 0x07)) {
 		val64 = readq(&bar0->gpio_control);
-		val64 ^= GPIO_CTRL_GPIO_0;
+		if (on)
+			val64 |= GPIO_CTRL_GPIO_0;
+		else
+			val64 &= ~GPIO_CTRL_GPIO_0;
+
 		writeq(val64, &bar0->gpio_control);
 	} else {
 		val64 = readq(&bar0->adapter_control);
-		val64 ^= ADAPTER_LED_ON;
+		if (on)
+			val64 |= ADAPTER_LED_ON;
+		else
+			val64 &= ~ADAPTER_LED_ON;
+
 		writeq(val64, &bar0->adapter_control);
 	}
 
-	mod_timer(&sp->id_timer, jiffies + HZ / 2);
 }
 
 /**
- * s2io_ethtool_idnic - To physically identify the nic on the system.
- * @sp : private member of the device structure, which is a pointer to the
- * s2io_nic structure.
- * @id : pointer to the structure with identification parameters given by
- * ethtool.
+ * s2io_ethtool_set_led - To physically identify the nic on the system.
+ * @dev : network device
+ * @state: led setting
+ *
  * Description: Used to physically identify the NIC on the system.
  * The Link LED will blink for a time specified by the user for
  * identification.
  * NOTE: The Link has to be Up to be able to blink the LED. Hence
  * identification is possible only if it's link is up.
- * Return value:
- * int , returns 0 on success
  */
 
-static int s2io_ethtool_idnic(struct net_device *dev, u32 data)
+static int s2io_ethtool_set_led(struct net_device *dev,
+				enum ethtool_phys_id_state state)
 {
-	u64 val64 = 0, last_gpio_ctrl_val;
 	struct s2io_nic *sp = netdev_priv(dev);
 	struct XENA_dev_config __iomem *bar0 = sp->bar0;
-	u16 subid;
+	u16 subid = sp->pdev->subsystem_device;
 
-	subid = sp->pdev->subsystem_device;
-	last_gpio_ctrl_val = readq(&bar0->gpio_control);
 	if ((sp->device_type == XFRAME_I_DEVICE) && ((subid & 0xFF) < 0x07)) {
-		val64 = readq(&bar0->adapter_control);
+		u64 val64 = readq(&bar0->adapter_control);
 		if (!(val64 & ADAPTER_CNTL_EN)) {
 			pr_err("Adapter Link down, cannot blink LED\n");
-			return -EFAULT;
+			return -EAGAIN;
 		}
 	}
-	if (sp->id_timer.function == NULL) {
-		init_timer(&sp->id_timer);
-		sp->id_timer.function = s2io_phy_id;
-		sp->id_timer.data = (unsigned long)sp;
-	}
-	mod_timer(&sp->id_timer, jiffies);
-	if (data)
-		msleep_interruptible(data * HZ);
-	else
-		msleep_interruptible(MAX_FLICKER_TIME);
-	del_timer_sync(&sp->id_timer);
 
-	if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid)) {
-		writeq(last_gpio_ctrl_val, &bar0->gpio_control);
-		last_gpio_ctrl_val = readq(&bar0->gpio_control);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		sp->adapt_ctrl_org = readq(&bar0->gpio_control);
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		s2io_set_led(sp, true);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		s2io_set_led(sp, false);
+		break;
+
+	case ETHTOOL_ID_INACTIVE:
+		if (CARDS_WITH_FAULTY_LINK_INDICATORS(sp->device_type, subid))
+			writeq(sp->adapt_ctrl_org, &bar0->gpio_control);
 	}
 
 	return 0;
@@ -6776,7 +6772,7 @@ static const struct ethtool_ops netdev_e
 	.set_ufo = ethtool_op_set_ufo,
 	.self_test = s2io_ethtool_test,
 	.get_strings = s2io_ethtool_get_strings,
-	.phys_id = s2io_ethtool_idnic,
+	.set_phys_id = s2io_ethtool_set_led,
 	.get_ethtool_stats = s2io_get_ethtool_stats,
 	.get_sset_count = s2io_get_sset_count,
 };
drivers/net/s2io.h +0 -3
@@ -893,9 +893,6 @@ struct s2io_nic {
 	u16 all_multi_pos;
 	u16 promisc_flg;
 
-	/*  Id timer, used to blink NIC to physically identify NIC. */
-	struct timer_list id_timer;
-
 	/*  Restart timer, used to restart NIC if the device is stuck and
 	 *  a schedule task that will set the correct Link state once the
 	 *  NIC's PHY has stabilized after a state change.
#12 lore raw

PATCH 5/8 --- drivers/net/niu.c | 33 ++++++++++++++++++--------------- drivers/net/niu.h | 1 + 2 files changed, 19 insertions(+), 15 deletions(-)

In reply to Stephen Hemminger
drivers/net/niu.c +18 -15
@@ -7888,28 +7888,31 @@ static void niu_force_led(struct niu *np
 	nw64_mac(reg, val);
 }
 
-static int niu_phys_id(struct net_device *dev, u32 data)
+static int niu_set_phys_id(struct net_device *dev,
+			   enum ethtool_phys_id_state state)
+
 {
 	struct niu *np = netdev_priv(dev);
-	u64 orig_led_state;
-	int i;
 
 	if (!netif_running(dev))
 		return -EAGAIN;
 
-	if (data == 0)
-		data = 2;
-
-	orig_led_state = niu_led_state_save(np);
-	for (i = 0; i < (data * 2); i++) {
-		int on = ((i % 2) == 0);
-
-		niu_force_led(np, on);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		np->orig_led_state = niu_led_state_save(np);
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		niu_force_led(np, 1);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		niu_force_led(np, 0);
+		break;
 
-		if (msleep_interruptible(500))
-			break;
+	case ETHTOOL_ID_INACTIVE:
+		niu_led_state_restore(np, np->orig_led_state);
 	}
-	niu_led_state_restore(np, orig_led_state);
 
 	return 0;
 }
@@ -7932,7 +7935,7 @@ static const struct ethtool_ops niu_etht
 	.get_strings		= niu_get_strings,
 	.get_sset_count		= niu_get_sset_count,
 	.get_ethtool_stats	= niu_get_ethtool_stats,
-	.phys_id		= niu_phys_id,
+	.set_phys_id		= niu_set_phys_id,
 	.get_rxnfc		= niu_get_nfc,
 	.set_rxnfc		= niu_set_nfc,
 	.set_flags		= niu_set_flags,
drivers/net/niu.h +1 -0
@@ -3279,6 +3279,7 @@ struct niu {
 	unsigned long			xpcs_off;
 
 	struct timer_list		timer;
+	u64				orig_led_state;
 	const struct niu_phy_ops	*phy_ops;
 	int				phy_addr;
 
#13 lore raw

PATCH 5/8 niu: convert to new ethtool set_phys_id

---
Sorry for mangled version..

 drivers/net/niu.c |   33 ++++++++++++++++++---------------
 drivers/net/niu.h |    1 +
 2 files changed, 19 insertions(+), 15 deletions(-)
drivers/net/niu.c +18 -15
@@ -7888,28 +7888,31 @@ static void niu_force_led(struct niu *np
 	nw64_mac(reg, val);
 }
 
-static int niu_phys_id(struct net_device *dev, u32 data)
+static int niu_set_phys_id(struct net_device *dev,
+			   enum ethtool_phys_id_state state)
+
 {
 	struct niu *np = netdev_priv(dev);
-	u64 orig_led_state;
-	int i;
 
 	if (!netif_running(dev))
 		return -EAGAIN;
 
-	if (data == 0)
-		data = 2;
-
-	orig_led_state = niu_led_state_save(np);
-	for (i = 0; i < (data * 2); i++) {
-		int on = ((i % 2) == 0);
-
-		niu_force_led(np, on);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		np->orig_led_state = niu_led_state_save(np);
+		return -EINVAL;
+
+	case ETHTOOL_ID_ON:
+		niu_force_led(np, 1);
+		break;
+
+	case ETHTOOL_ID_OFF:
+		niu_force_led(np, 0);
+		break;
 
-		if (msleep_interruptible(500))
-			break;
+	case ETHTOOL_ID_INACTIVE:
+		niu_led_state_restore(np, np->orig_led_state);
 	}
-	niu_led_state_restore(np, orig_led_state);
 
 	return 0;
 }
@@ -7932,7 +7935,7 @@ static const struct ethtool_ops niu_etht
 	.get_strings		= niu_get_strings,
 	.get_sset_count		= niu_get_sset_count,
 	.get_ethtool_stats	= niu_get_ethtool_stats,
-	.phys_id		= niu_phys_id,
+	.set_phys_id		= niu_set_phys_id,
 	.get_rxnfc		= niu_get_nfc,
 	.set_rxnfc		= niu_set_nfc,
 	.set_flags		= niu_set_flags,
drivers/net/niu.h +1 -0
@@ -3279,6 +3279,7 @@ struct niu {
 	unsigned long			xpcs_off;
 
 	struct timer_list		timer;
+	u64				orig_led_state;
 	const struct niu_phy_ops	*phy_ops;
 	int				phy_addr;
 
#15 lore raw

PATCH 6/8 benet: convert to set_phys_id

In reply to Stephen Hemminger
---
 drivers/net/benet/be.h         |    1 
 drivers/net/benet/be_ethtool.c |   42 ++++++++++++++++++++++-------------------
 2 files changed, 24 insertions(+), 19 deletions(-)
drivers/net/benet/be.h +1 -0
@@ -313,6 +313,7 @@ struct be_adapter {
 	char fw_ver[FW_VER_LEN];
 	u32 if_handle;		/* Used to configure filtering */
 	u32 pmac_id;		/* MAC addr handle used by BE card */
+	u32 beacon_state;	/* for set_phys_id */
 
 	bool eeh_err;
 	bool link_up;
drivers/net/benet/be_ethtool.c +23 -19

59 lines not on this page · raw message

#16 lore raw

RE: [PATCH 6/8] benet: convert to set_phys_id

_______________________________________
From: Stephen Hemminger [[email protected]]
Sent: Monday, April 04, 2011 4:06 PM
To: David S. Miller; Ben Hutchings; Perla, Sathya; Seetharaman, Subramanian; Khaparde, Ajit
Cc: [email protected]
Subject: [PATCH 6/8] benet: convert to set_phys_id

Thanks Stephen. You finished the work, while I was in the middle of the same.

I have picked this patch for testing. I guess the ethtool patches will be re-spun. So I will probably wait for that before I send an update.

-Ajit

#18 lore raw

PATCH 7/8 pcnet32: convert to set_phys_id

In reply to Stephen Hemminger
drivers/net/pcnet32.c +28 -46
@@ -295,12 +295,14 @@ struct pcnet32_private {
 	struct net_device	*next;
 	struct mii_if_info	mii_if;
 	struct timer_list	watchdog_timer;
-	struct timer_list	blink_timer;
 	u32			msg_enable;	/* debug message level */
 
 	/* each bit indicates an available PHY */
 	u32			phymask;
 	unsigned short		chip_version;	/* which variant this is */
+
+	/* saved registers during ethtool blink */
+	u16 			save_regs[4];
 };
 
 static int pcnet32_probe_pci(struct pci_dev *, const struct pci_device_id *);
@@ -324,8 +326,6 @@ static void pcnet32_restart(struct net_d
 static void pcnet32_ethtool_test(struct net_device *dev,
 				 struct ethtool_test *eth_test, u64 * data);
 static int pcnet32_loopback_test(struct net_device *dev, uint64_t * data1);
-static int pcnet32_phys_id(struct net_device *dev, u32 data);
-static void pcnet32_led_blink_callback(struct net_device *dev);
 static int pcnet32_get_regs_len(struct net_device *dev);
 static void pcnet32_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 			     void *ptr);
@@ -1022,7 +1022,8 @@ clean_up:
 	return rc;
 }				/* end pcnet32_loopback_test  */
 
-static void pcnet32_led_blink_callback(struct net_device *dev)
+static int pcnet32_set_phys_id(struct net_device *dev,
+			       enum ethtool_phys_id_state state)
 {
 	struct pcnet32_private *lp = netdev_priv(dev);
 	struct pcnet32_access *a = &lp->a;
@@ -1030,50 +1031,31 @@ static void pcnet32_led_blink_callback(s
 	unsigned long flags;
 	int i;
 
-	spin_lock_irqsave(&lp->lock, flags);
-	for (i = 4; i < 8; i++)
-		a->write_bcr(ioaddr, i, a->read_bcr(ioaddr, i) ^ 0x4000);
-	spin_unlock_irqrestore(&lp->lock, flags);
-
-	mod_timer(&lp->blink_timer, PCNET32_BLINK_TIMEOUT);
-}
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* Save the current value of the bcrs */
+		spin_lock_irqsave(&lp->lock, flags);
+		for (i = 4; i < 8; i++)
+			lp->save_regs[i - 4] = a->read_bcr(ioaddr, i);
+		spin_unlock_irqrestore(&lp->lock, flags);
+		return -EINVAL;
 
-static int pcnet32_phys_id(struct net_device *dev, u32 data)
-{
-	struct pcnet32_private *lp = netdev_priv(dev);
-	struct pcnet32_access *a = &lp->a;
-	ulong ioaddr = dev->base_addr;
-	unsigned long flags;
-	int i, regs[4];
+	case ETHTOOL_ID_ON:
+	case ETHTOOL_ID_OFF:
+		/* Blink the led */
+		spin_lock_irqsave(&lp->lock, flags);
+		for (i = 4; i < 8; i++)
+			a->write_bcr(ioaddr, i, a->read_bcr(ioaddr, i) ^ 0x4000);
+		spin_unlock_irqrestore(&lp->lock, flags);
+		break;
 
-	if (!lp->blink_timer.function) {
-		init_timer(&lp->blink_timer);
-		lp->blink_timer.function = (void *)pcnet32_led_blink_callback;
-		lp->blink_timer.data = (unsigned long)dev;
+	case ETHTOOL_ID_INACTIVE:
+		/* Restore the original value of the bcrs */
+		spin_lock_irqsave(&lp->lock, flags);
+		for (i = 4; i < 8; i++)
+			a->write_bcr(ioaddr, i, lp->save_regs[i - 4]);
+		spin_unlock_irqrestore(&lp->lock, flags);
 	}
-
-	/* Save the current value of the bcrs */
-	spin_lock_irqsave(&lp->lock, flags);
-	for (i = 4; i < 8; i++)
-		regs[i - 4] = a->read_bcr(ioaddr, i);
-	spin_unlock_irqrestore(&lp->lock, flags);
-
-	mod_timer(&lp->blink_timer, jiffies);
-	set_current_state(TASK_INTERRUPTIBLE);
-
-	/* AV: the limit here makes no sense whatsoever */
-	if ((!data) || (data > (u32) (MAX_SCHEDULE_TIMEOUT / HZ)))
-		data = (u32) (MAX_SCHEDULE_TIMEOUT / HZ);
-
-	msleep_interruptible(data * 1000);
-	del_timer_sync(&lp->blink_timer);
-
-	/* Restore the original value of the bcrs */
-	spin_lock_irqsave(&lp->lock, flags);
-	for (i = 4; i < 8; i++)
-		a->write_bcr(ioaddr, i, regs[i - 4]);
-	spin_unlock_irqrestore(&lp->lock, flags);
-
 	return 0;
 }
 
@@ -1450,7 +1432,7 @@ static const struct ethtool_ops pcnet32_
 	.set_ringparam		= pcnet32_set_ringparam,
 	.get_strings		= pcnet32_get_strings,
 	.self_test		= pcnet32_ethtool_test,
-	.phys_id		= pcnet32_phys_id,
+	.set_phys_id		= pcnet32_set_phys_id,
 	.get_regs_len		= pcnet32_get_regs_len,
 	.get_regs		= pcnet32_get_regs,
 	.get_sset_count		= pcnet32_get_sset_count,
#20 lore raw

PATCH 8/8 ewrk3: convert to set_phys_id

In reply to Stephen Hemminger

Keep orginal locking and error handling.

drivers/net/ewrk3.c +24 -28
@@ -1604,55 +1604,51 @@ static u32 ewrk3_get_link(struct net_dev
 	return !(cmr & CMR_LINK);
 }
 
-static int ewrk3_phys_id(struct net_device *dev, u32 data)
+static int ewrk3_set_phys_id(struct net_device *dev,
+			     enum ethtool_phys_id_state state)
 {
 	struct ewrk3_private *lp = netdev_priv(dev);
 	unsigned long iobase = dev->base_addr;
 	unsigned long flags;
 	u8 cr;
-	int count;
-
-	/* Toggle LED 4x per second */
-	count = data << 2;
 
 	spin_lock_irqsave(&lp->hw_lock, flags);
 
-	/* Bail if a PHYS_ID is already in progress */
-	if (lp->led_mask == 0) {
-		spin_unlock_irqrestore(&lp->hw_lock, flags);
-		return -EBUSY;
-	}
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* Bail if a PHYS_ID is already in progress */
+		if (lp->led_mask == 0) {
+			spin_unlock_irqrestore(&lp->hw_lock, flags);
+			return -EBUSY;
+		}
 
-	/* Prevent ISR from twiddling the LED */
-	lp->led_mask = 0;
+		/* Prevent ISR from twiddling the LED */
+		lp->led_mask = 0;
+		spin_unlock_irqrestore(&lp->hw_lock, flags);
+		return -EINVAL;
 
-	while (count--) {
+	case ETHTOOL_ID_ON:
+	case ETHTOOL_ID_OFF:
 		/* Toggle the LED */
 		cr = inb(EWRK3_CR);
 		outb(cr ^ CR_LED, EWRK3_CR);
+		break;
 
-		/* Wait a little while */
-		spin_unlock_irqrestore(&lp->hw_lock, flags);
-		msleep(250);
-		spin_lock_irqsave(&lp->hw_lock, flags);
-
-		/* Exit if we got a signal */
-		if (signal_pending(current))
-			break;
+	case ETHTOOL_ID_INACTIVE:
+		lp->led_mask = CR_LED;
+		cr = inb(EWRK3_CR);
+		outb(cr & ~CR_LED, EWRK3_CR);
 	}
-
-	lp->led_mask = CR_LED;
-	cr = inb(EWRK3_CR);
-	outb(cr & ~CR_LED, EWRK3_CR);
 	spin_unlock_irqrestore(&lp->hw_lock, flags);
-	return signal_pending(current) ? -ERESTARTSYS : 0;
+
+	return 0;
 }
 
 static const struct ethtool_ops ethtool_ops_203 = {
 	.get_drvinfo = ewrk3_get_drvinfo,
 	.get_settings = ewrk3_get_settings,
 	.set_settings = ewrk3_set_settings,
-	.phys_id = ewrk3_phys_id,
+	.set_phys_id = ewrk3_set_phys_id,
 };
 
 static const struct ethtool_ops ethtool_ops = {
@@ -1660,7 +1656,7 @@ static const struct ethtool_ops ethtool_
 	.get_settings = ewrk3_get_settings,
 	.set_settings = ewrk3_set_settings,
 	.get_link = ewrk3_get_link,
-	.phys_id = ewrk3_phys_id,
+	.set_phys_id = ewrk3_set_phys_id,
 };
 
 /*
#21 lore raw

Re: [PATCH 8/8] ewrk3: convert to set_phys_id

Quoted 28 lines · Stephen Hemminger

Keep orginal locking and error handling.

drivers/net/ewrk3.c +2 -7
@@ -1604,55 +1604,51 @@ static u32 ewrk3_get_link(struct net_dev
 	return !(cmr & CMR_LINK);
 }
 
-static int ewrk3_phys_id(struct net_device *dev, u32 data)
+static int ewrk3_set_phys_id(struct net_device *dev,
+			     enum ethtool_phys_id_state state)
 {
 	struct ewrk3_private *lp = netdev_priv(dev);
 	unsigned long iobase = dev->base_addr;
 	unsigned long flags;
 	u8 cr;
-	int count;
-
-	/* Toggle LED 4x per second */
-	count = data << 2;
 
 	spin_lock_irqsave(&lp->hw_lock, flags);
 
-	/* Bail if a PHYS_ID is already in progress */
-	if (lp->led_mask == 0) {

drivers/net/ewrk3.c static u32 ewrk3_get_link(struct net_dev · line 1623

Quoted 10 lines
drivers/net/ewrk3.c +7 -3
-		spin_unlock_irqrestore(&lp->hw_lock, flags);
-		return -EBUSY;
-	}
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* Bail if a PHYS_ID is already in progress */
+		if (lp->led_mask == 0) {
+			spin_unlock_irqrestore(&lp->hw_lock, flags);
+			return -EBUSY;
+		}

[...]

This can never happen. Well, actually it can with the first version of my patch, but I'll fix that. :-)

Ben.

Signature · 3 lines
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.
#23 lore raw

Re: [PATCH 8/8] ewrk3: convert to set_phys_id

Use ethtool infrastructure for blinking, which is now does locking at higher level.

drivers/net/ewrk3.c +24 -32
@@ -1604,55 +1604,47 @@ static u32 ewrk3_get_link(struct net_dev
 	return !(cmr & CMR_LINK);
 }
 
-static int ewrk3_phys_id(struct net_device *dev, u32 data)
+static int ewrk3_set_phys_id(struct net_device *dev,
+			     enum ethtool_phys_id_state state)
 {
 	struct ewrk3_private *lp = netdev_priv(dev);
 	unsigned long iobase = dev->base_addr;
-	unsigned long flags;
 	u8 cr;
-	int count;
 
-	/* Toggle LED 4x per second */
-	count = data << 2;
+	spin_lock_irq(&lp->hw_lock);
 
-	spin_lock_irqsave(&lp->hw_lock, flags);
+	switch (state) {
+	case ETHTOOL_ID_ACTIVE:
+		/* Prevent ISR from twiddling the LED */
+		lp->led_mask = 0;
+		spin_unlock_irq(&lp->hw_lock);
+		return -EINVAL;
 
-	/* Bail if a PHYS_ID is already in progress */
-	if (lp->led_mask == 0) {
-		spin_unlock_irqrestore(&lp->hw_lock, flags);
-		return -EBUSY;
-	}
-
-	/* Prevent ISR from twiddling the LED */
-	lp->led_mask = 0;
+	case ETHTOOL_ID_ON:
+		cr = inb(EWRK3_CR);
+		outb(cr | CR_LED, EWRK3_CR);
+		break;
 
-	while (count--) {
-		/* Toggle the LED */
+	case ETHTOOL_ID_OFF:
 		cr = inb(EWRK3_CR);
-		outb(cr ^ CR_LED, EWRK3_CR);
+		outb(cr & ~CR_LED, EWRK3_CR);
+		break;
 
-		/* Wait a little while */
-		spin_unlock_irqrestore(&lp->hw_lock, flags);
-		msleep(250);
-		spin_lock_irqsave(&lp->hw_lock, flags);
-
-		/* Exit if we got a signal */
-		if (signal_pending(current))
-			break;
+	case ETHTOOL_ID_INACTIVE:
+		lp->led_mask = CR_LED;
+		cr = inb(EWRK3_CR);
+		outb(cr & ~CR_LED, EWRK3_CR);
 	}
+	spin_unlock_irq(&lp->hw_lock);
 
-	lp->led_mask = CR_LED;
-	cr = inb(EWRK3_CR);
-	outb(cr & ~CR_LED, EWRK3_CR);
-	spin_unlock_irqrestore(&lp->hw_lock, flags);
-	return signal_pending(current) ? -ERESTARTSYS : 0;
+	return 0;
 }
 
 static const struct ethtool_ops ethtool_ops_203 = {
 	.get_drvinfo = ewrk3_get_drvinfo,
 	.get_settings = ewrk3_get_settings,
 	.set_settings = ewrk3_set_settings,
-	.phys_id = ewrk3_phys_id,
+	.set_phys_id = ewrk3_set_phys_id,
 };
 
 static const struct ethtool_ops ethtool_ops = {
@@ -1660,7 +1652,7 @@ static const struct ethtool_ops ethtool_
 	.get_settings = ewrk3_get_settings,
 	.set_settings = ewrk3_set_settings,
 	.get_link = ewrk3_get_link,
-	.phys_id = ewrk3_phys_id,
+	.set_phys_id = ewrk3_set_phys_id,
 };
 
 /*