[PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[Hao Lan]

From: Jie Wang <wangjie125@huawei.com>

Currently, it used old rss size to get current tc mode. As a result, the
rss size is updated, but the tc mode is still configured based on the old
rss size.

So this patch fixes it by using the new rss size in both process.

Fixes: 93969dc14fcd ("net: hns3: refactor VF rss init APIs with new common rss init APIs")
Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Hao Lan <lanhao@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 081bd2c3f289..e84e5be8e59e 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -3130,7 +3130,7 @@ static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
 
 	hclgevf_update_rss_size(handle, new_tqps_num);
 
-	hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
+	hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,
 				   tc_offset, tc_valid, tc_size);
 	ret = hclge_comm_set_rss_tc_mode(&hdev->hw.hw, tc_offset,
 					 tc_valid, tc_size);
-- 
2.30.0

Re: [PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[Alexander H Duyck]

On Tue, 2023-01-10 at 19:53 +0800, Hao Lan wrote:
> From: Jie Wang <wangjie125@huawei.com>
> 
> Currently, it used old rss size to get current tc mode. As a result, the
> rss size is updated, but the tc mode is still configured based on the old
> rss size.
> 
> So this patch fixes it by using the new rss size in both process.
> 
> Fixes: 93969dc14fcd ("net: hns3: refactor VF rss init APIs with new common rss init APIs")
> Signed-off-by: Jie Wang <wangjie125@huawei.com>
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> index 081bd2c3f289..e84e5be8e59e 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> @@ -3130,7 +3130,7 @@ static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
>  
>  	hclgevf_update_rss_size(handle, new_tqps_num);
>  
> -	hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
> +	hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,
>  				   tc_offset, tc_valid, tc_size);
>  	ret = hclge_comm_set_rss_tc_mode(&hdev->hw.hw, tc_offset,
>  					 tc_valid, tc_size);

I can see how this was confused. It isn't really clear that handle is
being used to update the kinfo->rss_size value. Maybe think about
adding a comment to prevent someone from reverting this without
realizing that? It might also be useful to do a follow-on patch for
net-next that renames cur_rss_size to orig_rss_size to make it more
obvious that the value is changing.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[Hao Lan]

On 2023/1/12 0:31, Alexander H Duyck wrote:
> On Tue, 2023-01-10 at 19:53 +0800, Hao Lan wrote:
>> From: Jie Wang <wangjie125@huawei.com>
>>
>> Currently, it used old rss size to get current tc mode. As a result, the
>> rss size is updated, but the tc mode is still configured based on the old
>> rss size.
>>
>> So this patch fixes it by using the new rss size in both process.
>>
>> Fixes: 93969dc14fcd ("net: hns3: refactor VF rss init APIs with new common rss init APIs")
>> Signed-off-by: Jie Wang <wangjie125@huawei.com>
>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>> ---
>>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> index 081bd2c3f289..e84e5be8e59e 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> @@ -3130,7 +3130,7 @@ static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
>>  
>>  	hclgevf_update_rss_size(handle, new_tqps_num);
>>  
>> -	hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
>> +	hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,
>>  				   tc_offset, tc_valid, tc_size);
>>  	ret = hclge_comm_set_rss_tc_mode(&hdev->hw.hw, tc_offset,
>>  					 tc_valid, tc_size);
> 
> I can see how this was confused. It isn't really clear that handle is
> being used to update the kinfo->rss_size value. Maybe think about
> adding a comment to prevent someone from reverting this without
> realizing that? It might also be useful to do a follow-on patch for
> net-next that renames cur_rss_size to orig_rss_size to make it more
> obvious that the value is changing.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
> .
> 
Hi Alexander Duyck,
Thank you for your reviewed.And thank you for your valuable advice.
Would it be better if we changed it to the following.

-	u16 cur_rss_size = kinfo->rss_size;
-	u16 cur_tqps = kinfo->num_tqps;
+	u16 orig_rss_size = kinfo->rss_size;
+	u16 orig_tqps = kinfo->num_tqps;
 	u32 *rss_indir;
 	unsigned int i;
 	int ret;

 	hclgevf_update_rss_size(handle, new_tqps_num);

-	hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
+	/* RSS size will be updated after hclgevf_update_rss_size,
+	 * so we use kinfo->rss_size instead of orig_rss_size.
+	 */
+	hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,

Re: [PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 10 Jan 2023 19:53:59 +0800 you wrote:
> From: Jie Wang <wangjie125@huawei.com>
> 
> Currently, it used old rss size to get current tc mode. As a result, the
> rss size is updated, but the tc mode is still configured based on the old
> rss size.
> 
> So this patch fixes it by using the new rss size in both process.
> 
> [...]

Here is the summary with links:
  - [net] net: hns3: fix wrong use of rss size during VF rss config
    https://git.kernel.org/netdev/net/c/ae9f29fdfd82

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: hns3: fix wrong use of rss size during VF rss config

[Alexander Duyck]

On Wed, Jan 11, 2023 at 6:56 PM Hao Lan <lanhao@huawei.com> wrote:
>
>
>
> On 2023/1/12 0:31, Alexander H Duyck wrote:
> > On Tue, 2023-01-10 at 19:53 +0800, Hao Lan wrote:
> >> From: Jie Wang <wangjie125@huawei.com>
> >>
> >> Currently, it used old rss size to get current tc mode. As a result, the
> >> rss size is updated, but the tc mode is still configured based on the old
> >> rss size.
> >>
> >> So this patch fixes it by using the new rss size in both process.
> >>
> >> Fixes: 93969dc14fcd ("net: hns3: refactor VF rss init APIs with new common rss init APIs")
> >> Signed-off-by: Jie Wang <wangjie125@huawei.com>
> >> Signed-off-by: Hao Lan <lanhao@huawei.com>
> >> ---
> >>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> index 081bd2c3f289..e84e5be8e59e 100644
> >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> @@ -3130,7 +3130,7 @@ static int hclgevf_set_channels(struct hnae3_handle *handle, u32 new_tqps_num,
> >>
> >>      hclgevf_update_rss_size(handle, new_tqps_num);
> >>
> >> -    hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
> >> +    hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,
> >>                                 tc_offset, tc_valid, tc_size);
> >>      ret = hclge_comm_set_rss_tc_mode(&hdev->hw.hw, tc_offset,
> >>                                       tc_valid, tc_size);
> >
> > I can see how this was confused. It isn't really clear that handle is
> > being used to update the kinfo->rss_size value. Maybe think about
> > adding a comment to prevent someone from reverting this without
> > realizing that? It might also be useful to do a follow-on patch for
> > net-next that renames cur_rss_size to orig_rss_size to make it more
> > obvious that the value is changing.
> >
> > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
> > .
> >
> Hi Alexander Duyck,
> Thank you for your reviewed.And thank you for your valuable advice.
> Would it be better if we changed it to the following.
>
> -       u16 cur_rss_size = kinfo->rss_size;
> -       u16 cur_tqps = kinfo->num_tqps;
> +       u16 orig_rss_size = kinfo->rss_size;
> +       u16 orig_tqps = kinfo->num_tqps;
>         u32 *rss_indir;
>         unsigned int i;
>         int ret;
>
>         hclgevf_update_rss_size(handle, new_tqps_num);
>
> -       hclge_comm_get_rss_tc_info(cur_rss_size, hdev->hw_tc_map,
> +       /* RSS size will be updated after hclgevf_update_rss_size,
> +        * so we use kinfo->rss_size instead of orig_rss_size.
> +        */
> +       hclge_comm_get_rss_tc_info(kinfo->rss_size, hdev->hw_tc_map,

Yes, something like this would make it more obvious that these fields
are changing.