* [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).