Skip to contentSkip to conversation

qlcnic: fix a timeout loop

PATCH In review Dan Carpenter 5 participants 11 messages Dec 15, 2015 – Jan 5, 2016

Subsystems (4)
Networking Network drivers Kernel janitors QLogic qlcnic (1/10)Gb Ethernet driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 qlcnic: fix a timeout loop

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by changing it to a pre-op. I had to change the starting value from "QLCNIC_DEV_NPAR_OPER_TIMEO" (30) to "QLCNIC_DEV_NPAR_OPER_TIMEO + 1" so that we still loop the same number of times as before.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +2 -2
index be7d7a6..9919245 100644
@@ -234,7 +234,7 @@ int qlcnic_83xx_config_vnic_opmode(struct qlcnic_adapter *adapter)
 	}
 
 	ahw->idc.vnic_state = QLCNIC_DEV_NPAR_NON_OPER;
-	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;
+	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO + 1;
 
 	return 0;
 }

@@ -246,7 +246,7 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter *adapter)
 	u32 state;
 
 	state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
-	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
+	while (state != QLCNIC_DEV_NPAR_OPER && --idc->vnic_wait_limit) {
 		msleep(1000);
 		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
 	}
#1 lore raw
Quoted 44 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Dan Carpenter
Sent: Tuesday, December 15, 2015 3:46 PM
To: Dept-GE Linux NIC Dev <[email protected]>; Rajesh Borundia <[email protected]>
Cc: netdev <[email protected]>; [email protected]
Subject: [patch] qlcnic: fix a timeout loop

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by changing it to a pre-op. I had to change the starting value from "QLCNIC_DEV_NPAR_OPER_TIMEO" (30) to "QLCNIC_DEV_NPAR_OPER_TIMEO
+ 1" so that we still loop the same number of times as before.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +1 -1
index be7d7a6..9919245 100644
@@ -234,7 +234,7 @@ int qlcnic_83xx_config_vnic_opmode(struct
 qlcnic_adapter *adapter)
 	}
 
 	ahw->idc.vnic_state = QLCNIC_DEV_NPAR_NON_OPER;
-	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;
+	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO + 1;
 
 	return 0;

}

drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +1 -1
@@ -246,7 +246,7 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter
 *adapter)
 	u32 state;
 
 	state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
-	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
+	while (state != QLCNIC_DEV_NPAR_OPER && --idc->vnic_wait_limit) {
 		msleep(1000);
 		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
Quoted 1 line

}

Hi Dan,
It looks bit odd incrementing 1 in QLCNIC_DEV_NPAR_OPER_TIMEO. Can't we just post increment inside the loop ?

ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;

while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit) {
	idc->vnic_wait_limit--;
	-----;
	-----;
}

Thanks,
Manish

author
#2 lore raw

PATCH v2 1/1 qlcnic: fix a timeout loop

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by moving the decrement inside the loop.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>

---
v2: different color on the bikeshed

drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +2 -1
index be7d7a6..b1a452f 100644
@@ -246,7 +246,8 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter *adapter)
 	u32 state;
 
 	state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
-	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
+	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit) {
+		idc->vnic_wait_limit--;
 		msleep(1000);
 		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
 	}
maintainer Applied
#3 lore raw

From: Dan Carpenter <[email protected]>
Date: Tue, 15 Dec 2015 16:56:16 +0300

Quoted 7 lines

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by moving the decrement inside the loop.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>

Applied.

author
#4 lore raw
Quoted 8 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Tue, 15 Dec 2015 16:56:16 +0300

Quoted 5 lines

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by moving the decrement inside the loop.

Quoted 4 lines
Quoted 2 lines
Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>

Applied.

Ugh... I appologize, I just noticed a mistake in this one. I have to do the decrement at the end of the loop and not the start of the loop.

I will send a fix for that on top of this fix.

regards,
dan carpenter

author
#5 lore raw
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · David Miller

From: Dan Carpenter <[email protected]>
Date: Tue, 15 Dec 2015 16:56:16 +0300

Quoted 5 lines

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by moving the decrement inside the loop.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>

Applied.

Ugh... I appologize, I just noticed a mistake in this one. I have to do the decrement at the end of the loop and not the start of the loop.

Nope. I'm wrong again. My original patch was the correct way to do this. Otherwise you can end up with state = QLCNIC_DEV_NPAR_OPER and ->vnic_wait_limit set to zero.

regards,
dan carpenter

author
#6 lore raw

PATCH 1/1 qlcnic: fix a loop exit condition better

In the original code, if we succeeded on the last iteration through the loop then we still returned failure.

Fixes 389e4e04ad2d ('qlcnic: fix a timeout loop')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +1 -1
index b1a452f..3490675 100644
@@ -252,7 +252,7 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter *adapter)
 		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
 	}
 
-	if (!idc->vnic_wait_limit) {
+	if (state != QLCNIC_DEV_NPAR_OPER) {
 		dev_err(&adapter->pdev->dev,
 			"vNIC mode not operational, state check timed out.\n");
 		return -EIO;
maintainer Applied
#7 lore raw

Re: [patch] qlcnic: fix a loop exit condition better

From: Dan Carpenter <[email protected]>
Date: Thu, 24 Dec 2015 12:21:22 +0300

Quoted 5 lines

In the original code, if we succeeded on the last iteration through the loop then we still returned failure.

Fixes 389e4e04ad2d ('qlcnic: fix a timeout loop')
Signed-off-by Dan Carpenter <[email protected]>

Applied, thanks Dan.

#8 lore raw
In reply to Dan Carpenter
Quoted 25 lines · Dan Carpenter

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by moving the decrement inside the loop.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>

---
v2: different color on the bikeshed

drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +2 -2
index be7d7a6..b1a452f 100644
@@ -246,7 +246,8 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter *adapter)
  	u32 state;
 
  	state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
-	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
+	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit) {
+		idc->vnic_wait_limit--;
  		msleep(1000);
  		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
  	}
--
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi everybody, & Happy new year.

We're seeing very sporadic & rare kernel oops due to qlcnic, even with latests kernels. I only have 1 trace, which can be seen here https://paste2.org/4m8EGEG1

Others kernel oops (not recorded) showed different stack trace, but in the end, problem was always in qlcnic_xmit_frame

Any chance this bug is corrected by this very patch or is this a totally different problem ?

Cheers,
Yann.

author
#9 lore raw

No. This patch is a suspend resume thing and your bug is something else. Honestly, this patch is a static checker fix and I doubt it has much real worl impact at all.

regards,
dan carpenter

#10 lore raw
In reply to Manish Chopra

Am 15.12.2015 14:46, schrieb Manish Chopra:

Quoted 50 lines
Quoted 44 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Dan Carpenter
Sent: Tuesday, December 15, 2015 3:46 PM
To: Dept-GE Linux NIC Dev <[email protected]>; Rajesh Borundia <[email protected]>
Cc: netdev <[email protected]>; [email protected]
Subject: [patch] qlcnic: fix a timeout loop

The problem here is that at the end of the loop we test for if idc->vnic_wait_limit is zero, but since idc->vnic_wait_limit-- is a post-op, it actually ends up set to (u8)-1. I have fixed this by changing it to a pre-op. I had to change the starting value from "QLCNIC_DEV_NPAR_OPER_TIMEO" (30) to "QLCNIC_DEV_NPAR_OPER_TIMEO
+ 1" so that we still loop the same number of times as before.

Fixes 486a5bc77a4a ('qlcnic: Add support for 83xx suspend and resume.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +1 -1
index be7d7a6..9919245 100644
@@ -234,7 +234,7 @@ int qlcnic_83xx_config_vnic_opmode(struct
 qlcnic_adapter *adapter)
 	}
 
 	ahw->idc.vnic_state = QLCNIC_DEV_NPAR_NON_OPER;
-	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;
+	ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO + 1;
 
 	return 0;

}

drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c +1 -1
@@ -246,7 +246,7 @@ int qlcnic_83xx_check_vnic_state(struct qlcnic_adapter
 *adapter)
 	u32 state;
 
 	state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
-	while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit--) {
+	while (state != QLCNIC_DEV_NPAR_OPER && --idc->vnic_wait_limit) {
 		msleep(1000);
 		state = QLCRDX(ahw, QLC_83XX_VNIC_STATE);
Quoted 1 line

}

Hi Dan,
It looks bit odd incrementing 1 in QLCNIC_DEV_NPAR_OPER_TIMEO. Can't we just post increment inside the loop ?

ahw->idc.vnic_wait_limit = QLCNIC_DEV_NPAR_OPER_TIMEO;

Quoted 8 lines
while (state != QLCNIC_DEV_NPAR_OPER && idc->vnic_wait_limit) {
	idc->vnic_wait_limit--;
	-----;
	-----;
}

Thanks,
Manish

Hi Manish,
i would like to ask an other question. Why do you choose this way ? Basicly you have a
#define QLCNIC_DEV_NPAR_OPER_TIMEO idc->vnic_wait_limit=QLCNIC_DEV_NPAR_OPER_TIMEO;
while ( ... --idc->vnic_wait_limit)

Do you need the time it took to chance the state ?

Look at Dan patches, there is a whole list that shows that programmers are terrible at counting backwarts. Maybe it is possible to change the code into something like

while ( cnt++ < idc->vnic_wait_limit)

this way you have a flexible limit, and it is better to understand for others what you want to archive.

just my 2 cents,
re,
wh

Quoted 7 lines

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html