[PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs

Local variable msi_idx defined as unsigned int is always >= 0, thus both
'if' checks are always true. On the other side presence of USING_MSIX flag
suggests the checks should not be trivially true.
The simplest solution is to replace incorrect checks with direct testing
of adap->flags and remove spare variables.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

Signed-off-by: Andrzej Hajda <redacted>
---
Hi,

I am not familiar with the code, and it is not clear to me, so my solution
can be incorrect, anyway there is an issue here.

Regards
Andrzej
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c
index aac6e44..1e0c952 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c

@@ -121,20 +121,14 @@ static int alloc_uld_rxqs(struct adapter *adap,
 			  struct sge_uld_rxq_info *rxq_info,
 			  unsigned int nq, unsigned int offset, bool lro)
 {
-	struct sge *s = &adap->sge;
 	struct sge_ofld_rxq *q = rxq_info->uldrxq + offset;
 	unsigned short *ids = rxq_info->rspq_id + offset;
 	unsigned int per_chan = nq / adap->params.nports;
-	unsigned int msi_idx, bmap_idx;
+	unsigned int bmap_idx;
 	int i, err;
 
-	if (adap->flags & USING_MSIX)
-		msi_idx = 1;
-	else
-		msi_idx = -((int)s->intrq.abs_id + 1);
-
 	for (i = 0; i < nq; i++, q++) {
-		if (msi_idx >= 0) {
+		if (adap->flags & USING_MSIX) {
 			bmap_idx = get_msix_idx_from_bmap(adap);
 			adap->msi_idx++;
 		}

@@ -147,7 +141,7 @@ static int alloc_uld_rxqs(struct adapter *adap,
 				       0);
 		if (err)
 			goto freeout;
-		if (msi_idx >= 0)
+		if (adap->flags & USING_MSIX)
 			rxq_info->msix_tbl[i + offset] = bmap_idx;
 		memset(&q->stats, 0, sizeof(q->stats));
 		if (ids)

-- 
1.9.1

On Tuesday, August 08/23/16, 2016 at 08:16:19 +0200, Andrzej Hajda wrote:

Local variable msi_idx defined as unsigned int is always >= 0, thus both
'if' checks are always true. On the other side presence of USING_MSIX flag
suggests the checks should not be trivially true.
The simplest solution is to replace incorrect checks with direct testing
of adap->flags and remove spare variables.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

The correct fix is to have local variable 'msi_idx' as int instead of unsigned
int. Thanks for reporting the issue. Do you want me to send a V2?

Thanks,
Hari

On 08/23/2016 09:46 AM, Hariprasad Shenai wrote:

On Tuesday, August 08/23/16, 2016 at 08:16:19 +0200, Andrzej Hajda wrote:

quoted

Local variable msi_idx defined as unsigned int is always >= 0, thus both
'if' checks are always true. On the other side presence of USING_MSIX flag
suggests the checks should not be trivially true.
The simplest solution is to replace incorrect checks with direct testing
of adap->flags and remove spare variables.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

The correct fix is to have local variable 'msi_idx' as int instead of unsigned
int. Thanks for reporting the issue. Do you want me to send a V2?

Thanks,
Hari

If adap->flags is constant during the call I see no point in creating
separate

variable with complicated initialization used only for the same thing as

'adap->flags & USING_MSIX', and even if adap->flags changes during the call

much simpler would be to use local var:

    int using_msix = adap->flags & USING_MSIX;

and later use tests:

    if (using_msix)

        ...

Am I correct?


Regards

Andrzej

On Tuesday, August 08/23/16, 2016 at 10:01:59 +0200, Andrzej Hajda wrote:

On 08/23/2016 09:46 AM, Hariprasad Shenai wrote:

quoted

On Tuesday, August 08/23/16, 2016 at 08:16:19 +0200, Andrzej Hajda wrote:

quoted

Local variable msi_idx defined as unsigned int is always >= 0, thus both
'if' checks are always true. On the other side presence of USING_MSIX flag
suggests the checks should not be trivially true.
The simplest solution is to replace incorrect checks with direct testing
of adap->flags and remove spare variables.

The problem has been detected using semantic patch
scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci

The correct fix is to have local variable 'msi_idx' as int instead of unsigned
int. Thanks for reporting the issue. Do you want me to send a V2?

Thanks,
Hari

If adap->flags is constant during the call I see no point in creating
separate

variable with complicated initialization used only for the same thing as

'adap->flags & USING_MSIX', and even if adap->flags changes during the call

much simpler would be to use local var:

    int using_msix = adap->flags & USING_MSIX;

and later use tests:

    if (using_msix)

        ...

Am I correct?

Yes, you are correct.