* [PATCH] net: variables reach -1, but 0 tested @ 2009-01-31 10:36 Roel Kluin 2009-01-31 20:35 ` Daniel Walker 0 siblings, 1 reply; 5+ messages in thread From: Roel Kluin @ 2009-01-31 10:36 UTC (permalink / raw) To: netdev; +Cc: lkml 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 <roel.kluin@gmail.com> --- 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(-) diff --git a/drivers/net/ibm_newemac/phy.c b/drivers/net/ibm_newemac/phy.c index c40cd8d..ac9d964 100644 --- a/drivers/net/ibm_newemac/phy.c +++ b/drivers/net/ibm_newemac/phy.c @@ -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; diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c index bf3aa2a..527c336 100644 --- a/drivers/net/smc911x.c +++ b/drivers/net/smc911x.c @@ -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){ diff --git a/drivers/net/smsc9420.c b/drivers/net/smsc9420.c index c14a4c6..41f5303 100644 --- a/drivers/net/smsc9420.c +++ b/drivers/net/smsc9420.c @@ -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 */ diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c index 86c765d..b17efa9 100644 --- a/drivers/net/sungem.c +++ b/drivers/net/sungem.c @@ -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; diff --git a/drivers/net/sunqe.c b/drivers/net/sunqe.c index 6e8f377..fe0c3f2 100644 --- a/drivers/net/sunqe.c +++ b/drivers/net/sunqe.c @@ -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); diff --git a/drivers/net/tsi108_eth.c b/drivers/net/tsi108_eth.c index 75461db..a9fd2b2 100644 --- a/drivers/net/tsi108_eth.c +++ b/drivers/net/tsi108_eth.c @@ -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); diff --git a/drivers/net/tulip/de2104x.c b/drivers/net/tulip/de2104x.c index d5d53b6..0bf2114 100644 --- a/drivers/net/tulip/de2104x.c +++ b/drivers/net/tulip/de2104x.c @@ -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; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: variables reach -1, but 0 tested 2009-01-31 10:36 [PATCH] net: variables reach -1, but 0 tested Roel Kluin @ 2009-01-31 20:35 ` Daniel Walker 2009-02-01 9:58 ` David Miller 2009-02-01 16:23 ` Roel Kluin 0 siblings, 2 replies; 5+ messages in thread From: Daniel Walker @ 2009-01-31 20:35 UTC (permalink / raw) To: Roel Kluin; +Cc: netdev, lkml On Sat, 2009-01-31 at 11:36 +0100, Roel Kluin wrote: > > - 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? > > /* 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: variables reach -1, but 0 tested 2009-01-31 20:35 ` Daniel Walker @ 2009-02-01 9:58 ` David Miller 2009-02-01 16:23 ` Roel Kluin 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2009-02-01 9:58 UTC (permalink / raw) To: dwalker; +Cc: roel.kluin, netdev, linux-kernel From: Daniel Walker <dwalker@fifo99.com> Date: Sat, 31 Jan 2009 12:35:12 -0800 > On Sat, 2009-01-31 at 11:36 +0100, Roel Kluin wrote: > > > > > - 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: variables reach -1, but 0 tested 2009-01-31 20:35 ` Daniel Walker 2009-02-01 9:58 ` David Miller @ 2009-02-01 16:23 ` Roel Kluin 2009-02-03 7:18 ` David Miller 1 sibling, 1 reply; 5+ messages in thread From: Roel Kluin @ 2009-02-01 16:23 UTC (permalink / raw) To: Daniel Walker, davem; +Cc: netdev, lkml Daniel Walker wrote: > On Sat, 2009-01-31 at 11:36 +0100, Roel Kluin wrote: > >> >> - 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. >> /* 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 <roel.kluin@gmail.com> --- 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(-) diff --git a/drivers/net/ibm_newemac/phy.c b/drivers/net/ibm_newemac/phy.c index c40cd8d..ac9d964 100644 --- a/drivers/net/ibm_newemac/phy.c +++ b/drivers/net/ibm_newemac/phy.c @@ -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; diff --git a/drivers/net/smc911x.c b/drivers/net/smc911x.c index bf3aa2a..223cde0 100644 --- a/drivers/net/smc911x.c +++ b/drivers/net/smc911x.c @@ -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; diff --git a/drivers/net/smsc9420.c b/drivers/net/smsc9420.c index c14a4c6..41f5303 100644 --- a/drivers/net/smsc9420.c +++ b/drivers/net/smsc9420.c @@ -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 */ diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c index 86c765d..b17efa9 100644 --- a/drivers/net/sungem.c +++ b/drivers/net/sungem.c @@ -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; diff --git a/drivers/net/sunqe.c b/drivers/net/sunqe.c index 6e8f377..fe0c3f2 100644 --- a/drivers/net/sunqe.c +++ b/drivers/net/sunqe.c @@ -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); diff --git a/drivers/net/tsi108_eth.c b/drivers/net/tsi108_eth.c index 75461db..a9fd2b2 100644 --- a/drivers/net/tsi108_eth.c +++ b/drivers/net/tsi108_eth.c @@ -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); diff --git a/drivers/net/tulip/de2104x.c b/drivers/net/tulip/de2104x.c index d5d53b6..0bf2114 100644 --- a/drivers/net/tulip/de2104x.c +++ b/drivers/net/tulip/de2104x.c @@ -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; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: variables reach -1, but 0 tested 2009-02-01 16:23 ` Roel Kluin @ 2009-02-03 7:18 ` David Miller 0 siblings, 0 replies; 5+ messages in thread From: David Miller @ 2009-02-03 7:18 UTC (permalink / raw) To: roel.kluin; +Cc: dwalker, netdev, linux-kernel From: Roel Kluin <roel.kluin@gmail.com> Date: Sun, 01 Feb 2009 17:23:29 +0100 > Ok, here it is again: > > while (timeout--) { ... } > > timeout becomes -1 if the loop isn't ended otherwise, not 0. > > Signed-off-by: Roel Kluin <roel.kluin@gmail.com> Applied. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-02-03 7:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-31 10:36 [PATCH] net: variables reach -1, but 0 tested Roel Kluin 2009-01-31 20:35 ` Daniel Walker 2009-02-01 9:58 ` David Miller 2009-02-01 16:23 ` Roel Kluin 2009-02-03 7:18 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).