diff mbox series

net/txgbe: fix a bit with boolean operator

Message ID tencent_D18B07E35425224027E35B6E6441A439E605@qq.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/txgbe: fix a bit with boolean operator |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Weiguo Li March 1, 2022, 6:08 a.m. UTC 
  Since boolean value is in 0 and 1, it's strange to combines a boolean
value with a bit operator.

Thus it's highly possible a typo error with "if (A & !B)", and more
probably to use "if (A & ~B)" instead.

Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")

Signed-off-by: Weiguo Li <liwg06@foxmail.com>
---
 drivers/net/txgbe/txgbe_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jiawen Wu March 2, 2022, 8:02 a.m. UTC | #1 
On March 1, 2022 2:09 PM, Weiguo Li wrote:
> Since boolean value is in 0 and 1, it's strange to combines a boolean
value with
> a bit operator.
> 
> Thus it's highly possible a typo error with "if (A & !B)", and more
probably to
> use "if (A & ~B)" instead.
> 
> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
> 
> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
> ---
>  drivers/net/txgbe/txgbe_ethdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/txgbe/txgbe_ethdev.c
> b/drivers/net/txgbe/txgbe_ethdev.c
> index 19d4444748..f0994f028d 100644
> --- a/drivers/net/txgbe/txgbe_ethdev.c
> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> @@ -376,7 +376,7 @@ txgbe_dev_queue_stats_mapping_set(struct
> rte_eth_dev *eth_dev,
>  	if (hw->mac.type != txgbe_mac_raptor)
>  		return -ENOSYS;
> 
> -	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
> +	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
>  		return -EIO;
> 
>  	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat
> index %d",
> --
> 2.25.1

Thanks.

Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
Ferruh Yigit March 2, 2022, 5:23 p.m. UTC | #2 
On 3/2/2022 8:02 AM, Jiawen Wu wrote:
> On March 1, 2022 2:09 PM, Weiguo Li wrote:
>> Since boolean value is in 0 and 1, it's strange to combines a boolean
> value with
>> a bit operator.
>>
>> Thus it's highly possible a typo error with "if (A & !B)", and more
> probably to
>> use "if (A & ~B)" instead.
>>
>> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
>>
>> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
>> ---
>>   drivers/net/txgbe/txgbe_ethdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/txgbe/txgbe_ethdev.c
>> b/drivers/net/txgbe/txgbe_ethdev.c
>> index 19d4444748..f0994f028d 100644
>> --- a/drivers/net/txgbe/txgbe_ethdev.c
>> +++ b/drivers/net/txgbe/txgbe_ethdev.c
>> @@ -376,7 +376,7 @@ txgbe_dev_queue_stats_mapping_set(struct
>> rte_eth_dev *eth_dev,
>>   	if (hw->mac.type != txgbe_mac_raptor)
>>   		return -ENOSYS;
>>
>> -	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
>> +	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
>>   		return -EIO;
>>
>>   	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat
>> index %d",
>> --
>> 2.25.1
> 
> Thanks.
> 
> Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
> 

Hi Weiguo,

Good catch, I wonder how did you detect this?

If there is an automated way, maybe we can consider using
it in our CI.
Ferruh Yigit March 2, 2022, 5:34 p.m. UTC | #3 
On 3/2/2022 8:02 AM, Jiawen Wu wrote:
> On March 1, 2022 2:09 PM, Weiguo Li wrote:
>> Since boolean value is in 0 and 1, it's strange to combines a boolean
> value with
>> a bit operator.
>>
>> Thus it's highly possible a typo error with "if (A & !B)", and more
> probably to
>> use "if (A & ~B)" instead.
>>
>> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
>>
>> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
> 
> Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
> 

Applied to dpdk-next-net/main, thanks.
Weiguo Li March 3, 2022, 1:31 p.m. UTC | #4 
On March 2, 2022, 5:23 p.m Ferruh Yigit wrote:
> On 3/2/2022 8:02 AM, Jiawen Wu wrote:
> > On March 1, 2022 2:09 PM, Weiguo Li wrote:
> >> Since boolean value is in 0 and 1, it's strange to combines a boolean
> > value with
> >> a bit operator.
> >>
> >> Thus it's highly possible a typo error with "if (A & !B)", and more
> > probably to
> >> use "if (A & ~B)" instead.
> >>
> >> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
> >>
> >> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
> >> ---
> >>   drivers/net/txgbe/txgbe_ethdev.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/txgbe/txgbe_ethdev.c
> >> b/drivers/net/txgbe/txgbe_ethdev.c
> >> index 19d4444748..f0994f028d 100644
> >> --- a/drivers/net/txgbe/txgbe_ethdev.c
> >> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> >> @@ -376,7 +376,7 @@ txgbe_dev_queue_stats_mapping_set(struct
> >> rte_eth_dev *eth_dev,
> >>   	if (hw->mac.type != txgbe_mac_raptor)
> >>   		return -ENOSYS;
> >>
> >> -	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
> >> +	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
> >>   		return -EIO;
> >>
> >>   	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat
> >> index %d",
> >> --
> >> 2.25.1
> > 
> > Thanks.
> > 
> > Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > 
> 
> Hi Weiguo,
> 
> Good catch, I wonder how did you detect this?
> 
> If there is an automated way, maybe we can consider using
> it in our CI.
> 

Hi Ferruh,

It's found by a coccinell script:

@fix_bit_boolean @ expression E; constant C; @@
(
  !E & !C
|
- !E & C
+ !(E & C)
|
- E & !C
+ E & ~C
)

the idea came from a demo script in coccinelle website:
(https://coccinelle.gitlabpages.inria.fr/website/rules/notand.html)

@@ expression E; constant C; @@
(
  !E & !C
|
- !E & C
+ !(E & C)
)

The difference is in the last two lines and by which found the problem.

I'd be happy if it would used in CI.
Maybe better to let Julia know, since she's the original author.
cc Julia

-Weiguo
Ferruh Yigit March 3, 2022, 3:18 p.m. UTC | #5 
On 3/3/2022 1:31 PM, Weiguo Li wrote:
> On March 2, 2022, 5:23 p.m Ferruh Yigit wrote:
>> On 3/2/2022 8:02 AM, Jiawen Wu wrote:
>>> On March 1, 2022 2:09 PM, Weiguo Li wrote:
>>>> Since boolean value is in 0 and 1, it's strange to combines a boolean
>>> value with
>>>> a bit operator.
>>>>
>>>> Thus it's highly possible a typo error with "if (A & !B)", and more
>>> probably to
>>>> use "if (A & ~B)" instead.
>>>>
>>>> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
>>>>
>>>> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
>>>> ---
>>>>    drivers/net/txgbe/txgbe_ethdev.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/txgbe/txgbe_ethdev.c
>>>> b/drivers/net/txgbe/txgbe_ethdev.c
>>>> index 19d4444748..f0994f028d 100644
>>>> --- a/drivers/net/txgbe/txgbe_ethdev.c
>>>> +++ b/drivers/net/txgbe/txgbe_ethdev.c
>>>> @@ -376,7 +376,7 @@ txgbe_dev_queue_stats_mapping_set(struct
>>>> rte_eth_dev *eth_dev,
>>>>    	if (hw->mac.type != txgbe_mac_raptor)
>>>>    		return -ENOSYS;
>>>>
>>>> -	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
>>>> +	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
>>>>    		return -EIO;
>>>>
>>>>    	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat
>>>> index %d",
>>>> --
>>>> 2.25.1
>>>
>>> Thanks.
>>>
>>> Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>>
>>
>> Hi Weiguo,
>>
>> Good catch, I wonder how did you detect this?
>>
>> If there is an automated way, maybe we can consider using
>> it in our CI.
>>
> 
> Hi Ferruh,
> 
> It's found by a coccinell script:
> 
> @fix_bit_boolean @ expression E; constant C; @@
> (
>    !E & !C
> |
> - !E & C
> + !(E & C)
> |
> - E & !C
> + E & ~C
> )
> 
> the idea came from a demo script in coccinelle website:
> (https://coccinelle.gitlabpages.inria.fr/website/rules/notand.html)
> 
> @@ expression E; constant C; @@
> (
>    !E & !C
> |
> - !E & C
> + !(E & C)
> )
> 
> The difference is in the last two lines and by which found the problem.
> 
> I'd be happy if it would used in CI.
> Maybe better to let Julia know, since she's the original author.
> cc Julia
> 

Thanks Weiguo. cc'ed lab people and Ali too.

Aaron, lab, what do you think, can this be added into CI checks?
Stephen Hemminger March 3, 2022, 4:16 p.m. UTC | #6 
On Thu,  3 Mar 2022 21:31:00 +0800
Weiguo Li <liwg06@foxmail.com> wrote:

> On March 2, 2022, 5:23 p.m Ferruh Yigit wrote:
> > On 3/2/2022 8:02 AM, Jiawen Wu wrote:  
> > > On March 1, 2022 2:09 PM, Weiguo Li wrote:  
> > >> Since boolean value is in 0 and 1, it's strange to combines a boolean  
> > > value with  
> > >> a bit operator.
> > >>
> > >> Thus it's highly possible a typo error with "if (A & !B)", and more  
> > > probably to  
> > >> use "if (A & ~B)" instead.
> > >>
> > >> Fixes: c1d4e9d37abdc6 ("net/txgbe: add queue stats mapping")
> > >>
> > >> Signed-off-by: Weiguo Li <liwg06@foxmail.com>
> > >> ---
> > >>   drivers/net/txgbe/txgbe_ethdev.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/net/txgbe/txgbe_ethdev.c
> > >> b/drivers/net/txgbe/txgbe_ethdev.c
> > >> index 19d4444748..f0994f028d 100644
> > >> --- a/drivers/net/txgbe/txgbe_ethdev.c
> > >> +++ b/drivers/net/txgbe/txgbe_ethdev.c
> > >> @@ -376,7 +376,7 @@ txgbe_dev_queue_stats_mapping_set(struct
> > >> rte_eth_dev *eth_dev,
> > >>   	if (hw->mac.type != txgbe_mac_raptor)
> > >>   		return -ENOSYS;
> > >>
> > >> -	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
> > >> +	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
> > >>   		return -EIO;
> > >>
> > >>   	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat
> > >> index %d",
> > >> --
> > >> 2.25.1  
> > > 
> > > Thanks.
> > > 
> > > Acked-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > >   
> > 
> > Hi Weiguo,
> > 
> > Good catch, I wonder how did you detect this?
> > 
> > If there is an automated way, maybe we can consider using
> > it in our CI.
> >   
> 
> Hi Ferruh,
> 
> It's found by a coccinell script:
> 
> @fix_bit_boolean @ expression E; constant C; @@
> (
>   !E & !C
> |
> - !E & C
> + !(E & C)
> |
> - E & !C
> + E & ~C
> )
> 
> the idea came from a demo script in coccinelle website:
> (https://coccinelle.gitlabpages.inria.fr/website/rules/notand.html)
> 
> @@ expression E; constant C; @@
> (
>   !E & !C
> |
> - !E & C
> + !(E & C)
> )
> 
> The difference is in the last two lines and by which found the problem.
> 
> I'd be happy if it would used in CI.
> Maybe better to let Julia know, since she's the original author.
> cc Julia
> 
> -Weiguo
> 
> 

Could you add similar script to DPDK cocci scripts please?
diff mbox series

Patch

diff --git a/drivers/net/txgbe/txgbe_ethdev.c b/drivers/net/txgbe/txgbe_ethdev.c
index 19d4444748..f0994f028d 100644
--- a/drivers/net/txgbe/txgbe_ethdev.c
+++ b/drivers/net/txgbe/txgbe_ethdev.c
@@ -376,7 +376,7 @@  txgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev,
 	if (hw->mac.type != txgbe_mac_raptor)
 		return -ENOSYS;
 
-	if (stat_idx & !QMAP_FIELD_RESERVED_BITS_MASK)
+	if (stat_idx & ~QMAP_FIELD_RESERVED_BITS_MASK)
 		return -EIO;
 
 	PMD_INIT_LOG(DEBUG, "Setting port %d, %s queue_id %d to stat index %d",