| Message ID | 1471932979-17864-1-git-send-email-a.hajda@samsung.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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: >> 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: > > 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 > > > > > 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.
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)
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 <a.hajda@samsung.com> --- 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(-)