Skip to contentSkip to conversation

net: variables reach -1, but 0 tested

PATCH Applied Roel Kluin 3 participants 5 messages Jan 31, 2009 – Feb 3, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 net: variables reach -1, but 0 tested

With a postfix decrement these reach -1 rather than 0, but after the loop it is tested whether they have become 0.

Signed-off-by Roel Kluin <[email protected]>
---
 drivers/net/ibm_newemac/phy.c |    4 ++--
 drivers/net/smc911x.c         |    2 +-
 drivers/net/smsc9420.c        |   12 ++++++------
 drivers/net/sungem.c          |    2 +-
 drivers/net/sunqe.c           |    2 +-
 drivers/net/tsi108_eth.c      |    2 +-
 drivers/net/tulip/de2104x.c   |    2 +-
 7 files changed, 13 insertions(+), 13 deletions(-)
drivers/net/ibm_newemac/phy.c +2 -2
index c40cd8d..ac9d964 100644
@@ -60,7 +60,7 @@ int emac_mii_reset_phy(struct mii_phy *phy)
 
 	udelay(300);
 
-	while (limit--) {
+	while (--limit) {
 		val = phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;

@@ -84,7 +84,7 @@ int emac_mii_reset_gpcs(struct mii_phy *phy)
 
 	udelay(300);
 
-	while (limit--) {
+	while (--limit) {
 		val = gpcs_phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;
drivers/net/smc911x.c +1 -1
index bf3aa2a..527c336 100644
@@ -220,7 +220,7 @@ static void smc911x_reset(struct net_device *dev)
 
 	/* make sure EEPROM has finished loading before setting GPIO_CFG */
 	timeout=1000;
-	while ( timeout-- && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
+	while ( --timeout && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
 		udelay(10);
 	}
 	if (timeout == 0){
drivers/net/smsc9420.c +6 -6
index c14a4c6..41f5303 100644
@@ -498,7 +498,7 @@ static void smsc9420_check_mac_address(struct net_device *dev)
 static void smsc9420_stop_tx(struct smsc9420_pdata *pd)
 {
 	u32 dmac_control, mac_cr, dma_intr_ena;
-	int timeOut = 1000;
+	int timeout = 1000;
 
 	/* disable TX DMAC */
 	dmac_control = smsc9420_reg_read(pd, DMAC_CONTROL);

@@ -506,13 +506,13 @@ static void smsc9420_stop_tx(struct smsc9420_pdata *pd)
 	smsc9420_reg_write(pd, DMAC_CONTROL, dmac_control);
 
 	/* Wait max 10ms for transmit process to stop */
-	while (timeOut--) {
+	while (--timeout) {
 		if (smsc9420_reg_read(pd, DMAC_STATUS) & DMAC_STS_TS_)
 			break;
 		udelay(10);
 	}
 
-	if (!timeOut)
+	if (!timeout)
 		smsc_warn(IFDOWN, "TX DMAC failed to stop");
 
 	/* ACK Tx DMAC stop bit */

@@ -596,7 +596,7 @@ static void smsc9420_free_rx_ring(struct smsc9420_pdata *pd)
 
 static void smsc9420_stop_rx(struct smsc9420_pdata *pd)
 {
-	int timeOut = 1000;
+	int timeout = 1000;
 	u32 mac_cr, dmac_control, dma_intr_ena;
 
 	/* mask RX DMAC interrupts */

@@ -617,13 +617,13 @@ static void smsc9420_stop_rx(struct smsc9420_pdata *pd)
 	smsc9420_pci_flush_write(pd);
 
 	/* wait up to 10ms for receive to stop */
-	while (timeOut--) {
+	while (--timeout) {
 		if (smsc9420_reg_read(pd, DMAC_STATUS) & DMAC_STS_RS_)
 			break;
 		udelay(10);
 	}
 
-	if (!timeOut)
+	if (!timeout)
 		smsc_warn(IFDOWN, "RX DMAC did not stop! timeout.");
 
 	/* ACK the Rx DMAC stop bit */
drivers/net/sungem.c +1 -1
index 86c765d..b17efa9 100644
@@ -148,7 +148,7 @@ static u16 __phy_read(struct gem *gp, int phy_addr, int reg)
 	cmd |= (MIF_FRAME_TAMSB);
 	writel(cmd, gp->regs + MIF_FRAME);
 
-	while (limit--) {
+	while (--limit) {
 		cmd = readl(gp->regs + MIF_FRAME);
 		if (cmd & MIF_FRAME_TALSB)
 			break;
drivers/net/sunqe.c +1 -1
index 6e8f377..fe0c3f2 100644
@@ -227,7 +227,7 @@ static int qe_init(struct sunqe *qep, int from_irq)
 	if (!(sbus_readb(mregs + MREGS_PHYCONFIG) & MREGS_PHYCONFIG_LTESTDIS)) {
 		int tries = 50;
 
-		while (tries--) {
+		while (--tries) {
 			u8 tmp;
 
 			mdelay(5);
drivers/net/tsi108_eth.c +1 -1
index 75461db..a9fd2b2 100644
@@ -1237,7 +1237,7 @@ static void tsi108_init_phy(struct net_device *dev)
 	spin_lock_irqsave(&phy_lock, flags);
 
 	tsi108_write_mii(data, MII_BMCR, BMCR_RESET);
-	while (i--){
+	while (--i) {
 		if(!(tsi108_read_mii(data, MII_BMCR) & BMCR_RESET))
 			break;
 		udelay(10);
drivers/net/tulip/de2104x.c +1 -1
index d5d53b6..0bf2114 100644
@@ -392,7 +392,7 @@ static void de_rx (struct de_private *de)
 	unsigned drop = 0;
 	int rc;
 
-	while (rx_work--) {
+	while (--rx_work) {
 		u32 status, len;
 		dma_addr_t mapping;
 		struct sk_buff *skb, *copy_skb;
#1 lore raw

drivers/net/ibm_newemac/phy.c int emac_mii_reset_phy(struct mii_phy *phy) · line 66

Quoted 6 lines · Roel Kluin
drivers/net/ibm_newemac/phy.c +1 -1
-	while (limit--) {
+	while (--limit) {
 		val = phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;

It looks like these are checked for <= to 0 , are these changes strictly nessesary?

drivers/net/smc911x.c static void smc911x_reset(struct net_device *dev) · line 224

Quoted 6 lines
drivers/net/smc911x.c +1 -1
 	/* make sure EEPROM has finished loading before setting GPIO_CFG */
 	timeout=1000;
-	while ( timeout-- && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
+	while ( --timeout && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
 		udelay(10);

If your doing "timeOut" to "timeout" below may as well drop the space above after the "(" ..

Daniel

maintainer
#2 lore raw

From: Daniel Walker <[email protected]>
Date: Sat, 31 Jan 2009 12:35:12 -0800

Quoted 11 lines · Roel Kluin

drivers/net/ibm_newemac/phy.c int emac_mii_reset_phy(struct mii_phy *phy) · line 66

Quoted 7 lines
drivers/net/ibm_newemac/phy.c +1 -1
-	while (limit--) {
+	while (--limit) {
 		val = phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;

It looks like these are checked for <= to 0 , are these changes strictly nessesary?

Agreed.

I'd like to apply these fixes to net-2.6 but I can't if there are spurious cases in here.

I'm dropping this patch for now.

author
#3 lore raw
In reply to Daniel Walker
Quoted 11 lines · Daniel Walker

Review

Quoted 6 lines · Roel Kluin
(quoted diff) +1 -1
-	while (limit--) {
+	while (--limit) {
 		val = phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;

It looks like these are checked for <= to 0 , are these changes strictly nessesary?

Although the chance that this goes wrong in practice is small, my change was correct:

In the last iteration, limit is 1 when the test occurs, before the decrement causes limit to become 0. If just then the break occurs, we continue after the loop with:

        if ((val & BMCR_ISOLATE) && limit > 0)
                phy_write(phy, MII_BMCR, val & ~BMCR_ISOLATE);

        return limit <= 0;

Since limit is 0, the phy_write won't occur and the return is 1, this is not desired, I assume.

With the prefix decrement limit is always greater than 0 when the break occurs. I do agree that the chance that this goes wrong in practice could be small.

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +1 -1
 	/* make sure EEPROM has finished loading before setting GPIO_CFG */
 	timeout=1000;
-	while ( timeout-- && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
+	while ( --timeout && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
 		udelay(10);

If your doing "timeOut" to "timeout" below may as well drop the space above after the "(" ..

Ok, here it is again:

while (timeout--) { ... }

timeout becomes -1 if the loop isn't ended otherwise, not 0.

Signed-off-by Roel Kluin <[email protected]>
---
 drivers/net/ibm_newemac/phy.c |    4 ++--
 drivers/net/smc911x.c         |    4 ++--
 drivers/net/smsc9420.c        |   12 ++++++------
 drivers/net/sungem.c          |    2 +-
 drivers/net/sunqe.c           |    2 +-
 drivers/net/tsi108_eth.c      |    2 +-
 drivers/net/tulip/de2104x.c   |    2 +-
 7 files changed, 14 insertions(+), 14 deletions(-)
drivers/net/ibm_newemac/phy.c +2 -2
index c40cd8d..ac9d964 100644
@@ -60,7 +60,7 @@ int emac_mii_reset_phy(struct mii_phy *phy)
 
 	udelay(300);
 
-	while (limit--) {
+	while (--limit) {
 		val = phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;

@@ -84,7 +84,7 @@ int emac_mii_reset_gpcs(struct mii_phy *phy)
 
 	udelay(300);
 
-	while (limit--) {
+	while (--limit) {
 		val = gpcs_phy_read(phy, MII_BMCR);
 		if (val >= 0 && (val & BMCR_RESET) == 0)
 			break;
drivers/net/smc911x.c +2 -2
index bf3aa2a..223cde0 100644
@@ -220,9 +220,9 @@ static void smc911x_reset(struct net_device *dev)
 
 	/* make sure EEPROM has finished loading before setting GPIO_CFG */
 	timeout=1000;
-	while ( timeout-- && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_)) {
+	while (--timeout && (SMC_GET_E2P_CMD(lp) & E2P_CMD_EPC_BUSY_))
 		udelay(10);
-	}
+
 	if (timeout == 0){
 		PRINTK("%s: smc911x_reset timeout waiting for EEPROM busy\n", dev->name);
 		return;
drivers/net/smsc9420.c +6 -6
index c14a4c6..41f5303 100644
@@ -498,7 +498,7 @@ static void smsc9420_check_mac_address(struct net_device *dev)
 static void smsc9420_stop_tx(struct smsc9420_pdata *pd)
 {
 	u32 dmac_control, mac_cr, dma_intr_ena;
-	int timeOut = 1000;
+	int timeout = 1000;
 
 	/* disable TX DMAC */
 	dmac_control = smsc9420_reg_read(pd, DMAC_CONTROL);

@@ -506,13 +506,13 @@ static void smsc9420_stop_tx(struct smsc9420_pdata *pd)
 	smsc9420_reg_write(pd, DMAC_CONTROL, dmac_control);
 
 	/* Wait max 10ms for transmit process to stop */
-	while (timeOut--) {
+	while (--timeout) {
 		if (smsc9420_reg_read(pd, DMAC_STATUS) & DMAC_STS_TS_)
 			break;
 		udelay(10);
 	}
 
-	if (!timeOut)
+	if (!timeout)
 		smsc_warn(IFDOWN, "TX DMAC failed to stop");
 
 	/* ACK Tx DMAC stop bit */

@@ -596,7 +596,7 @@ static void smsc9420_free_rx_ring(struct smsc9420_pdata *pd)
 
 static void smsc9420_stop_rx(struct smsc9420_pdata *pd)
 {
-	int timeOut = 1000;
+	int timeout = 1000;
 	u32 mac_cr, dmac_control, dma_intr_ena;
 
 	/* mask RX DMAC interrupts */

@@ -617,13 +617,13 @@ static void smsc9420_stop_rx(struct smsc9420_pdata *pd)
 	smsc9420_pci_flush_write(pd);
 
 	/* wait up to 10ms for receive to stop */
-	while (timeOut--) {
+	while (--timeout) {
 		if (smsc9420_reg_read(pd, DMAC_STATUS) & DMAC_STS_RS_)
 			break;
 		udelay(10);
 	}
 
-	if (!timeOut)
+	if (!timeout)
 		smsc_warn(IFDOWN, "RX DMAC did not stop! timeout.");
 
 	/* ACK the Rx DMAC stop bit */
drivers/net/sungem.c +1 -1
index 86c765d..b17efa9 100644
@@ -148,7 +148,7 @@ static u16 __phy_read(struct gem *gp, int phy_addr, int reg)
 	cmd |= (MIF_FRAME_TAMSB);
 	writel(cmd, gp->regs + MIF_FRAME);
 
-	while (limit--) {
+	while (--limit) {
 		cmd = readl(gp->regs + MIF_FRAME);
 		if (cmd & MIF_FRAME_TALSB)
 			break;
drivers/net/sunqe.c +1 -1
index 6e8f377..fe0c3f2 100644
@@ -227,7 +227,7 @@ static int qe_init(struct sunqe *qep, int from_irq)
 	if (!(sbus_readb(mregs + MREGS_PHYCONFIG) & MREGS_PHYCONFIG_LTESTDIS)) {
 		int tries = 50;
 
-		while (tries--) {
+		while (--tries) {
 			u8 tmp;
 
 			mdelay(5);
drivers/net/tsi108_eth.c +1 -1
index 75461db..a9fd2b2 100644
@@ -1237,7 +1237,7 @@ static void tsi108_init_phy(struct net_device *dev)
 	spin_lock_irqsave(&phy_lock, flags);
 
 	tsi108_write_mii(data, MII_BMCR, BMCR_RESET);
-	while (i--){
+	while (--i) {
 		if(!(tsi108_read_mii(data, MII_BMCR) & BMCR_RESET))
 			break;
 		udelay(10);
drivers/net/tulip/de2104x.c +1 -1
index d5d53b6..0bf2114 100644
@@ -392,7 +392,7 @@ static void de_rx (struct de_private *de)
 	unsigned drop = 0;
 	int rc;
 
-	while (rx_work--) {
+	while (--rx_work) {
 		u32 status, len;
 		dma_addr_t mapping;
 		struct sk_buff *skb, *copy_skb;