* [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs
@ 2016-08-23 6:16 Andrzej Hajda
2016-08-23 7:46 ` Hariprasad Shenai
0 siblings, 1 reply; 4+ messages in thread
From: Andrzej Hajda @ 2016-08-23 6:16 UTC (permalink / raw)
To: Hariprasad S, open list:CXGB4 ETHERNET DRIVER (CXGB4)
Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski, open list
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs
2016-08-23 6:16 [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs Andrzej Hajda
@ 2016-08-23 7:46 ` Hariprasad Shenai
2016-08-23 8:01 ` Andrzej Hajda
0 siblings, 1 reply; 4+ messages in thread
From: Hariprasad Shenai @ 2016-08-23 7:46 UTC (permalink / raw)
To: Andrzej Hajda
Cc: open list:CXGB4 ETHERNET DRIVER (CXGB4),
Bartlomiej Zolnierkiewicz, Marek Szyprowski, open list
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs
2016-08-23 7:46 ` Hariprasad Shenai
@ 2016-08-23 8:01 ` Andrzej Hajda
2016-08-23 8:08 ` Hariprasad Shenai
0 siblings, 1 reply; 4+ messages in thread
From: Andrzej Hajda @ 2016-08-23 8:01 UTC (permalink / raw)
To: Hariprasad Shenai
Cc: open list:CXGB4 ETHERNET DRIVER (CXGB4),
Bartlomiej Zolnierkiewicz, Marek Szyprowski, open list
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs
2016-08-23 8:01 ` Andrzej Hajda
@ 2016-08-23 8:08 ` Hariprasad Shenai
0 siblings, 0 replies; 4+ messages in thread
From: Hariprasad Shenai @ 2016-08-23 8:08 UTC (permalink / raw)
To: Andrzej Hajda
Cc: open list:CXGB4 ETHERNET DRIVER (CXGB4),
Bartlomiej Zolnierkiewicz, Marek Szyprowski, open list
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-23 8:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-23 6:16 [PATCH] cxgb4: fix invalid checks in alloc_uld_rxqs Andrzej Hajda
2016-08-23 7:46 ` Hariprasad Shenai
2016-08-23 8:01 ` Andrzej Hajda
2016-08-23 8:08 ` Hariprasad Shenai
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).