[patch] qlcnic: fix a timeout loop

[patch] qlcnic: fix a timeout loop

[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
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 <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
index be7d7a6..9919245 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
@@ -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);
 	}

RE: [patch] qlcnic: fix a timeout loop

[Manish Chopra]

> -----Original Message-----
> From: dept_hsg_linux_nic_dev-bounces@qlclistserver.qlogic.com
> [mailto:dept_hsg_linux_nic_dev-bounces@qlclistserver.qlogic.com] On Behalf
> Of Dan Carpenter
> Sent: Tuesday, December 15, 2015 3:46 PM
> To: Dept-GE Linux NIC Dev <Dept-GELinuxNICDev@qlogic.com>; Rajesh
> Borundia <rajesh.borundia@qlogic.com>
> Cc: netdev <netdev@vger.kernel.org>; kernel-janitors@vger.kernel.org
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> index be7d7a6..9919245 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> @@ -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);
>  	}

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 
 

[patch v2] qlcnic: fix a timeout loop

[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 <dan.carpenter@oracle.com>
---
v2: different color on the bikeshed

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
index be7d7a6..b1a452f 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
@@ -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);
 	}

Re: [patch] qlcnic: fix a timeout loop

[walter harms]

Am 15.12.2015 14:46, schrieb Manish Chopra:
>> -----Original Message-----
>> From: dept_hsg_linux_nic_dev-bounces@qlclistserver.qlogic.com
>> [mailto:dept_hsg_linux_nic_dev-bounces@qlclistserver.qlogic.com] On Behalf
>> Of Dan Carpenter
>> Sent: Tuesday, December 15, 2015 3:46 PM
>> To: Dept-GE Linux NIC Dev <Dept-GELinuxNICDev@qlogic.com>; Rajesh
>> Borundia <rajesh.borundia@qlogic.com>
>> Cc: netdev <netdev@vger.kernel.org>; kernel-janitors@vger.kernel.org
>> 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 <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> index be7d7a6..9919245 100644
>> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
>> @@ -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);
>>  	}
> 
> 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 


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






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

Re: [patch v2] qlcnic: fix a timeout loop

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 15 Dec 2015 16:56:16 +0300

> 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 <dan.carpenter@oracle.com>

Applied.

Re: [patch v2] qlcnic: fix a timeout loop

[Dan Carpenter]

On Tue, Dec 15, 2015 at 01:13:41PM -0500, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Tue, 15 Dec 2015 16:56:16 +0300
> 
> > 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 <dan.carpenter@oracle.com>
> 
> 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

Re: [patch v2] qlcnic: fix a timeout loop

[Dan Carpenter]

On Tue, Dec 15, 2015 at 11:13:50PM +0300, Dan Carpenter wrote:
> On Tue, Dec 15, 2015 at 01:13:41PM -0500, David Miller wrote:
> > From: Dan Carpenter <dan.carpenter@oracle.com>
> > Date: Tue, 15 Dec 2015 16:56:16 +0300
> > 
> > > 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 <dan.carpenter@oracle.com>
> > 
> > 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

[patch] qlcnic: fix a loop exit condition better

[Dan Carpenter]

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 <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
index b1a452f..3490675 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
@@ -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;

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

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 24 Dec 2015 12:21:22 +0300

> 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 <dan.carpenter@oracle.com>

Applied, thanks Dan.

Re: [patch v2] qlcnic: fix a timeout loop

[Yann Dupont - Veille Techno]

Le 15/12/2015 14:56, Dan Carpenter a écrit :
> 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 <dan.carpenter@oracle.com>
> ---
> v2: different color on the bikeshed
>
> diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> index be7d7a6..b1a452f 100644
> --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_vnic.c
> @@ -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);
>   	}
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> 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.

Re: [patch v2] qlcnic: fix a timeout loop

[Dan Carpenter]

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