[PATCH net 0/3] There are some bugfix for the HNS3 ethernet driver

[PATCH net 0/3] There are some bugfix for the HNS3 ethernet driver

[Jijie Shao]

There are some bugfix for the HNS3 ethernet driver

Hao Lan (1):
  net: hns3: fix spelling mistake "reg_um" -> "reg_num"

Jian Shen (1):
  net: hns3: store rx VLAN tag offload state for VF

Yonglong Liu (1):
  net: hns3: fix a use of uninitialized variable problem

 .../hisilicon/hns3/hns3pf/hclge_main.c        |  2 +-
 .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 25 ++++++++++++-----
 .../hisilicon/hns3/hns3vf/hclgevf_main.h      |  3 ++-
 .../hisilicon/hns3/hns3vf/hclgevf_regs.c      | 27 ++++++++++---------
 4 files changed, 36 insertions(+), 21 deletions(-)

-- 
2.33.0

[PATCH net 1/3] net: hns3: fix a use of uninitialized variable problem

[Jijie Shao]

From: Yonglong Liu <liuyonglong@huawei.com>

In hclge_add_fd_entry(), if the flow type is FLOW_EXT, and the data of
m_ext is all zero, then some members of the local variable "info" are
not initialized.

Fixes: 67b0e1428e2f ("net: hns3: add support for user-def data of flow director")
Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 92f9b8ec76d9..f61355b03e14 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -6545,8 +6545,8 @@ static int hclge_add_fd_entry(struct hnae3_handle *handle,
 			      struct ethtool_rxnfc *cmd)
 {
 	struct hclge_vport *vport = hclge_get_vport(handle);
+	struct hclge_fd_user_def_info info = {0};
 	struct hclge_dev *hdev = vport->back;
-	struct hclge_fd_user_def_info info;
 	u16 dst_vport_id = 0, q_index = 0;
 	struct ethtool_rx_flow_spec *fs;
 	struct hclge_fd_rule *rule;
-- 
2.33.0

[PATCH net 2/3] net: hns3: fix spelling mistake "reg_um" -> "reg_num"

[Jijie Shao]

From: Hao Lan <lanhao@huawei.com>

There are spelling mistakes in hclgevf_get_regs. Fix them.

Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../hisilicon/hns3/hns3vf/hclgevf_regs.c      | 27 ++++++++++---------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
index 7d9d9dbc7560..9de01e344e27 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
@@ -127,37 +127,38 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
 
 	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
 	struct hnae3_queue *tqp;
-	int i, j, reg_um;
+	int i, j, reg_num;
 	u32 *reg = data;
 
 	*version = hdev->fw_version;
 	reg += hclgevf_reg_get_header(reg);
 
 	/* fetching per-VF registers values from VF PCIe register space */
-	reg_um = ARRAY_SIZE(cmdq_reg_addr_list);
-	reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_CMDQ, reg_um, reg);
-	for (i = 0; i < reg_um; i++)
+	reg_num = ARRAY_SIZE(cmdq_reg_addr_list);
+	reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_CMDQ, reg_num, reg);
+	for (i = 0; i < reg_num; i++)
 		*reg++ = hclgevf_read_dev(&hdev->hw, cmdq_reg_addr_list[i]);
 
-	reg_um = ARRAY_SIZE(common_reg_addr_list);
-	reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_COMMON, reg_um, reg);
-	for (i = 0; i < reg_um; i++)
+	reg_num = ARRAY_SIZE(common_reg_addr_list);
+	reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_COMMON, reg_num, reg);
+	for (i = 0; i < reg_num; i++)
 		*reg++ = hclgevf_read_dev(&hdev->hw, common_reg_addr_list[i]);
 
-	reg_um = ARRAY_SIZE(ring_reg_addr_list);
+	reg_num = ARRAY_SIZE(ring_reg_addr_list);
 	for (j = 0; j < hdev->num_tqps; j++) {
-		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_RING, reg_um, reg);
+		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_RING, reg_num, reg);
 		tqp = &hdev->htqp[j].q;
-		for (i = 0; i < reg_um; i++)
+		for (i = 0; i < reg_num; i++)
 			*reg++ = readl_relaxed(tqp->io_base -
 					       HCLGEVF_TQP_REG_OFFSET +
 					       ring_reg_addr_list[i]);
 	}
 
-	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
+	reg_num = ARRAY_SIZE(tqp_intr_reg_addr_list);
 	for (j = 0; j < hdev->num_msi_used - 1; j++) {
-		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_TQP_INTR, reg_um, reg);
-		for (i = 0; i < reg_um; i++)
+		reg += hclgevf_reg_get_tlv(HCLGEVF_REG_TAG_TQP_INTR,
+					   reg_num, reg);
+		for (i = 0; i < reg_num; i++)
 			*reg++ = hclgevf_read_dev(&hdev->hw,
 						  tqp_intr_reg_addr_list[i] +
 						  HCLGEVF_RING_INT_REG_OFFSET * j);
-- 
2.33.0

[PATCH net 3/3] net: hns3: store rx VLAN tag offload state for VF

[Jijie Shao]

From: Jian Shen <shenjian15@huawei.com>

The VF driver missed to store the rx VLAN tag strip state when
user change the rx VLAN tag offload state. And it will default
to enable the rx vlan tag strip when re-init VF device after
reset. So if user disable rx VLAN tag offload, and trig reset,
then the HW will still strip the VLAN tag from packet nad fill
into RX BD, but the VF driver will ignore it for rx VLAN tag
offload disabled. It may cause the rx VLAN tag dropped.

Fixes: b2641e2ad456 ("net: hns3: Add support of hardware rx-vlan-offload to HNS3 VF driver")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 25 ++++++++++++++-----
 .../hisilicon/hns3/hns3vf/hclgevf_main.h      |  3 ++-
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 9ba767740a04..dada42e7e0ec 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -1292,9 +1292,8 @@ static void hclgevf_sync_vlan_filter(struct hclgevf_dev *hdev)
 	rtnl_unlock();
 }
 
-static int hclgevf_en_hw_strip_rxvtag(struct hnae3_handle *handle, bool enable)
+static int hclgevf_en_hw_strip_rxvtag_cmd(struct hclgevf_dev *hdev, bool enable)
 {
-	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
 	struct hclge_vf_to_pf_msg send_msg;
 
 	hclgevf_build_send_msg(&send_msg, HCLGE_MBX_SET_VLAN,
@@ -1303,6 +1302,19 @@ static int hclgevf_en_hw_strip_rxvtag(struct hnae3_handle *handle, bool enable)
 	return hclgevf_send_mbx_msg(hdev, &send_msg, false, NULL, 0);
 }
 
+static int hclgevf_en_hw_strip_rxvtag(struct hnae3_handle *handle, bool enable)
+{
+	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
+	int ret;
+
+	ret = hclgevf_en_hw_strip_rxvtag_cmd(hdev, enable);
+	if (ret)
+		return ret;
+
+	hdev->rxvtag_strip_en = enable;
+	return 0;
+}
+
 static int hclgevf_reset_tqp(struct hnae3_handle *handle)
 {
 #define HCLGEVF_RESET_ALL_QUEUE_DONE	1U
@@ -2204,12 +2216,13 @@ static int hclgevf_rss_init_hw(struct hclgevf_dev *hdev)
 					  tc_valid, tc_size);
 }
 
-static int hclgevf_init_vlan_config(struct hclgevf_dev *hdev)
+static int hclgevf_init_vlan_config(struct hclgevf_dev *hdev,
+				    bool rxvtag_strip_en)
 {
 	struct hnae3_handle *nic = &hdev->nic;
 	int ret;
 
-	ret = hclgevf_en_hw_strip_rxvtag(nic, true);
+	ret = hclgevf_en_hw_strip_rxvtag(nic, rxvtag_strip_en);
 	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"failed to enable rx vlan offload, ret = %d\n", ret);
@@ -2879,7 +2892,7 @@ static int hclgevf_reset_hdev(struct hclgevf_dev *hdev)
 	if (ret)
 		return ret;
 
-	ret = hclgevf_init_vlan_config(hdev);
+	ret = hclgevf_init_vlan_config(hdev, hdev->rxvtag_strip_en);
 	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"failed(%d) to initialize VLAN config\n", ret);
@@ -2994,7 +3007,7 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
 		goto err_config;
 	}
 
-	ret = hclgevf_init_vlan_config(hdev);
+	ret = hclgevf_init_vlan_config(hdev, true);
 	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"failed(%d) to initialize VLAN config\n", ret);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
index cccef3228461..1e452b14b04e 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
@@ -252,7 +252,8 @@ struct hclgevf_dev {
 	u16 *vector_status;
 	int *vector_irq;
 
-	bool gro_en;
+	u32 gro_en :1;
+	u32 rxvtag_strip_en :1;
 
 	unsigned long vlan_del_fail_bmap[BITS_TO_LONGS(VLAN_N_VID)];
 
-- 
2.33.0

Re: [PATCH net 1/3] net: hns3: fix a use of uninitialized variable problem

[Simon Horman]

On Wed, Apr 02, 2025 at 08:09:59PM +0800, Jijie Shao wrote:
> From: Yonglong Liu <liuyonglong@huawei.com>
> 
> In hclge_add_fd_entry(), if the flow type is FLOW_EXT, and the data of
> m_ext is all zero, then some members of the local variable "info" are
> not initialized.

Hi,

I am assuming that this occurs when hclge_fd_check_spec() returns early
like this:

        if (!(fs->flow_type & FLOW_EXT) || hclge_fd_is_user_def_all_masked(fs))
                return 0;

But if so, should the description be "flow type is not FLOW_EXT, ..."
(note the "not")? Or perhaps more clearly refer to the FLOW_EXT bit
not being set?

Also, I think it would be worth mentioning hclge_fd_check_spec()
in the patch description, perhaps something like this.

  In hclge_add_fd_entry(), if  hclge_fd_check_spec() returns early
  because the FLOW_EXT bit is not set in the flow type, and ...

Also, does this manifest in a user-visible problem? If so, I think
it would be good to describe it in the patch description.

If not, I think it would be good to mention how the problem was found.
E.g.: "flagged by static analysis" (and mention the tool if it is publicly
Available. Or, "found by inspection".

> 
> Fixes: 67b0e1428e2f ("net: hns3: add support for user-def data of flow director")
> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>

...

Re: [PATCH net 2/3] net: hns3: fix spelling mistake "reg_um" -> "reg_num"

[Simon Horman]

On Wed, Apr 02, 2025 at 08:10:00PM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@huawei.com>
> 
> There are spelling mistakes in hclgevf_get_regs. Fix them.
> 
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>

Hi,

I agree this is a nice change. But I would lean to it being a clean-up
and thus material for net-next (when it reopens) rather than a (bug) fix
for net.

...

Re: [PATCH net 3/3] net: hns3: store rx VLAN tag offload state for VF

[Simon Horman]

On Wed, Apr 02, 2025 at 08:10:01PM +0800, Jijie Shao wrote:
> From: Jian Shen <shenjian15@huawei.com>
> 
> The VF driver missed to store the rx VLAN tag strip state when
> user change the rx VLAN tag offload state. And it will default
> to enable the rx vlan tag strip when re-init VF device after
> reset. So if user disable rx VLAN tag offload, and trig reset,
> then the HW will still strip the VLAN tag from packet nad fill
> into RX BD, but the VF driver will ignore it for rx VLAN tag
> offload disabled. It may cause the rx VLAN tag dropped.
> 
> Fixes: b2641e2ad456 ("net: hns3: Add support of hardware rx-vlan-offload to HNS3 VF driver")
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>

Overall this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
> index cccef3228461..1e452b14b04e 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
> @@ -252,7 +252,8 @@ struct hclgevf_dev {
>  	u16 *vector_status;
>  	int *vector_irq;
>  
> -	bool gro_en;
> +	u32 gro_en :1;
> +	u32 rxvtag_strip_en :1;

FWIIW, as there is space I would have used two bools here.

>  
>  	unsigned long vlan_del_fail_bmap[BITS_TO_LONGS(VLAN_N_VID)];
>  
> -- 
> 2.33.0
> 

Re: [PATCH net 3/3] net: hns3: store rx VLAN tag offload state for VF

[Jijie Shao]

on 2025/4/2 22:01, Simon Horman wrote:
> On Wed, Apr 02, 2025 at 08:10:01PM +0800, Jijie Shao wrote:
>> From: Jian Shen <shenjian15@huawei.com>
>>
>> The VF driver missed to store the rx VLAN tag strip state when
>> user change the rx VLAN tag offload state. And it will default
>> to enable the rx vlan tag strip when re-init VF device after
>> reset. So if user disable rx VLAN tag offload, and trig reset,
>> then the HW will still strip the VLAN tag from packet nad fill
>> into RX BD, but the VF driver will ignore it for rx VLAN tag
>> offload disabled. It may cause the rx VLAN tag dropped.
>>
>> Fixes: b2641e2ad456 ("net: hns3: Add support of hardware rx-vlan-offload to HNS3 VF driver")
>> Signed-off-by: Jian Shen <shenjian15@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Overall this looks good to me.
>
> Reviewed-by: Simon Horman <horms@kernel.org>
>
> ...
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
>> index cccef3228461..1e452b14b04e 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
>> @@ -252,7 +252,8 @@ struct hclgevf_dev {
>>   	u16 *vector_status;
>>   	int *vector_irq;
>>   
>> -	bool gro_en;
>> +	u32 gro_en :1;
>> +	u32 rxvtag_strip_en :1;
> FWIIW, as there is space I would have used two bools here.

ok, I will change to bool in v2

Thanks,
Jijie Shao

>
>>   
>>   	unsigned long vlan_del_fail_bmap[BITS_TO_LONGS(VLAN_N_VID)];
>>   
>> -- 
>> 2.33.0
>>

Re: [PATCH net 2/3] net: hns3: fix spelling mistake "reg_um" -> "reg_num"

[Jijie Shao]

on 2025/4/2 21:59, Simon Horman wrote:
> On Wed, Apr 02, 2025 at 08:10:00PM +0800, Jijie Shao wrote:
>> From: Hao Lan <lanhao@huawei.com>
>>
>> There are spelling mistakes in hclgevf_get_regs. Fix them.
>>
>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Hi,
>
> I agree this is a nice change. But I would lean to it being a clean-up
> and thus material for net-next (when it reopens) rather than a (bug) fix
> for net.

ok, Thanks!

Jijie Shao

>
> ...
>

Re: [PATCH net 1/3] net: hns3: fix a use of uninitialized variable problem

[Jijie Shao]

on 2025/4/2 21:58, Simon Horman wrote:
> On Wed, Apr 02, 2025 at 08:09:59PM +0800, Jijie Shao wrote:
>> From: Yonglong Liu <liuyonglong@huawei.com>
>>
>> In hclge_add_fd_entry(), if the flow type is FLOW_EXT, and the data of
>> m_ext is all zero, then some members of the local variable "info" are
>> not initialized.
> Hi,
>
> I am assuming that this occurs when hclge_fd_check_spec() returns early
> like this:
>
>          if (!(fs->flow_type & FLOW_EXT) || hclge_fd_is_user_def_all_masked(fs))
>                  return 0;
>
> But if so, should the description be "flow type is not FLOW_EXT, ..."
> (note the "not")? Or perhaps more clearly refer to the FLOW_EXT bit
> not being set?
>
> Also, I think it would be worth mentioning hclge_fd_check_spec()
> in the patch description, perhaps something like this.
>
>    In hclge_add_fd_entry(), if  hclge_fd_check_spec() returns early
>    because the FLOW_EXT bit is not set in the flow type, and ...
>
> Also, does this manifest in a user-visible problem? If so, I think
> it would be good to describe it in the patch description.
>
> If not, I think it would be good to mention how the problem was found.
> E.g.: "flagged by static analysis" (and mention the tool if it is publicly
> Available. Or, "found by inspection".

Yes, your comments are reasonable.
I will add the description in v2

Thanks,
Jijie Shao

>
>> Fixes: 67b0e1428e2f ("net: hns3: add support for user-def data of flow director")
>> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ...