| Message ID | 20230110115359.10163-1-lanhao@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ae9f29fdfd827ad06c1ae8155c042245a9d00757 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: hns3: fix wrong use of rss size during VF rss config | expand |
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>
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,
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!
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.
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);