diff mbox series

[05/12] net/hns3: fix an unreasonable memset

Message ID 20220519122917.2334-6-humin29@huawei.com (mailing list archive)
State Changes Requested, archived
Delegated to: Andrew Rybchenko
Headers
Series bugfix for hns3 PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

humin (Q) May 19, 2022, 12:29 p.m. UTC 
  From: Huisong Li <lihuisong@huawei.com>

This patch fixes an unreasonable memset.

Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 drivers/net/hns3/hns3_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Rybchenko May 20, 2022, 7:58 a.m. UTC | #1 
On 5/19/22 15:29, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> This patch fixes an unreasonable memset.
> 
> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 510802be05..5a2cfe5a54 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>   	int ret;
>   	uint16_t i;
>   
> -	memset(msg_data, 0, sizeof(uint16_t));
> +	memset(msg_data, 0, sizeof(msg_data));

It looks a bit suspicious. May be it is better to do:
     memset(&msg_data, 0, sizeof(msg_data));
It is pretty common mistake to do:
     memset(p, 0, sizeof(p));
instead of
     memset(p, 0, sizeof(*p));
when p is a pointer.
I realize that msg_data is an array in this case, but
I think it is better to avoid bad pattern in the code.

>   	ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>   				sizeof(msg_data), true, &reset_status,
>   				sizeof(reset_status));
humin (Q) May 20, 2022, 8:37 a.m. UTC | #2 
Hi, Andrew ,

在 2022/5/20 15:58, Andrew Rybchenko 写道:
> On 5/19/22 15:29, Min Hu (Connor) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> This patch fixes an unreasonable memset.
>>
>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>> index 510802be05..5a2cfe5a54 100644
>> --- a/drivers/net/hns3/hns3_rxtx.c
>> +++ b/drivers/net/hns3/hns3_rxtx.c
>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>       int ret;
>>       uint16_t i;
>> -    memset(msg_data, 0, sizeof(uint16_t));
>> +    memset(msg_data, 0, sizeof(msg_data));
> 
> It looks a bit suspicious. May be it is better to do:
>      memset(&msg_data, 0, sizeof(msg_data));
I think this is too hard to understand for &msg_data is **p.
maybe it confuse others when use memset to operate level-2 pointer.


> It is pretty common mistake to do:
>      memset(p, 0, sizeof(p));
> instead of
>      memset(p, 0, sizeof(*p));
> when p is a pointer.
> I realize that msg_data is an array in this case, but
> I think it is better to avoid bad pattern in the code.
> 
>>       ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>>                   sizeof(msg_data), true, &reset_status,
>>                   sizeof(reset_status));
> 
> .
Andrew Rybchenko May 20, 2022, 8:59 a.m. UTC | #3 
On 5/20/22 11:37, Min Hu (Connor) wrote:
> Hi, Andrew ,
> 
> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> This patch fixes an unreasonable memset.
>>>
>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> ---
>>>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
>>> index 510802be05..5a2cfe5a54 100644
>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>       int ret;
>>>       uint16_t i;
>>> -    memset(msg_data, 0, sizeof(uint16_t));
>>> +    memset(msg_data, 0, sizeof(msg_data));
>>
>> It looks a bit suspicious. May be it is better to do:
>>      memset(&msg_data, 0, sizeof(msg_data));
> I think this is too hard to understand for &msg_data is **p.
> maybe it confuse others when use memset to operate level-2 pointer.

msg_data == &amsg_data
since it is an array

> 
> 
>> It is pretty common mistake to do:
>>      memset(p, 0, sizeof(p));
>> instead of
>>      memset(p, 0, sizeof(*p));
>> when p is a pointer.
>> I realize that msg_data is an array in this case, but
>> I think it is better to avoid bad pattern in the code.
>>
>>>       ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>>>                   sizeof(msg_data), true, &reset_status,
>>>                   sizeof(reset_status));
>>
>> .
humin (Q) May 20, 2022, 9:14 a.m. UTC | #4 
Hi,Andrew,

在 2022/5/20 16:59, Andrew Rybchenko 写道:
> On 5/20/22 11:37, Min Hu (Connor) wrote:
>> Hi, Andrew ,
>>
>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> This patch fixes an unreasonable memset.
>>>>
>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c 
>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>> index 510802be05..5a2cfe5a54 100644
>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>>       int ret;
>>>>       uint16_t i;
>>>> -    memset(msg_data, 0, sizeof(uint16_t));
>>>> +    memset(msg_data, 0, sizeof(msg_data));
>>>
>>> It looks a bit suspicious. May be it is better to do:
>>>      memset(&msg_data, 0, sizeof(msg_data));
>> I think this is too hard to understand for &msg_data is **p.
>> maybe it confuse others when use memset to operate level-2 pointer.
> 
> msg_data == &amsg_data
> since it is an array
Yes, you are right, the address of 'msg_data ' is the same
as the address of '&amsg_data'.

But I still think this usage is not common.
like the current code in DPDK:
*****
static int
app_parse_flow_conf(const char *conf_str)
{
	int ret;
	uint32_t vals[5];
	struct flow_conf *pconf;
	uint64_t mask;

	memset(vals, 0, sizeof(vals));
...
*****

how about others's opinion? @Ferruh, @Thomas.

> 
>>
>>
>>> It is pretty common mistake to do:
>>>      memset(p, 0, sizeof(p));
>>> instead of
>>>      memset(p, 0, sizeof(*p));
>>> when p is a pointer.
>>> I realize that msg_data is an array in this case, but
>>> I think it is better to avoid bad pattern in the code.
>>>
>>>>       ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
>>>>                   sizeof(msg_data), true, &reset_status,
>>>>                   sizeof(reset_status));
>>>
>>> .
> 
> .
Ferruh Yigit May 20, 2022, 10:43 a.m. UTC | #5 
On 5/20/2022 10:14 AM, Min Hu (Connor) wrote:
> [CAUTION: External Email]
> 
> Hi,Andrew,
> 
> 在 2022/5/20 16:59, Andrew Rybchenko 写道:
>> On 5/20/22 11:37, Min Hu (Connor) wrote:
>>> Hi, Andrew ,
>>>
>>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>
>>>>> This patch fixes an unreasonable memset.
>>>>>
>>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>> ---
>>>>>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c
>>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>>> index 510802be05..5a2cfe5a54 100644
>>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>>>       int ret;
>>>>>       uint16_t i;
>>>>> -    memset(msg_data, 0, sizeof(uint16_t));
>>>>> +    memset(msg_data, 0, sizeof(msg_data));
>>>>
>>>> It looks a bit suspicious. May be it is better to do:
>>>>      memset(&msg_data, 0, sizeof(msg_data));
>>> I think this is too hard to understand for &msg_data is **p.
>>> maybe it confuse others when use memset to operate level-2 pointer.
>>
>> msg_data == &amsg_data
>> since it is an array
> Yes, you are right, the address of 'msg_data ' is the same
> as the address of '&amsg_data'.
> 
> But I still think this usage is not common.
> like the current code in DPDK:
> *****
> static int
> app_parse_flow_conf(const char *conf_str)
> {
>         int ret;
>         uint32_t vals[5];
>         struct flow_conf *pconf;
>         uint64_t mask;
> 
>         memset(vals, 0, sizeof(vals));
> ...
> *****
> 
> how about others's opinion? @Ferruh, @Thomas.
> 

I would use array without address operator, so original patch lgtm.

+1 to main purpose of the patch (to replace 'sizeof(uint16_t)').
humin (Q) May 20, 2022, 11 a.m. UTC | #6 
Hi,

在 2022/5/20 18:43, Ferruh Yigit 写道:
> On 5/20/2022 10:14 AM, Min Hu (Connor) wrote:
>> [CAUTION: External Email]
>>
>> Hi,Andrew,
>>
>> 在 2022/5/20 16:59, Andrew Rybchenko 写道:
>>> On 5/20/22 11:37, Min Hu (Connor) wrote:
>>>> Hi, Andrew ,
>>>>
>>>> 在 2022/5/20 15:58, Andrew Rybchenko 写道:
>>>>> On 5/19/22 15:29, Min Hu (Connor) wrote:
>>>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>>>
>>>>>> This patch fixes an unreasonable memset.
>>>>>>
>>>>>> Fixes: bba636698316 ("net/hns3: support Rx/Tx and related 
>>>>>> operations")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>>>> ---
>>>>>>   drivers/net/hns3/hns3_rxtx.c | 2 +-
>>>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/hns3/hns3_rxtx.c
>>>>>> b/drivers/net/hns3/hns3_rxtx.c
>>>>>> index 510802be05..5a2cfe5a54 100644
>>>>>> --- a/drivers/net/hns3/hns3_rxtx.c
>>>>>> +++ b/drivers/net/hns3/hns3_rxtx.c
>>>>>> @@ -776,7 +776,7 @@ hns3vf_reset_all_tqps(struct hns3_hw *hw)
>>>>>>       int ret;
>>>>>>       uint16_t i;
>>>>>> -    memset(msg_data, 0, sizeof(uint16_t));
>>>>>> +    memset(msg_data, 0, sizeof(msg_data));
>>>>>
>>>>> It looks a bit suspicious. May be it is better to do:
>>>>>      memset(&msg_data, 0, sizeof(msg_data));
>>>> I think this is too hard to understand for &msg_data is **p.
>>>> maybe it confuse others when use memset to operate level-2 pointer.
>>>
>>> msg_data == &amsg_data
>>> since it is an array
>> Yes, you are right, the address of 'msg_data ' is the same
>> as the address of '&amsg_data'.
>>
>> But I still think this usage is not common.
>> like the current code in DPDK:
>> *****
>> static int
>> app_parse_flow_conf(const char *conf_str)
>> {
>>         int ret;
>>         uint32_t vals[5];
>>         struct flow_conf *pconf;
>>         uint64_t mask;
>>
>>         memset(vals, 0, sizeof(vals));
>> ...
>> *****
>>
>> how about others's opinion? @Ferruh, @Thomas.
>>
> 
> I would use array without address operator, so original patch lgtm.
thanks Ferruh.
> 
> +1 to main purpose of the patch (to replace 'sizeof(uint16_t)').
> .
diff mbox series

Patch

diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
index 510802be05..5a2cfe5a54 100644
--- a/drivers/net/hns3/hns3_rxtx.c
+++ b/drivers/net/hns3/hns3_rxtx.c
@@ -776,7 +776,7 @@  hns3vf_reset_all_tqps(struct hns3_hw *hw)
 	int ret;
 	uint16_t i;
 
-	memset(msg_data, 0, sizeof(uint16_t));
+	memset(msg_data, 0, sizeof(msg_data));
 	ret = hns3_send_mbx_msg(hw, HNS3_MBX_QUEUE_RESET, 0, msg_data,
 				sizeof(msg_data), true, &reset_status,
 				sizeof(reset_status));