[PATCH 0/5] smsc95xx enhancements (v2)

[PATCH 0/5] smsc95xx enhancements (v2)

[Steve Glendinning]

This patchset makes a few minor enhancements to the smsc95xx driver
in preparation for some forthcoming power-saving improvements I'm
working on.

Please consider all for net-next.

Changes since v1:
 remove duplicate .suspend and .resume entries

Steve Glendinning (5):
  smsc95xx: sleep before read for lengthy operations
  smsc95xx: remove unnecessary variables
  smsc95xx: check return code from control messages
  smsc95xx: fix resume when usb device is reset
  smsc95xx: enable power saving mode during system suspend

 drivers/net/usb/smsc95xx.c |  388 +++++++++++++++++++++-----------------------
 drivers/net/usb/smsc95xx.h |    7 +-
 2 files changed, 190 insertions(+), 205 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] smsc95xx: sleep before read for lengthy operations

[Steve Glendinning]

During init, the device reset is unexpected to complete immediately,
so sleep before testing the condition rather than after it.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index d45e539..ed1e551 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -763,12 +763,12 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	timeout = 0;
 	do {
+		msleep(10);
 		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
 		if (ret < 0) {
 			netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
 			return ret;
 		}
-		msleep(10);
 		timeout++;
 	} while ((read_buf & HW_CFG_LRST_) && (timeout < 100));
 
@@ -786,12 +786,12 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	timeout = 0;
 	do {
+		msleep(10);
 		ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
 		if (ret < 0) {
 			netdev_warn(dev->net, "Failed to read PM_CTRL: %d\n", ret);
 			return ret;
 		}
-		msleep(10);
 		timeout++;
 	} while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
 
-- 
1.7.9.5

[PATCH 2/5] smsc95xx: remove unnecessary variables

[Steve Glendinning]

Removes unnecessary variables as smsc95xx_write_reg takes its
value by parameter.  Early versions passed this parameter by
reference.

Also replace hardcoded interrupt status value with a #define

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |   29 +++++++++--------------------
 drivers/net/usb/smsc95xx.h |    1 +
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index ed1e551..5d0256b 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -460,12 +460,10 @@ static int smsc95xx_link_reset(struct usbnet *dev)
 	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
 	unsigned long flags;
 	u16 lcladv, rmtadv;
-	u32 intdata;
 
 	/* clear interrupt status */
 	smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
-	intdata = 0xFFFFFFFF;
-	smsc95xx_write_reg(dev, INT_STS, intdata);
+	smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
 
 	mii_check_media(mii, 1, 1);
 	mii_ethtool_gset(&dev->mii, &ecmd);
@@ -677,7 +675,6 @@ static void smsc95xx_start_tx_path(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	unsigned long flags;
-	u32 reg_val;
 
 	/* Enable Tx at MAC */
 	spin_lock_irqsave(&pdata->mac_cr_lock, flags);
@@ -687,8 +684,7 @@ static void smsc95xx_start_tx_path(struct usbnet *dev)
 	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
 
 	/* Enable Tx at SCSRs */
-	reg_val = TX_CFG_ON_;
-	smsc95xx_write_reg(dev, TX_CFG, reg_val);
+	smsc95xx_write_reg(dev, TX_CFG, TX_CFG_ON_);
 }
 
 /* Starts the Receive path */
@@ -753,8 +749,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	netif_dbg(dev, ifup, dev->net, "entering smsc95xx_reset\n");
 
-	write_buf = HW_CFG_LRST_;
-	ret = smsc95xx_write_reg(dev, HW_CFG, write_buf);
+	ret = smsc95xx_write_reg(dev, HW_CFG, HW_CFG_LRST_);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write HW_CFG_LRST_ bit in HW_CFG register, ret = %d\n",
 			    ret);
@@ -777,8 +772,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 		return ret;
 	}
 
-	write_buf = PM_CTL_PHY_RST_;
-	ret = smsc95xx_write_reg(dev, PM_CTRL, write_buf);
+	ret = smsc95xx_write_reg(dev, PM_CTRL, PM_CTL_PHY_RST_);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write PM_CTRL: %d\n", ret);
 		return ret;
@@ -863,8 +857,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 		  "Read Value from BURST_CAP after writing: 0x%08x\n",
 		  read_buf);
 
-	read_buf = DEFAULT_BULK_IN_DELAY;
-	ret = smsc95xx_write_reg(dev, BULK_IN_DLY, read_buf);
+	ret = smsc95xx_write_reg(dev, BULK_IN_DLY, DEFAULT_BULK_IN_DELAY);
 	if (ret < 0) {
 		netdev_warn(dev->net, "ret = %d\n", ret);
 		return ret;
@@ -910,8 +903,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from HW_CFG after writing: 0x%08x\n", read_buf);
 
-	write_buf = 0xFFFFFFFF;
-	ret = smsc95xx_write_reg(dev, INT_STS, write_buf);
+	ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write INT_STS register, ret=%d\n",
 			    ret);
@@ -936,15 +928,13 @@ static int smsc95xx_reset(struct usbnet *dev)
 	}
 
 	/* Init Tx */
-	write_buf = 0;
-	ret = smsc95xx_write_reg(dev, FLOW, write_buf);
+	ret = smsc95xx_write_reg(dev, FLOW, 0);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write FLOW: %d\n", ret);
 		return ret;
 	}
 
-	read_buf = AFC_CFG_DEFAULT;
-	ret = smsc95xx_write_reg(dev, AFC_CFG, read_buf);
+	ret = smsc95xx_write_reg(dev, AFC_CFG, AFC_CFG_DEFAULT);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write AFC_CFG: %d\n", ret);
 		return ret;
@@ -959,8 +949,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	/* Init Rx */
 	/* Set Vlan */
-	write_buf = (u32)ETH_P_8021Q;
-	ret = smsc95xx_write_reg(dev, VLAN1, write_buf);
+	ret = smsc95xx_write_reg(dev, VLAN1, (u32)ETH_P_8021Q);
 	if (ret < 0) {
 		netdev_warn(dev->net, "Failed to write VAN1: %d\n", ret);
 		return ret;
diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
index 86bc449..a275b62 100644
--- a/drivers/net/usb/smsc95xx.h
+++ b/drivers/net/usb/smsc95xx.h
@@ -63,6 +63,7 @@
 #define INT_STS_TDFO_			(0x00001000)
 #define INT_STS_RXDF_			(0x00000800)
 #define INT_STS_GPIOS_			(0x000007FF)
+#define INT_STS_CLEAR_ALL_		(0xFFFFFFFF)
 
 #define RX_CFG				(0x0C)
 #define RX_FIFO_FLUSH_			(0x00000001)
-- 
1.7.9.5

[PATCH 3/5] smsc95xx: check return code from control messages

[Steve Glendinning]

This patch adds additional checks of the values returned by
smsc95xx_(read|write)_reg, and wraps their common patterns
in macros.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |  331 ++++++++++++++++++++------------------------
 1 file changed, 148 insertions(+), 183 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 5d0256b..d0ff01e 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -47,6 +47,15 @@
 #define SMSC95XX_TX_OVERHEAD		(8)
 #define SMSC95XX_TX_OVERHEAD_CSUM	(12)
 
+#define check_warn(ret, fmt, args...) \
+	({ if (ret < 0) netdev_warn(dev->net, fmt, ##args); })
+
+#define check_warn_return(ret, fmt, args...) \
+	({ if (ret < 0) { netdev_warn(dev->net, fmt, ##args); return ret; } })
+
+#define check_warn_goto_done(ret, fmt, args...) \
+	({ if (ret < 0) { netdev_warn(dev->net, fmt, ##args); goto done; } })
+
 struct smsc95xx_priv {
 	u32 mac_cr;
 	u32 hash_hi;
@@ -63,7 +72,8 @@ static bool turbo_mode = true;
 module_param(turbo_mode, bool, 0644);
 MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction");
 
-static int smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data)
+static int __must_check smsc95xx_read_reg(struct usbnet *dev, u32 index,
+					  u32 *data)
 {
 	u32 *buf = kmalloc(4, GFP_KERNEL);
 	int ret;
@@ -88,7 +98,8 @@ static int smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data)
 	return ret;
 }
 
-static int smsc95xx_write_reg(struct usbnet *dev, u32 index, u32 data)
+static int __must_check smsc95xx_write_reg(struct usbnet *dev, u32 index,
+					   u32 data)
 {
 	u32 *buf = kmalloc(4, GFP_KERNEL);
 	int ret;
@@ -116,13 +127,15 @@ static int smsc95xx_write_reg(struct usbnet *dev, u32 index, u32 data)
 
 /* Loop until the read is completed with timeout
  * called with phy_mutex held */
-static int smsc95xx_phy_wait_not_busy(struct usbnet *dev)
+static int __must_check smsc95xx_phy_wait_not_busy(struct usbnet *dev)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
+	int ret;
 
 	do {
-		smsc95xx_read_reg(dev, MII_ADDR, &val);
+		ret = smsc95xx_read_reg(dev, MII_ADDR, &val);
+		check_warn_return(ret, "Error reading MII_ACCESS");
 		if (!(val & MII_BUSY_))
 			return 0;
 	} while (!time_after(jiffies, start_time + HZ));
@@ -134,33 +147,32 @@ static int smsc95xx_mdio_read(struct net_device *netdev, int phy_id, int idx)
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	u32 val, addr;
+	int ret;
 
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	if (smsc95xx_phy_wait_not_busy(dev)) {
-		netdev_warn(dev->net, "MII is busy in smsc95xx_mdio_read\n");
-		mutex_unlock(&dev->phy_mutex);
-		return -EIO;
-	}
+	ret = smsc95xx_phy_wait_not_busy(dev);
+	check_warn_goto_done(ret, "MII is busy in smsc95xx_mdio_read");
 
 	/* set the address, index & direction (read from PHY) */
 	phy_id &= dev->mii.phy_id_mask;
 	idx &= dev->mii.reg_num_mask;
 	addr = (phy_id << 11) | (idx << 6) | MII_READ_;
-	smsc95xx_write_reg(dev, MII_ADDR, addr);
+	ret = smsc95xx_write_reg(dev, MII_ADDR, addr);
+	check_warn_goto_done(ret, "Error writing MII_ADDR");
 
-	if (smsc95xx_phy_wait_not_busy(dev)) {
-		netdev_warn(dev->net, "Timed out reading MII reg %02X\n", idx);
-		mutex_unlock(&dev->phy_mutex);
-		return -EIO;
-	}
+	ret = smsc95xx_phy_wait_not_busy(dev);
+	check_warn_goto_done(ret, "Timed out reading MII reg %02X", idx);
 
-	smsc95xx_read_reg(dev, MII_DATA, &val);
+	ret = smsc95xx_read_reg(dev, MII_DATA, &val);
+	check_warn_goto_done(ret, "Error reading MII_DATA");
 
-	mutex_unlock(&dev->phy_mutex);
+	ret = (u16)(val & 0xFFFF);
 
-	return (u16)(val & 0xFFFF);
+done:
+	mutex_unlock(&dev->phy_mutex);
+	return ret;
 }
 
 static void smsc95xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
@@ -168,38 +180,41 @@ static void smsc95xx_mdio_write(struct net_device *netdev, int phy_id, int idx,
 {
 	struct usbnet *dev = netdev_priv(netdev);
 	u32 val, addr;
+	int ret;
 
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	if (smsc95xx_phy_wait_not_busy(dev)) {
-		netdev_warn(dev->net, "MII is busy in smsc95xx_mdio_write\n");
-		mutex_unlock(&dev->phy_mutex);
-		return;
-	}
+	ret = smsc95xx_phy_wait_not_busy(dev);
+	check_warn_goto_done(ret, "MII is busy in smsc95xx_mdio_write");
 
 	val = regval;
-	smsc95xx_write_reg(dev, MII_DATA, val);
+	ret = smsc95xx_write_reg(dev, MII_DATA, val);
+	check_warn_goto_done(ret, "Error writing MII_DATA");
 
 	/* set the address, index & direction (write to PHY) */
 	phy_id &= dev->mii.phy_id_mask;
 	idx &= dev->mii.reg_num_mask;
 	addr = (phy_id << 11) | (idx << 6) | MII_WRITE_;
-	smsc95xx_write_reg(dev, MII_ADDR, addr);
+	ret = smsc95xx_write_reg(dev, MII_ADDR, addr);
+	check_warn_goto_done(ret, "Error writing MII_ADDR");
 
-	if (smsc95xx_phy_wait_not_busy(dev))
-		netdev_warn(dev->net, "Timed out writing MII reg %02X\n", idx);
+	ret = smsc95xx_phy_wait_not_busy(dev);
+	check_warn_goto_done(ret, "Timed out writing MII reg %02X", idx);
 
+done:
 	mutex_unlock(&dev->phy_mutex);
 }
 
-static int smsc95xx_wait_eeprom(struct usbnet *dev)
+static int __must_check smsc95xx_wait_eeprom(struct usbnet *dev)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
+	int ret;
 
 	do {
-		smsc95xx_read_reg(dev, E2P_CMD, &val);
+		ret = smsc95xx_read_reg(dev, E2P_CMD, &val);
+		check_warn_return(ret, "Error reading E2P_CMD");
 		if (!(val & E2P_CMD_BUSY_) || (val & E2P_CMD_TIMEOUT_))
 			break;
 		udelay(40);
@@ -213,13 +228,15 @@ static int smsc95xx_wait_eeprom(struct usbnet *dev)
 	return 0;
 }
 
-static int smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev)
+static int __must_check smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
+	int ret;
 
 	do {
-		smsc95xx_read_reg(dev, E2P_CMD, &val);
+		ret = smsc95xx_read_reg(dev, E2P_CMD, &val);
+		check_warn_return(ret, "Error reading E2P_CMD");
 
 		if (!(val & E2P_CMD_BUSY_))
 			return 0;
@@ -246,13 +263,15 @@ static int smsc95xx_read_eeprom(struct usbnet *dev, u32 offset, u32 length,
 
 	for (i = 0; i < length; i++) {
 		val = E2P_CMD_BUSY_ | E2P_CMD_READ_ | (offset & E2P_CMD_ADDR_);
-		smsc95xx_write_reg(dev, E2P_CMD, val);
+		ret = smsc95xx_write_reg(dev, E2P_CMD, val);
+		check_warn_return(ret, "Error writing E2P_CMD");
 
 		ret = smsc95xx_wait_eeprom(dev);
 		if (ret < 0)
 			return ret;
 
-		smsc95xx_read_reg(dev, E2P_DATA, &val);
+		ret = smsc95xx_read_reg(dev, E2P_DATA, &val);
+		check_warn_return(ret, "Error reading E2P_DATA");
 
 		data[i] = val & 0xFF;
 		offset++;
@@ -276,7 +295,8 @@ static int smsc95xx_write_eeprom(struct usbnet *dev, u32 offset, u32 length,
 
 	/* Issue write/erase enable command */
 	val = E2P_CMD_BUSY_ | E2P_CMD_EWEN_;
-	smsc95xx_write_reg(dev, E2P_CMD, val);
+	ret = smsc95xx_write_reg(dev, E2P_CMD, val);
+	check_warn_return(ret, "Error writing E2P_DATA");
 
 	ret = smsc95xx_wait_eeprom(dev);
 	if (ret < 0)
@@ -286,11 +306,13 @@ static int smsc95xx_write_eeprom(struct usbnet *dev, u32 offset, u32 length,
 
 		/* Fill data register */
 		val = data[i];
-		smsc95xx_write_reg(dev, E2P_DATA, val);
+		ret = smsc95xx_write_reg(dev, E2P_DATA, val);
+		check_warn_return(ret, "Error writing E2P_DATA");
 
 		/* Send "write" command */
 		val = E2P_CMD_BUSY_ | E2P_CMD_WRITE_ | (offset & E2P_CMD_ADDR_);
-		smsc95xx_write_reg(dev, E2P_CMD, val);
+		ret = smsc95xx_write_reg(dev, E2P_CMD, val);
+		check_warn_return(ret, "Error writing E2P_CMD");
 
 		ret = smsc95xx_wait_eeprom(dev);
 		if (ret < 0)
@@ -308,14 +330,14 @@ static void smsc95xx_async_cmd_callback(struct urb *urb)
 	struct usbnet *dev = usb_context->dev;
 	int status = urb->status;
 
-	if (status < 0)
-		netdev_warn(dev->net, "async callback failed with %d\n", status);
+	check_warn(status, "async callback failed with %d\n", status);
 
 	kfree(usb_context);
 	usb_free_urb(urb);
 }
 
-static int smsc95xx_write_reg_async(struct usbnet *dev, u16 index, u32 *data)
+static int __must_check smsc95xx_write_reg_async(struct usbnet *dev, u16 index,
+						 u32 *data)
 {
 	struct usb_context *usb_context;
 	int status;
@@ -371,6 +393,7 @@ static void smsc95xx_set_multicast(struct net_device *netdev)
 	struct usbnet *dev = netdev_priv(netdev);
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	unsigned long flags;
+	int ret;
 
 	pdata->hash_hi = 0;
 	pdata->hash_lo = 0;
@@ -411,21 +434,23 @@ static void smsc95xx_set_multicast(struct net_device *netdev)
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
 	/* Initiate async writes, as we can't wait for completion here */
-	smsc95xx_write_reg_async(dev, HASHH, &pdata->hash_hi);
-	smsc95xx_write_reg_async(dev, HASHL, &pdata->hash_lo);
-	smsc95xx_write_reg_async(dev, MAC_CR, &pdata->mac_cr);
+	ret = smsc95xx_write_reg_async(dev, HASHH, &pdata->hash_hi);
+	check_warn(ret, "failed to initiate async write to HASHH");
+
+	ret = smsc95xx_write_reg_async(dev, HASHL, &pdata->hash_lo);
+	check_warn(ret, "failed to initiate async write to HASHL");
+
+	ret = smsc95xx_write_reg_async(dev, MAC_CR, &pdata->mac_cr);
+	check_warn(ret, "failed to initiate async write to MAC_CR");
 }
 
-static void smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
-					    u16 lcladv, u16 rmtadv)
+static int smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
+					   u16 lcladv, u16 rmtadv)
 {
 	u32 flow, afc_cfg = 0;
 
 	int ret = smsc95xx_read_reg(dev, AFC_CFG, &afc_cfg);
-	if (ret < 0) {
-		netdev_warn(dev->net, "error reading AFC_CFG\n");
-		return;
-	}
+	check_warn_return(ret, "Error reading AFC_CFG");
 
 	if (duplex == DUPLEX_FULL) {
 		u8 cap = mii_resolve_flowctrl_fdx(lcladv, rmtadv);
@@ -449,8 +474,13 @@ static void smsc95xx_phy_update_flowcontrol(struct usbnet *dev, u8 duplex,
 		afc_cfg |= 0xF;
 	}
 
-	smsc95xx_write_reg(dev, FLOW, flow);
-	smsc95xx_write_reg(dev,	AFC_CFG, afc_cfg);
+	ret = smsc95xx_write_reg(dev, FLOW, flow);
+	check_warn_return(ret, "Error writing FLOW");
+
+	ret = smsc95xx_write_reg(dev, AFC_CFG, afc_cfg);
+	check_warn_return(ret, "Error writing AFC_CFG");
+
+	return 0;
 }
 
 static int smsc95xx_link_reset(struct usbnet *dev)
@@ -460,10 +490,14 @@ static int smsc95xx_link_reset(struct usbnet *dev)
 	struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET };
 	unsigned long flags;
 	u16 lcladv, rmtadv;
+	int ret;
 
 	/* clear interrupt status */
-	smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
-	smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
+	ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
+	check_warn_return(ret, "Error reading PHY_INT_SRC");
+
+	ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
+	check_warn_return(ret, "Error writing INT_STS");
 
 	mii_check_media(mii, 1, 1);
 	mii_ethtool_gset(&dev->mii, &ecmd);
@@ -484,9 +518,11 @@ static int smsc95xx_link_reset(struct usbnet *dev)
 	}
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
-	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	check_warn_return(ret, "Error writing MAC_CR");
 
-	smsc95xx_phy_update_flowcontrol(dev, ecmd.duplex, lcladv, rmtadv);
+	ret = smsc95xx_phy_update_flowcontrol(dev, ecmd.duplex, lcladv, rmtadv);
+	check_warn_return(ret, "Error updating PHY flow control");
 
 	return 0;
 }
@@ -522,10 +558,7 @@ static int smsc95xx_set_features(struct net_device *netdev,
 	int ret;
 
 	ret = smsc95xx_read_reg(dev, COE_CR, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read COE_CR: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read COE_CR: %d\n", ret);
 
 	if (features & NETIF_F_HW_CSUM)
 		read_buf |= Tx_COE_EN_;
@@ -538,10 +571,7 @@ static int smsc95xx_set_features(struct net_device *netdev,
 		read_buf &= ~Rx_COE_EN_;
 
 	ret = smsc95xx_write_reg(dev, COE_CR, read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write COE_CR: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write COE_CR: %d\n", ret);
 
 	netif_dbg(dev, hw, dev->net, "COE_CR = 0x%08x\n", read_buf);
 	return 0;
@@ -656,53 +686,56 @@ static int smsc95xx_set_mac_address(struct usbnet *dev)
 	int ret;
 
 	ret = smsc95xx_write_reg(dev, ADDRL, addr_lo);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write ADDRL: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write ADDRL: %d\n", ret);
 
 	ret = smsc95xx_write_reg(dev, ADDRH, addr_hi);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write ADDRH: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write ADDRH: %d\n", ret);
 
 	return 0;
 }
 
 /* starts the TX path */
-static void smsc95xx_start_tx_path(struct usbnet *dev)
+static int smsc95xx_start_tx_path(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	unsigned long flags;
+	int ret;
 
 	/* Enable Tx at MAC */
 	spin_lock_irqsave(&pdata->mac_cr_lock, flags);
 	pdata->mac_cr |= MAC_CR_TXEN_;
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
-	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	check_warn_return(ret, "Failed to write MAC_CR: %d\n", ret);
 
 	/* Enable Tx at SCSRs */
-	smsc95xx_write_reg(dev, TX_CFG, TX_CFG_ON_);
+	ret = smsc95xx_write_reg(dev, TX_CFG, TX_CFG_ON_);
+	check_warn_return(ret, "Failed to write TX_CFG: %d\n", ret);
+
+	return 0;
 }
 
 /* Starts the Receive path */
-static void smsc95xx_start_rx_path(struct usbnet *dev)
+static int smsc95xx_start_rx_path(struct usbnet *dev)
 {
 	struct smsc95xx_priv *pdata = (struct smsc95xx_priv *)(dev->data[0]);
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&pdata->mac_cr_lock, flags);
 	pdata->mac_cr |= MAC_CR_RXEN_;
 	spin_unlock_irqrestore(&pdata->mac_cr_lock, flags);
 
-	smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	ret = smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr);
+	check_warn_return(ret, "Failed to write MAC_CR: %d\n", ret);
+
+	return 0;
 }
 
 static int smsc95xx_phy_initialize(struct usbnet *dev)
 {
-	int bmcr, timeout = 0;
+	int bmcr, ret, timeout = 0;
 
 	/* Initialize MII structure */
 	dev->mii.dev = dev->net;
@@ -731,7 +764,8 @@ static int smsc95xx_phy_initialize(struct usbnet *dev)
 		ADVERTISE_PAUSE_ASYM);
 
 	/* read to clear */
-	smsc95xx_mdio_read(dev->net, dev->mii.phy_id, PHY_INT_SRC);
+	ret = smsc95xx_mdio_read(dev->net, dev->mii.phy_id, PHY_INT_SRC);
+	check_warn_return(ret, "Failed to read PHY_INT_SRC during init");
 
 	smsc95xx_mdio_write(dev->net, dev->mii.phy_id, PHY_INT_MASK,
 		PHY_INT_MASK_DEFAULT_);
@@ -750,20 +784,13 @@ static int smsc95xx_reset(struct usbnet *dev)
 	netif_dbg(dev, ifup, dev->net, "entering smsc95xx_reset\n");
 
 	ret = smsc95xx_write_reg(dev, HW_CFG, HW_CFG_LRST_);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write HW_CFG_LRST_ bit in HW_CFG register, ret = %d\n",
-			    ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write HW_CFG_LRST_ bit in HW_CFG\n");
 
 	timeout = 0;
 	do {
 		msleep(10);
 		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
-		if (ret < 0) {
-			netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
-			return ret;
-		}
+		check_warn_return(ret, "Failed to read HW_CFG: %d\n", ret);
 		timeout++;
 	} while ((read_buf & HW_CFG_LRST_) && (timeout < 100));
 
@@ -773,19 +800,13 @@ static int smsc95xx_reset(struct usbnet *dev)
 	}
 
 	ret = smsc95xx_write_reg(dev, PM_CTRL, PM_CTL_PHY_RST_);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write PM_CTRL: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write PM_CTRL: %d\n", ret);
 
 	timeout = 0;
 	do {
 		msleep(10);
 		ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
-		if (ret < 0) {
-			netdev_warn(dev->net, "Failed to read PM_CTRL: %d\n", ret);
-			return ret;
-		}
+		check_warn_return(ret, "Failed to read PM_CTRL: %d\n", ret);
 		timeout++;
 	} while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
 
@@ -802,10 +823,7 @@ static int smsc95xx_reset(struct usbnet *dev)
 		  "MAC Address: %pM\n", dev->net->dev_addr);
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read HW_CFG: %d\n", ret);
 
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from HW_CFG : 0x%08x\n", read_buf);
@@ -813,17 +831,10 @@ static int smsc95xx_reset(struct usbnet *dev)
 	read_buf |= HW_CFG_BIR_;
 
 	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write HW_CFG_BIR_ bit in HW_CFG register, ret = %d\n",
-			    ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write HW_CFG_BIR_ bit in HW_CFG\n");
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read HW_CFG: %d\n", ret);
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from HW_CFG after writing HW_CFG_BIR_: 0x%08x\n",
 		  read_buf);
@@ -843,40 +854,28 @@ static int smsc95xx_reset(struct usbnet *dev)
 		  "rx_urb_size=%ld\n", (ulong)dev->rx_urb_size);
 
 	ret = smsc95xx_write_reg(dev, BURST_CAP, burst_cap);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write BURST_CAP: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write BURST_CAP: %d\n", ret);
 
 	ret = smsc95xx_read_reg(dev, BURST_CAP, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read BURST_CAP: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read BURST_CAP: %d\n", ret);
+
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from BURST_CAP after writing: 0x%08x\n",
 		  read_buf);
 
 	ret = smsc95xx_write_reg(dev, BULK_IN_DLY, DEFAULT_BULK_IN_DELAY);
-	if (ret < 0) {
-		netdev_warn(dev->net, "ret = %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write BULK_IN_DLY: %d\n", ret);
 
 	ret = smsc95xx_read_reg(dev, BULK_IN_DLY, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read BULK_IN_DLY: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read BULK_IN_DLY: %d\n", ret);
+
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from BULK_IN_DLY after writing: 0x%08x\n",
 		  read_buf);
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read HW_CFG: %d\n", ret);
+
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from HW_CFG: 0x%08x\n", read_buf);
 
@@ -889,97 +888,66 @@ static int smsc95xx_reset(struct usbnet *dev)
 	read_buf |= NET_IP_ALIGN << 9;
 
 	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write HW_CFG register, ret=%d\n",
-			    ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write HW_CFG: %d\n", ret);
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read HW_CFG: %d\n", ret);
+
 	netif_dbg(dev, ifup, dev->net,
 		  "Read Value from HW_CFG after writing: 0x%08x\n", read_buf);
 
 	ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write INT_STS register, ret=%d\n",
-			    ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write INT_STS: %d\n", ret);
 
 	ret = smsc95xx_read_reg(dev, ID_REV, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read ID_REV: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read ID_REV: %d\n", ret);
 	netif_dbg(dev, ifup, dev->net, "ID_REV = 0x%08x\n", read_buf);
 
 	/* Configure GPIO pins as LED outputs */
 	write_buf = LED_GPIO_CFG_SPD_LED | LED_GPIO_CFG_LNK_LED |
 		LED_GPIO_CFG_FDX_LED;
 	ret = smsc95xx_write_reg(dev, LED_GPIO_CFG, write_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write LED_GPIO_CFG register, ret=%d\n",
-			    ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write LED_GPIO_CFG: %d\n", ret);
 
 	/* Init Tx */
 	ret = smsc95xx_write_reg(dev, FLOW, 0);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write FLOW: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write FLOW: %d\n", ret);
 
 	ret = smsc95xx_write_reg(dev, AFC_CFG, AFC_CFG_DEFAULT);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write AFC_CFG: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write AFC_CFG: %d\n", ret);
 
 	/* Don't need mac_cr_lock during initialisation */
 	ret = smsc95xx_read_reg(dev, MAC_CR, &pdata->mac_cr);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read MAC_CR: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read MAC_CR: %d\n", ret);
 
 	/* Init Rx */
 	/* Set Vlan */
 	ret = smsc95xx_write_reg(dev, VLAN1, (u32)ETH_P_8021Q);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write VAN1: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write VLAN1: %d\n", ret);
 
 	/* Enable or disable checksum offload engines */
-	smsc95xx_set_features(dev->net, dev->net->features);
+	ret = smsc95xx_set_features(dev->net, dev->net->features);
+	check_warn_return(ret, "Failed to set checksum offload features");
 
 	smsc95xx_set_multicast(dev->net);
 
-	if (smsc95xx_phy_initialize(dev) < 0)
-		return -EIO;
+	ret = smsc95xx_phy_initialize(dev);
+	check_warn_return(ret, "Failed to init PHY");
 
 	ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to read INT_EP_CTL: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to read INT_EP_CTL: %d\n", ret);
 
 	/* enable PHY interrupts */
 	read_buf |= INT_EP_CTL_PHY_INT_;
 
 	ret = smsc95xx_write_reg(dev, INT_EP_CTL, read_buf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "Failed to write INT_EP_CTL: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "Failed to write INT_EP_CTL: %d\n", ret);
 
-	smsc95xx_start_tx_path(dev);
-	smsc95xx_start_rx_path(dev);
+	ret = smsc95xx_start_tx_path(dev);
+	check_warn_return(ret, "Failed to start TX path");
+
+	ret = smsc95xx_start_rx_path(dev);
+	check_warn_return(ret, "Failed to start RX path");
 
 	netif_dbg(dev, ifup, dev->net, "smsc95xx_reset, return 0\n");
 	return 0;
@@ -1006,10 +974,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	printk(KERN_INFO SMSC_CHIPNAME " v" SMSC_DRIVER_VERSION "\n");
 
 	ret = usbnet_get_endpoints(dev, intf);
-	if (ret < 0) {
-		netdev_warn(dev->net, "usbnet_get_endpoints failed: %d\n", ret);
-		return ret;
-	}
+	check_warn_return(ret, "usbnet_get_endpoints failed: %d\n", ret);
 
 	dev->data[0] = (unsigned long)kzalloc(sizeof(struct smsc95xx_priv),
 		GFP_KERNEL);
-- 
1.7.9.5

[PATCH 4/5] smsc95xx: fix resume when usb device is reset

[Steve Glendinning]

This patch fixes an issue on some systems, where after suspend the
link is re-established but the ethernet interface does not resume.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index d0ff01e..f29860d 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1282,6 +1282,7 @@ static struct usb_driver smsc95xx_driver = {
 	.probe		= usbnet_probe,
 	.suspend	= usbnet_suspend,
 	.resume		= usbnet_resume,
+	.reset_resume	= usbnet_resume,
 	.disconnect	= usbnet_disconnect,
 	.disable_hub_initiated_lpm = 1,
 };
-- 
1.7.9.5

[PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Steve Glendinning]

This patch enables the device to enter its lowest power SUSPEND2
state during system suspend, instead of staying up using full power.

Patch updated to not add two pointers to .suspend & .resume.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |   27 ++++++++++++++++++++++++++-
 drivers/net/usb/smsc95xx.h |    6 +++++-
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index f29860d..6c89a08 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -1018,6 +1018,31 @@ static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
 	}
 }
 
+static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct usbnet *dev = usb_get_intfdata(intf);
+	int ret;
+	u32 val;
+
+	BUG_ON(!dev);
+
+	ret = usbnet_suspend(intf, message);
+	check_warn_return(ret, "usbnet_suspend error");
+
+	netdev_info(dev->net, "entering SUSPEND2 mode");
+
+	ret = smsc95xx_read_reg(dev, PM_CTRL, &val);
+	check_warn_return(ret, "Error reading PM_CTRL");
+
+	val &= ~(PM_CTL_SUS_MODE_ | PM_CTL_WUPS_ | PM_CTL_PHY_RST_);
+	val |= PM_CTL_SUS_MODE_2;
+
+	ret = smsc95xx_write_reg(dev, PM_CTRL, val);
+	check_warn_return(ret, "Error writing PM_CTRL");
+
+	return 0;
+}
+
 static void smsc95xx_rx_csum_offload(struct sk_buff *skb)
 {
 	skb->csum = *(u16 *)(skb_tail_pointer(skb) - 2);
@@ -1280,7 +1305,7 @@ static struct usb_driver smsc95xx_driver = {
 	.name		= "smsc95xx",
 	.id_table	= products,
 	.probe		= usbnet_probe,
-	.suspend	= usbnet_suspend,
+	.suspend	= smsc95xx_suspend,
 	.resume		= usbnet_resume,
 	.reset_resume	= usbnet_resume,
 	.disconnect	= usbnet_disconnect,
diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h
index a275b62..89ad925 100644
--- a/drivers/net/usb/smsc95xx.h
+++ b/drivers/net/usb/smsc95xx.h
@@ -84,12 +84,16 @@
 #define HW_CFG_BCE_			(0x00000002)
 #define HW_CFG_SRST_			(0x00000001)
 
+#define RX_FIFO_INF			(0x18)
+
 #define PM_CTRL				(0x20)
+#define PM_CTL_RES_CLR_WKP_STS		(0x00000200)
 #define PM_CTL_DEV_RDY_			(0x00000080)
 #define PM_CTL_SUS_MODE_		(0x00000060)
 #define PM_CTL_SUS_MODE_0		(0x00000000)
 #define PM_CTL_SUS_MODE_1		(0x00000020)
-#define PM_CTL_SUS_MODE_2		(0x00000060)
+#define PM_CTL_SUS_MODE_2		(0x00000040)
+#define PM_CTL_SUS_MODE_3		(0x00000060)
 #define PM_CTL_PHY_RST_			(0x00000010)
 #define PM_CTL_WOL_EN_			(0x00000008)
 #define PM_CTL_ED_EN_			(0x00000004)
-- 
1.7.9.5

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Bjørn Mork]

Steve Glendinning <steve.glendinning@shawell.net> writes:

> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct usbnet *dev = usb_get_intfdata(intf);
> +	int ret;
> +	u32 val;
> +
> +	BUG_ON(!dev);

That's not very user friendly.  Why not just return here?



Bjørn

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Steve Glendinning]

On 26 September 2012 15:23, Bjørn Mork <bjorn@mork.no> wrote:
> Steve Glendinning <steve.glendinning@shawell.net> writes:
>
>> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>> +{
>> +     struct usbnet *dev = usb_get_intfdata(intf);
>> +     int ret;
>> +     u32 val;
>> +
>> +     BUG_ON(!dev);
>
> That's not very user friendly.  Why not just return here?

I hadn't thought that was a situation that could arise, is it?  Would
this happen if the USB device was removed during suspend?

-Steve

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:
> On 26 September 2012 15:23, Bjørn Mork <bjorn@mork.no> wrote:
>> Steve Glendinning <steve.glendinning@shawell.net> writes:
>>
>>> +static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
>>> +{
>>> +     struct usbnet *dev = usb_get_intfdata(intf);
>>> +     int ret;
>>> +     u32 val;
>>> +
>>> +     BUG_ON(!dev);
>>
>> That's not very user friendly.  Why not just return here?
>
> I hadn't thought that was a situation that could arise, is it?  Would
> this happen if the USB device was removed during suspend?

No, it should not happen.  But then, why test at all?


Bjørn

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Steve Glendinning]

>> I hadn't thought that was a situation that could arise, is it?  Would
>> this happen if the USB device was removed during suspend?
>
> No, it should not happen.  But then, why test at all?

I thought it was common practice to add these tests to document an
assumption the developer made that later code relies on?  I had
assumed that the !dev condition should not be possible, hence the
simple BUG test.  If it is possible then I agree - I definitely need
to handle this more gracefully.

In this case, asserting that dev is not NULL will make the code fail
loudly there instead of a few lines down when the netdev_info call
dereferences dev->net.  Either way something bad will happen!

-Steve

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:

>>> I hadn't thought that was a situation that could arise, is it?  Would
>>> this happen if the USB device was removed during suspend?
>>
>> No, it should not happen.  But then, why test at all?
>
> I thought it was common practice to add these tests to document an
> assumption the developer made that later code relies on?  I had
> assumed that the !dev condition should not be possible, hence the
> simple BUG test.  If it is possible then I agree - I definitely need
> to handle this more gracefully.
>
> In this case, asserting that dev is not NULL will make the code fail
> loudly there instead of a few lines down when the netdev_info call
> dereferences dev->net.  Either way something bad will happen!

Yes, but you are a lot less likely to know about it if you BUG out.  The
user will be left with no other choice than hitting reset or poweroff.
What's the point of that?

If your driver crashes but the machine is left running, then the user
may forward the Oops to you.  That's much more useful.


Bjørn

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Steve Glendinning]

On 26 September 2012 17:17, Bjørn Mork <bjorn@mork.no> wrote:
> Yes, but you are a lot less likely to know about it if you BUG out.  The
> user will be left with no other choice than hitting reset or poweroff.
> What's the point of that?
>
> If your driver crashes but the machine is left running, then the user
> may forward the Oops to you.  That's much more useful.

Good point, I hadn't considered that.

So for user reportability am I better off to use WARN_ON in this case,
or simply remove the check and let the null pointer dereference
happen?

-Steve

Re: [PATCH 5/5] smsc95xx: enable power saving mode during system suspend

[Bjørn Mork]

Steve Glendinning <steve@shawell.net> writes:
> On 26 September 2012 17:17, Bjørn Mork <bjorn@mork.no> wrote:
>> Yes, but you are a lot less likely to know about it if you BUG out.  The
>> user will be left with no other choice than hitting reset or poweroff.
>> What's the point of that?
>>
>> If your driver crashes but the machine is left running, then the user
>> may forward the Oops to you.  That's much more useful.
>
> Good point, I hadn't considered that.
>
> So for user reportability am I better off to use WARN_ON in this case,
> or simply remove the check and let the null pointer dereference
> happen?

Exactly.  See also http://permalink.gmane.org/gmane.linux.kernel/1365689


Bjørn

[PATCH 0/5] smsc95xx enhancements

[Steve Glendinning]

This patchset makes a few minor enhancements to the smsc95xx driver
in preparation for some forthcoming power-saving improvements I'm
working on.

Please consider all for net-next.

Steve Glendinning (5):
  smsc95xx: sleep before read for lengthy operations
  smsc95xx: remove unnecessary variables
  smsc95xx: check return code from control messages
  smsc95xx: fix resume when usb device is reset
  smsc95xx: enable power saving mode during system suspend

 drivers/net/usb/smsc95xx.c |  389 +++++++++++++++++++++-----------------------
 drivers/net/usb/smsc95xx.h |    7 +-
 2 files changed, 192 insertions(+), 204 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] smsc95xx: sleep before read for lengthy operations

[Steve Glendinning]

During init, the device reset is unexpected to complete immediately,
so sleep before testing the condition rather than after it.

Signed-off-by: Steve Glendinning <steve.glendinning@shawell.net>
---
 drivers/net/usb/smsc95xx.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index d45e539..ed1e551 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -763,12 +763,12 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	timeout = 0;
 	do {
+		msleep(10);
 		ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
 		if (ret < 0) {
 			netdev_warn(dev->net, "Failed to read HW_CFG: %d\n", ret);
 			return ret;
 		}
-		msleep(10);
 		timeout++;
 	} while ((read_buf & HW_CFG_LRST_) && (timeout < 100));
 
@@ -786,12 +786,12 @@ static int smsc95xx_reset(struct usbnet *dev)
 
 	timeout = 0;
 	do {
+		msleep(10);
 		ret = smsc95xx_read_reg(dev, PM_CTRL, &read_buf);
 		if (ret < 0) {
 			netdev_warn(dev->net, "Failed to read PM_CTRL: %d\n", ret);
 			return ret;
 		}
-		msleep(10);
 		timeout++;
 	} while ((read_buf & PM_CTL_PHY_RST_) && (timeout < 100));
 
-- 
1.7.9.5