[PATCH RESEND V2 net 0/7] There are some bugfix for the HNS3 ethernet driver

[PATCH RESEND V2 net 0/7] There are some bugfix for the HNS3 ethernet driver

[Jijie Shao]

There's a series of bugfix that's been accepted:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=d80a3091308491455b6501b1c4b68698c4a7cd24

However, The series is making the driver poke into IOMMU internals instead of
implementing appropriate IOMMU workarounds. After discussion, the series was reverted:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=249cfa318fb1b77eb726c2ff4f74c9685f04e568

But only two patches are related to the IOMMU.
Other patches involve only the modification of the driver.
This series resends other patches.

---
ChangeLog:
v2 -> v2 RESEND:
  - Send to net instead of net-next.
  v2: https://lore.kernel.org/all/20241216132346.1197079-1-shaojijie@huawei.com/
v1 -> v2:
  - Fix a data inconsistency issue caused by simultaneous access of multiple readers,
    suggested by Jakub.
  v1: https://lore.kernel.org/all/20241107133023.3813095-1-shaojijie@huawei.com/
---
Hao Lan (4):
  net: hns3: fixed reset failure issues caused by the incorrect reset
    type
  net: hns3: fix missing features due to dev->features configuration too
    early
  net: hns3: Resolved the issue that the debugfs query result is
    inconsistent.
  net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds
    issue

Jian Shen (2):
  net: hns3: don't auto enable misc vector
  net: hns3: initialize reset_timer before hclgevf_misc_irq_init()

Jie Wang (1):
  net: hns3: fix kernel crash when 1588 is sent on HIP08 devices

 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |  3 -
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 96 ++++++-------------
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  1 -
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 45 +++++++--
 .../hisilicon/hns3/hns3pf/hclge_ptp.c         |  3 +
 .../hisilicon/hns3/hns3pf/hclge_regs.c        |  9 +-
 .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 40 ++++++--
 .../hisilicon/hns3/hns3vf/hclgevf_regs.c      |  9 +-
 8 files changed, 113 insertions(+), 93 deletions(-)

-- 
2.33.0

[PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Jijie Shao]

From: Hao Lan <lanhao@huawei.com>

When a reset type that is not supported by the driver is input, a reset
pending flag bit of the HNAE3_NONE_RESET type is generated in
reset_pending. The driver does not have a mechanism to clear this type
of error. As a result, the driver considers that the reset is not
complete. This patch provides a mechanism to clear the
HNAE3_NONE_RESET flag and the parameter of
hnae3_ae_ops.set_default_reset_request is verified.

The error message:
hns3 0000:39:01.0: cmd failed -16
hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
hns3 0000:39:01.0: failed to reset VF stack
hns3 0000:39:01.0: failed to reset VF(4)
hns3 0000:39:01.0: prepare reset(2) wait done
hns3 0000:39:01.0 eth4: already uninitialized

Use the crash tool to view struct hclgevf_dev:
struct hclgevf_dev {
...
	default_reset_request = 0x20,
	reset_level = HNAE3_NONE_RESET,
	reset_pending = 0x100,
	reset_type = HNAE3_NONE_RESET,
...
};

Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 33 ++++++++++++++--
 .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 38 ++++++++++++++++---
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 05942fa78b11..7d44dc777dc5 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3574,6 +3574,17 @@ static int hclge_set_vf_link_state(struct hnae3_handle *handle, int vf,
 	return ret;
 }
 
+static void hclge_set_reset_pending(struct hclge_dev *hdev,
+				    enum hnae3_reset_type reset_type)
+{
+	/* When an incorrect reset type is executed, the get_reset_level
+	 * function generates the HNAE3_NONE_RESET flag. As a result, this
+	 * type do not need to pending.
+	 */
+	if (reset_type != HNAE3_NONE_RESET)
+		set_bit(reset_type, &hdev->reset_pending);
+}
+
 static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
 {
 	u32 cmdq_src_reg, msix_src_reg, hw_err_src_reg;
@@ -3594,7 +3605,7 @@ static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
 	 */
 	if (BIT(HCLGE_VECTOR0_IMPRESET_INT_B) & msix_src_reg) {
 		dev_info(&hdev->pdev->dev, "IMP reset interrupt\n");
-		set_bit(HNAE3_IMP_RESET, &hdev->reset_pending);
+		hclge_set_reset_pending(hdev, HNAE3_IMP_RESET);
 		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
 		*clearval = BIT(HCLGE_VECTOR0_IMPRESET_INT_B);
 		hdev->rst_stats.imp_rst_cnt++;
@@ -3604,7 +3615,7 @@ static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
 	if (BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B) & msix_src_reg) {
 		dev_info(&hdev->pdev->dev, "global reset interrupt\n");
 		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
-		set_bit(HNAE3_GLOBAL_RESET, &hdev->reset_pending);
+		hclge_set_reset_pending(hdev, HNAE3_GLOBAL_RESET);
 		*clearval = BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B);
 		hdev->rst_stats.global_rst_cnt++;
 		return HCLGE_VECTOR0_EVENT_RST;
@@ -4052,7 +4063,7 @@ static void hclge_do_reset(struct hclge_dev *hdev)
 	case HNAE3_FUNC_RESET:
 		dev_info(&pdev->dev, "PF reset requested\n");
 		/* schedule again to check later */
-		set_bit(HNAE3_FUNC_RESET, &hdev->reset_pending);
+		hclge_set_reset_pending(hdev, HNAE3_FUNC_RESET);
 		hclge_reset_task_schedule(hdev);
 		break;
 	default:
@@ -4086,6 +4097,8 @@ static enum hnae3_reset_type hclge_get_reset_level(struct hnae3_ae_dev *ae_dev,
 		clear_bit(HNAE3_FLR_RESET, addr);
 	}
 
+	clear_bit(HNAE3_NONE_RESET, addr);
+
 	if (hdev->reset_type != HNAE3_NONE_RESET &&
 	    rst_level < hdev->reset_type)
 		return HNAE3_NONE_RESET;
@@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
 		return false;
 	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
 		hdev->rst_stats.reset_fail_cnt++;
-		set_bit(hdev->reset_type, &hdev->reset_pending);
+		hclge_set_reset_pending(hdev, hdev->reset_type);
 		dev_info(&hdev->pdev->dev,
 			 "re-schedule reset task(%u)\n",
 			 hdev->rst_stats.reset_fail_cnt);
@@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
 static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
 					enum hnae3_reset_type rst_type)
 {
+#define HCLGE_SUPPORT_RESET_TYPE \
+	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
+	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
+
 	struct hclge_dev *hdev = ae_dev->priv;
 
+	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
+		/* To prevent reset triggered by hclge_reset_event */
+		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
+		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
+			 rst_type);
+		return;
+	}
+
 	set_bit(rst_type, &hdev->default_reset_request);
 }
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 2f6ffb88e700..fd0abe37fdd7 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
 	return ret;
 }
 
+static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
+				      enum hnae3_reset_type reset_type)
+{
+	/* When an incorrect reset type is executed, the get_reset_level
+	 * function generates the HNAE3_NONE_RESET flag. As a result, this
+	 * type do not need to pending.
+	 */
+	if (reset_type != HNAE3_NONE_RESET)
+		set_bit(reset_type, &hdev->reset_pending);
+}
+
 static int hclgevf_reset_wait(struct hclgevf_dev *hdev)
 {
 #define HCLGEVF_RESET_WAIT_US	20000
@@ -1542,7 +1553,7 @@ static void hclgevf_reset_err_handle(struct hclgevf_dev *hdev)
 		hdev->rst_stats.rst_fail_cnt);
 
 	if (hdev->rst_stats.rst_fail_cnt < HCLGEVF_RESET_MAX_FAIL_CNT)
-		set_bit(hdev->reset_type, &hdev->reset_pending);
+		hclgevf_set_reset_pending(hdev, hdev->reset_type);
 
 	if (hclgevf_is_reset_pending(hdev)) {
 		set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
@@ -1662,6 +1673,8 @@ static enum hnae3_reset_type hclgevf_get_reset_level(unsigned long *addr)
 		clear_bit(HNAE3_FLR_RESET, addr);
 	}
 
+	clear_bit(HNAE3_NONE_RESET, addr);
+
 	return rst_level;
 }
 
@@ -1671,14 +1684,15 @@ static void hclgevf_reset_event(struct pci_dev *pdev,
 	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(pdev);
 	struct hclgevf_dev *hdev = ae_dev->priv;
 
-	dev_info(&hdev->pdev->dev, "received reset request from VF enet\n");
-
 	if (hdev->default_reset_request)
 		hdev->reset_level =
 			hclgevf_get_reset_level(&hdev->default_reset_request);
 	else
 		hdev->reset_level = HNAE3_VF_FUNC_RESET;
 
+	dev_info(&hdev->pdev->dev, "received reset request from VF enet, reset level is %d\n",
+		 hdev->reset_level);
+
 	/* reset of this VF requested */
 	set_bit(HCLGEVF_RESET_REQUESTED, &hdev->reset_state);
 	hclgevf_reset_task_schedule(hdev);
@@ -1689,8 +1703,20 @@ static void hclgevf_reset_event(struct pci_dev *pdev,
 static void hclgevf_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
 					  enum hnae3_reset_type rst_type)
 {
+#define HCLGEVF_SUPPORT_RESET_TYPE \
+	(BIT(HNAE3_VF_RESET) | BIT(HNAE3_VF_FUNC_RESET) | \
+	BIT(HNAE3_VF_PF_FUNC_RESET) | BIT(HNAE3_VF_FULL_RESET) | \
+	BIT(HNAE3_FLR_RESET) | BIT(HNAE3_VF_EXP_RESET))
+
 	struct hclgevf_dev *hdev = ae_dev->priv;
 
+	if (!(BIT(rst_type) & HCLGEVF_SUPPORT_RESET_TYPE)) {
+		/* To prevent reset triggered by hclge_reset_event */
+		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
+		dev_info(&hdev->pdev->dev, "unsupported reset type %d\n",
+			 rst_type);
+		return;
+	}
 	set_bit(rst_type, &hdev->default_reset_request);
 }
 
@@ -1847,14 +1873,14 @@ static void hclgevf_reset_service_task(struct hclgevf_dev *hdev)
 		 */
 		if (hdev->reset_attempts > HCLGEVF_MAX_RESET_ATTEMPTS_CNT) {
 			/* prepare for full reset of stack + pcie interface */
-			set_bit(HNAE3_VF_FULL_RESET, &hdev->reset_pending);
+			hclgevf_set_reset_pending(hdev, HNAE3_VF_FULL_RESET);
 
 			/* "defer" schedule the reset task again */
 			set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
 		} else {
 			hdev->reset_attempts++;
 
-			set_bit(hdev->reset_level, &hdev->reset_pending);
+			hclgevf_set_reset_pending(hdev, hdev->reset_level);
 			set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
 		}
 		hclgevf_reset_task_schedule(hdev);
@@ -1977,7 +2003,7 @@ static enum hclgevf_evt_cause hclgevf_check_evt_cause(struct hclgevf_dev *hdev,
 		rst_ing_reg = hclgevf_read_dev(&hdev->hw, HCLGEVF_RST_ING);
 		dev_info(&hdev->pdev->dev,
 			 "receive reset interrupt 0x%x!\n", rst_ing_reg);
-		set_bit(HNAE3_VF_RESET, &hdev->reset_pending);
+		hclgevf_set_reset_pending(hdev, HNAE3_VF_RESET);
 		set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
 		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
 		*clearval = ~(1U << HCLGEVF_VECTOR0_RST_INT_B);
-- 
2.33.0

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Michal Swiatkowski]

On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@huawei.com>
> 
> When a reset type that is not supported by the driver is input, a reset
> pending flag bit of the HNAE3_NONE_RESET type is generated in
> reset_pending. The driver does not have a mechanism to clear this type
> of error. As a result, the driver considers that the reset is not
> complete. This patch provides a mechanism to clear the
> HNAE3_NONE_RESET flag and the parameter of
> hnae3_ae_ops.set_default_reset_request is verified.
> 
> The error message:
> hns3 0000:39:01.0: cmd failed -16
> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
> hns3 0000:39:01.0: failed to reset VF stack
> hns3 0000:39:01.0: failed to reset VF(4)
> hns3 0000:39:01.0: prepare reset(2) wait done
> hns3 0000:39:01.0 eth4: already uninitialized
> 
> Use the crash tool to view struct hclgevf_dev:
> struct hclgevf_dev {
> ...
> 	default_reset_request = 0x20,
> 	reset_level = HNAE3_NONE_RESET,
> 	reset_pending = 0x100,
> 	reset_type = HNAE3_NONE_RESET,
> ...
> };
> 
> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  .../hisilicon/hns3/hns3pf/hclge_main.c        | 33 ++++++++++++++--
>  .../hisilicon/hns3/hns3vf/hclgevf_main.c      | 38 ++++++++++++++++---
>  2 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 05942fa78b11..7d44dc777dc5 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -3574,6 +3574,17 @@ static int hclge_set_vf_link_state(struct hnae3_handle *handle, int vf,
>  	return ret;
>  }
>  
> +static void hclge_set_reset_pending(struct hclge_dev *hdev,
> +				    enum hnae3_reset_type reset_type)
> +{
> +	/* When an incorrect reset type is executed, the get_reset_level
> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
> +	 * type do not need to pending.
> +	 */
> +	if (reset_type != HNAE3_NONE_RESET)
> +		set_bit(reset_type, &hdev->reset_pending);
> +}
> +
>  static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
>  {
>  	u32 cmdq_src_reg, msix_src_reg, hw_err_src_reg;
> @@ -3594,7 +3605,7 @@ static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
>  	 */
>  	if (BIT(HCLGE_VECTOR0_IMPRESET_INT_B) & msix_src_reg) {
>  		dev_info(&hdev->pdev->dev, "IMP reset interrupt\n");
> -		set_bit(HNAE3_IMP_RESET, &hdev->reset_pending);
> +		hclge_set_reset_pending(hdev, HNAE3_IMP_RESET);
>  		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
>  		*clearval = BIT(HCLGE_VECTOR0_IMPRESET_INT_B);
>  		hdev->rst_stats.imp_rst_cnt++;
> @@ -3604,7 +3615,7 @@ static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
>  	if (BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B) & msix_src_reg) {
>  		dev_info(&hdev->pdev->dev, "global reset interrupt\n");
>  		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
> -		set_bit(HNAE3_GLOBAL_RESET, &hdev->reset_pending);
> +		hclge_set_reset_pending(hdev, HNAE3_GLOBAL_RESET);
>  		*clearval = BIT(HCLGE_VECTOR0_GLOBALRESET_INT_B);
>  		hdev->rst_stats.global_rst_cnt++;
>  		return HCLGE_VECTOR0_EVENT_RST;
> @@ -4052,7 +4063,7 @@ static void hclge_do_reset(struct hclge_dev *hdev)
>  	case HNAE3_FUNC_RESET:
>  		dev_info(&pdev->dev, "PF reset requested\n");
>  		/* schedule again to check later */
> -		set_bit(HNAE3_FUNC_RESET, &hdev->reset_pending);
> +		hclge_set_reset_pending(hdev, HNAE3_FUNC_RESET);
>  		hclge_reset_task_schedule(hdev);
>  		break;
>  	default:
> @@ -4086,6 +4097,8 @@ static enum hnae3_reset_type hclge_get_reset_level(struct hnae3_ae_dev *ae_dev,
>  		clear_bit(HNAE3_FLR_RESET, addr);
>  	}
>  
> +	clear_bit(HNAE3_NONE_RESET, addr);
> +
>  	if (hdev->reset_type != HNAE3_NONE_RESET &&
>  	    rst_level < hdev->reset_type)
>  		return HNAE3_NONE_RESET;
> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>  		return false;
>  	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>  		hdev->rst_stats.reset_fail_cnt++;
> -		set_bit(hdev->reset_type, &hdev->reset_pending);
> +		hclge_set_reset_pending(hdev, hdev->reset_type);
Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
no reset? If yes, why in this case reset_fail_cnt++ is increasing?

Maybe the check for NONE_RESET should be done in this else if check to
prevent reset_fail_cnt from increasing (and also solve the problem with
pending bit set)
>  		dev_info(&hdev->pdev->dev,
>  			 "re-schedule reset task(%u)\n",
>  			 hdev->rst_stats.reset_fail_cnt);
> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
>  static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
>  					enum hnae3_reset_type rst_type)
>  {
> +#define HCLGE_SUPPORT_RESET_TYPE \
> +	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
> +	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
> +
>  	struct hclge_dev *hdev = ae_dev->priv;
>  
> +	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
> +		/* To prevent reset triggered by hclge_reset_event */
> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
> +		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
> +			 rst_type);
> +		return;
> +	}
Maybe (nit):
if (...) {
	rst_type = 
	dev_warn();
}

set_bit(rst_type, );
It is a little hard to follow with return in the if.

> +
>  	set_bit(rst_type, &hdev->default_reset_request);
>  }
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> index 2f6ffb88e700..fd0abe37fdd7 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
>  	return ret;
>  }
>  
> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
> +				      enum hnae3_reset_type reset_type)
> +{
> +	/* When an incorrect reset type is executed, the get_reset_level
> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
> +	 * type do not need to pending.
> +	 */
> +	if (reset_type != HNAE3_NONE_RESET)
> +		set_bit(reset_type, &hdev->reset_pending);
> +}
You already have a way to share the code between PF and VF, so please
move the same functions to common file in one direction up.

> +
>  static int hclgevf_reset_wait(struct hclgevf_dev *hdev)
>  {
>  #define HCLGEVF_RESET_WAIT_US	20000
> @@ -1542,7 +1553,7 @@ static void hclgevf_reset_err_handle(struct hclgevf_dev *hdev)
>  		hdev->rst_stats.rst_fail_cnt);
>  
>  	if (hdev->rst_stats.rst_fail_cnt < HCLGEVF_RESET_MAX_FAIL_CNT)
> -		set_bit(hdev->reset_type, &hdev->reset_pending);
> +		hclgevf_set_reset_pending(hdev, hdev->reset_type);
>  
>  	if (hclgevf_is_reset_pending(hdev)) {
>  		set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
> @@ -1662,6 +1673,8 @@ static enum hnae3_reset_type hclgevf_get_reset_level(unsigned long *addr)
>  		clear_bit(HNAE3_FLR_RESET, addr);
>  	}
>  
> +	clear_bit(HNAE3_NONE_RESET, addr);
> +
>  	return rst_level;
>  }
>  
> @@ -1671,14 +1684,15 @@ static void hclgevf_reset_event(struct pci_dev *pdev,
>  	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(pdev);
>  	struct hclgevf_dev *hdev = ae_dev->priv;
>  
> -	dev_info(&hdev->pdev->dev, "received reset request from VF enet\n");
> -
>  	if (hdev->default_reset_request)
>  		hdev->reset_level =
>  			hclgevf_get_reset_level(&hdev->default_reset_request);
>  	else
>  		hdev->reset_level = HNAE3_VF_FUNC_RESET;
>  
> +	dev_info(&hdev->pdev->dev, "received reset request from VF enet, reset level is %d\n",
> +		 hdev->reset_level);
> +
>  	/* reset of this VF requested */
>  	set_bit(HCLGEVF_RESET_REQUESTED, &hdev->reset_state);
>  	hclgevf_reset_task_schedule(hdev);
> @@ -1689,8 +1703,20 @@ static void hclgevf_reset_event(struct pci_dev *pdev,
>  static void hclgevf_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
>  					  enum hnae3_reset_type rst_type)
>  {
> +#define HCLGEVF_SUPPORT_RESET_TYPE \
> +	(BIT(HNAE3_VF_RESET) | BIT(HNAE3_VF_FUNC_RESET) | \
> +	BIT(HNAE3_VF_PF_FUNC_RESET) | BIT(HNAE3_VF_FULL_RESET) | \
> +	BIT(HNAE3_FLR_RESET) | BIT(HNAE3_VF_EXP_RESET))
> +
>  	struct hclgevf_dev *hdev = ae_dev->priv;
>  
> +	if (!(BIT(rst_type) & HCLGEVF_SUPPORT_RESET_TYPE)) {
> +		/* To prevent reset triggered by hclge_reset_event */
> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
> +		dev_info(&hdev->pdev->dev, "unsupported reset type %d\n",
> +			 rst_type);
> +		return;
> +	}
>  	set_bit(rst_type, &hdev->default_reset_request);
>  }
>  
> @@ -1847,14 +1873,14 @@ static void hclgevf_reset_service_task(struct hclgevf_dev *hdev)
>  		 */
>  		if (hdev->reset_attempts > HCLGEVF_MAX_RESET_ATTEMPTS_CNT) {
>  			/* prepare for full reset of stack + pcie interface */
> -			set_bit(HNAE3_VF_FULL_RESET, &hdev->reset_pending);
> +			hclgevf_set_reset_pending(hdev, HNAE3_VF_FULL_RESET);
>  
>  			/* "defer" schedule the reset task again */
>  			set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
>  		} else {
>  			hdev->reset_attempts++;
>  
> -			set_bit(hdev->reset_level, &hdev->reset_pending);
> +			hclgevf_set_reset_pending(hdev, hdev->reset_level);
>  			set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
>  		}
>  		hclgevf_reset_task_schedule(hdev);
> @@ -1977,7 +2003,7 @@ static enum hclgevf_evt_cause hclgevf_check_evt_cause(struct hclgevf_dev *hdev,
>  		rst_ing_reg = hclgevf_read_dev(&hdev->hw, HCLGEVF_RST_ING);
>  		dev_info(&hdev->pdev->dev,
>  			 "receive reset interrupt 0x%x!\n", rst_ing_reg);
> -		set_bit(HNAE3_VF_RESET, &hdev->reset_pending);
> +		hclgevf_set_reset_pending(hdev, HNAE3_VF_RESET);
>  		set_bit(HCLGEVF_RESET_PENDING, &hdev->reset_state);
>  		set_bit(HCLGE_COMM_STATE_CMD_DISABLE, &hdev->hw.hw.comm_state);
>  		*clearval = ~(1U << HCLGEVF_VECTOR0_RST_INT_B);
> -- 
> 2.33.0

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Paolo Abeni]

On 12/18/24 10:02, Michal Swiatkowski wrote:
> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>> From: Hao Lan <lanhao@huawei.com>
>>
>> When a reset type that is not supported by the driver is input, a reset
>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>> reset_pending. The driver does not have a mechanism to clear this type
>> of error. As a result, the driver considers that the reset is not
>> complete. This patch provides a mechanism to clear the
>> HNAE3_NONE_RESET flag and the parameter of
>> hnae3_ae_ops.set_default_reset_request is verified.
>>
>> The error message:
>> hns3 0000:39:01.0: cmd failed -16
>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>> hns3 0000:39:01.0: failed to reset VF stack
>> hns3 0000:39:01.0: failed to reset VF(4)
>> hns3 0000:39:01.0: prepare reset(2) wait done
>> hns3 0000:39:01.0 eth4: already uninitialized
>>
>> Use the crash tool to view struct hclgevf_dev:
>> struct hclgevf_dev {
>> ...
>> 	default_reset_request = 0x20,
>> 	reset_level = HNAE3_NONE_RESET,
>> 	reset_pending = 0x100,
>> 	reset_type = HNAE3_NONE_RESET,
>> ...
>> };
>>
>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

I haven't signed-off this patch.

Still no need to repost (yet) for this if the following points are
solved rapidly (as I may end-up merging the series and really adding my
SoB), but please avoid this kind of issue in the future.

>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>>  		return false;
>>  	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>>  		hdev->rst_stats.reset_fail_cnt++;
>> -		set_bit(hdev->reset_type, &hdev->reset_pending);
>> +		hclge_set_reset_pending(hdev, hdev->reset_type);
> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
> 
> Maybe the check for NONE_RESET should be done in this else if check to
> prevent reset_fail_cnt from increasing (and also solve the problem with
> pending bit set)

@Michal: I don't understand your comment above. hclge_reset_err_handle()
handles attempted reset failures. I don't see it triggered when
reset_type == HNAE3_NONE_RESET.

>> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
>>  static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
>>  					enum hnae3_reset_type rst_type)
>>  {
>> +#define HCLGE_SUPPORT_RESET_TYPE \
>> +	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
>> +	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
>> +
>>  	struct hclge_dev *hdev = ae_dev->priv;
>>  
>> +	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
>> +		/* To prevent reset triggered by hclge_reset_event */
>> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
>> +		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
>> +			 rst_type);
>> +		return;
>> +	}
> Maybe (nit):
> if (...) {
> 	rst_type = 
> 	dev_warn();
> }
> 
> set_bit(rst_type, );
> It is a little hard to follow with return in the if.

@Michal: I personally find the patch code quite readable, do you have
strong opinions here?

>>  	set_bit(rst_type, &hdev->default_reset_request);
>>  }
>>  
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> index 2f6ffb88e700..fd0abe37fdd7 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
>>  	return ret;
>>  }
>>  
>> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
>> +				      enum hnae3_reset_type reset_type)
>> +{
>> +	/* When an incorrect reset type is executed, the get_reset_level
>> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
>> +	 * type do not need to pending.
>> +	 */
>> +	if (reset_type != HNAE3_NONE_RESET)
>> +		set_bit(reset_type, &hdev->reset_pending);
>> +}
> You already have a way to share the code between PF and VF, so please
> move the same functions to common file in one direction up.

AFAICS this can't be shared short of a large refactor not suitable for
net as the functions eligible for sharing operate on different structs
with different layout (hclgevf_dev vs hclge_dev). Currently all the
shared code operates on shared structs.

Cheers,

Paolo

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Michal Swiatkowski]

On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
> On 12/18/24 10:02, Michal Swiatkowski wrote:
> > On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
> >> From: Hao Lan <lanhao@huawei.com>
> >>
> >> When a reset type that is not supported by the driver is input, a reset
> >> pending flag bit of the HNAE3_NONE_RESET type is generated in
> >> reset_pending. The driver does not have a mechanism to clear this type
> >> of error. As a result, the driver considers that the reset is not
> >> complete. This patch provides a mechanism to clear the
> >> HNAE3_NONE_RESET flag and the parameter of
> >> hnae3_ae_ops.set_default_reset_request is verified.
> >>
> >> The error message:
> >> hns3 0000:39:01.0: cmd failed -16
> >> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
> >> hns3 0000:39:01.0: failed to reset VF stack
> >> hns3 0000:39:01.0: failed to reset VF(4)
> >> hns3 0000:39:01.0: prepare reset(2) wait done
> >> hns3 0000:39:01.0 eth4: already uninitialized
> >>
> >> Use the crash tool to view struct hclgevf_dev:
> >> struct hclgevf_dev {
> >> ...
> >> 	default_reset_request = 0x20,
> >> 	reset_level = HNAE3_NONE_RESET,
> >> 	reset_pending = 0x100,
> >> 	reset_type = HNAE3_NONE_RESET,
> >> ...
> >> };
> >>
> >> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
> >> Signed-off-by: Hao Lan <lanhao@huawei.com>
> >> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> I haven't signed-off this patch.
> 
> Still no need to repost (yet) for this if the following points are
> solved rapidly (as I may end-up merging the series and really adding my
> SoB), but please avoid this kind of issue in the future.
> 
> >> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
> >>  		return false;
> >>  	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
> >>  		hdev->rst_stats.reset_fail_cnt++;
> >> -		set_bit(hdev->reset_type, &hdev->reset_pending);
> >> +		hclge_set_reset_pending(hdev, hdev->reset_type);
> > Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
> > no reset? If yes, why in this case reset_fail_cnt++ is increasing?
> > 
> > Maybe the check for NONE_RESET should be done in this else if check to
> > prevent reset_fail_cnt from increasing (and also solve the problem with
> > pending bit set)
> 
> @Michal: I don't understand your comment above. hclge_reset_err_handle()
> handles attempted reset failures. I don't see it triggered when
> reset_type == HNAE3_NONE_RESET.
> 

Maybe I missed sth. The hclge_set_reset_pending() is added to check if
reset type isn't HNAE3_NONE_RESET. If it is the set_bit isn't called. It
is the only place where hclge_set_reset_pending() is called with a
variable, so I assumed the fix is for this place.

This means that code can be reach here with HNAE3_NONE_RESET which is
unclear for me why to increment resets if rest_type in NONE. If it is
true that hclge_reset_err_handle() is never called with reset_type
HNAE3_NONE_RESET it shouldn't be needed to have the
hclge_set_reset_pending() function.

Following that I suggested to check if reset_type isn't NONE before
checking other things.

> >> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
> >>  static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
> >>  					enum hnae3_reset_type rst_type)
> >>  {
> >> +#define HCLGE_SUPPORT_RESET_TYPE \
> >> +	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
> >> +	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
> >> +
> >>  	struct hclge_dev *hdev = ae_dev->priv;
> >>  
> >> +	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
> >> +		/* To prevent reset triggered by hclge_reset_event */
> >> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
> >> +		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
> >> +			 rst_type);
> >> +		return;
> >> +	}
> > Maybe (nit):
> > if (...) {
> > 	rst_type = 
> > 	dev_warn();
> > }
> > 
> > set_bit(rst_type, );
> > It is a little hard to follow with return in the if.
> 
> @Michal: I personally find the patch code quite readable, do you have
> strong opinions here?
> 
> >>  	set_bit(rst_type, &hdev->default_reset_request);
> >>  }
> >>  
> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> index 2f6ffb88e700..fd0abe37fdd7 100644
> >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
> >>  	return ret;
> >>  }
> >>  
> >> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
> >> +				      enum hnae3_reset_type reset_type)
> >> +{
> >> +	/* When an incorrect reset type is executed, the get_reset_level
> >> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
> >> +	 * type do not need to pending.
> >> +	 */
> >> +	if (reset_type != HNAE3_NONE_RESET)
> >> +		set_bit(reset_type, &hdev->reset_pending);
> >> +}
> > You already have a way to share the code between PF and VF, so please
> > move the same functions to common file in one direction up.
> 
> AFAICS this can't be shared short of a large refactor not suitable for
> net as the functions eligible for sharing operate on different structs
> with different layout (hclgevf_dev vs hclge_dev). Currently all the
> shared code operates on shared structs.
> 
> Cheers,
> 
> Paolo
> 

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Paolo Abeni]

On 12/19/24 11:11, Michal Swiatkowski wrote:
> On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
>> On 12/18/24 10:02, Michal Swiatkowski wrote:
>>> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>>>> From: Hao Lan <lanhao@huawei.com>
>>>>
>>>> When a reset type that is not supported by the driver is input, a reset
>>>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>>>> reset_pending. The driver does not have a mechanism to clear this type
>>>> of error. As a result, the driver considers that the reset is not
>>>> complete. This patch provides a mechanism to clear the
>>>> HNAE3_NONE_RESET flag and the parameter of
>>>> hnae3_ae_ops.set_default_reset_request is verified.
>>>>
>>>> The error message:
>>>> hns3 0000:39:01.0: cmd failed -16
>>>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>>>> hns3 0000:39:01.0: failed to reset VF stack
>>>> hns3 0000:39:01.0: failed to reset VF(4)
>>>> hns3 0000:39:01.0: prepare reset(2) wait done
>>>> hns3 0000:39:01.0 eth4: already uninitialized
>>>>
>>>> Use the crash tool to view struct hclgevf_dev:
>>>> struct hclgevf_dev {
>>>> ...
>>>> 	default_reset_request = 0x20,
>>>> 	reset_level = HNAE3_NONE_RESET,
>>>> 	reset_pending = 0x100,
>>>> 	reset_type = HNAE3_NONE_RESET,
>>>> ...
>>>> };
>>>>
>>>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
>>>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>>>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>
>> I haven't signed-off this patch.
>>
>> Still no need to repost (yet) for this if the following points are
>> solved rapidly (as I may end-up merging the series and really adding my
>> SoB), but please avoid this kind of issue in the future.
>>
>>>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>>>>  		return false;
>>>>  	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>>>>  		hdev->rst_stats.reset_fail_cnt++;
>>>> -		set_bit(hdev->reset_type, &hdev->reset_pending);
>>>> +		hclge_set_reset_pending(hdev, hdev->reset_type);
>>> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
>>> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
>>>
>>> Maybe the check for NONE_RESET should be done in this else if check to
>>> prevent reset_fail_cnt from increasing (and also solve the problem with
>>> pending bit set)
>>
>> @Michal: I don't understand your comment above. hclge_reset_err_handle()
>> handles attempted reset failures. I don't see it triggered when
>> reset_type == HNAE3_NONE_RESET.
>>
> 
> Maybe I missed sth. The hclge_set_reset_pending() is added to check if
> reset type isn't HNAE3_NONE_RESET. If it is the set_bit isn't called. It
> is the only place where hclge_set_reset_pending() is called with a
> variable, so I assumed the fix is for this place.
> 
> This means that code can be reach here with HNAE3_NONE_RESET which is
> unclear for me why to increment resets if rest_type in NONE. If it is
> true that hclge_reset_err_handle() is never called with reset_type
> HNAE3_NONE_RESET it shouldn't be needed to have the
> hclge_set_reset_pending() function.

You are right, I felt off-track.

@Jijie: how can 'reset_type' be set to an unsupported value?!? I don't
see that in the code, short of a memory corruption on uninit problem.
Are you sure you are not papering over a different issue here? At least
some more info (either in the commit description or in a code comment)
is IMHO needed. Otherwise you should probably catch that before
hclge_reset_err_handle() time.

Thanks!

Paolo

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Jijie Shao]

on 2024/12/19 18:43, Paolo Abeni wrote:
> On 12/19/24 11:11, Michal Swiatkowski wrote:
>> On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
>>> On 12/18/24 10:02, Michal Swiatkowski wrote:
>>>> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>>>>> From: Hao Lan <lanhao@huawei.com>
>>>>>
>>>>> When a reset type that is not supported by the driver is input, a reset
>>>>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>>>>> reset_pending. The driver does not have a mechanism to clear this type
>>>>> of error. As a result, the driver considers that the reset is not
>>>>> complete. This patch provides a mechanism to clear the
>>>>> HNAE3_NONE_RESET flag and the parameter of
>>>>> hnae3_ae_ops.set_default_reset_request is verified.
>>>>>
>>>>> The error message:
>>>>> hns3 0000:39:01.0: cmd failed -16
>>>>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>>>>> hns3 0000:39:01.0: failed to reset VF stack
>>>>> hns3 0000:39:01.0: failed to reset VF(4)
>>>>> hns3 0000:39:01.0: prepare reset(2) wait done
>>>>> hns3 0000:39:01.0 eth4: already uninitialized
>>>>>
>>>>> Use the crash tool to view struct hclgevf_dev:
>>>>> struct hclgevf_dev {
>>>>> ...
>>>>> 	default_reset_request = 0x20,
>>>>> 	reset_level = HNAE3_NONE_RESET,
>>>>> 	reset_pending = 0x100,
>>>>> 	reset_type = HNAE3_NONE_RESET,
>>>>> ...
>>>>> };
>>>>>
>>>>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
>>>>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>>>>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> I haven't signed-off this patch.
>>>
>>> Still no need to repost (yet) for this if the following points are
>>> solved rapidly (as I may end-up merging the series and really adding my
>>> SoB), but please avoid this kind of issue in the future.
>>>
>>>>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>>>>>   		return false;
>>>>>   	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>>>>>   		hdev->rst_stats.reset_fail_cnt++;
>>>>> -		set_bit(hdev->reset_type, &hdev->reset_pending);
>>>>> +		hclge_set_reset_pending(hdev, hdev->reset_type);
>>>> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
>>>> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
>>>>
>>>> Maybe the check for NONE_RESET should be done in this else if check to
>>>> prevent reset_fail_cnt from increasing (and also solve the problem with
>>>> pending bit set)
>>> @Michal: I don't understand your comment above. hclge_reset_err_handle()
>>> handles attempted reset failures. I don't see it triggered when
>>> reset_type == HNAE3_NONE_RESET.
>>>
>> Maybe I missed sth. The hclge_set_reset_pending() is added to check if
>> reset type isn't HNAE3_NONE_RESET. If it is the set_bit isn't called. It
>> is the only place where hclge_set_reset_pending() is called with a
>> variable, so I assumed the fix is for this place.
>>
>> This means that code can be reach here with HNAE3_NONE_RESET which is
>> unclear for me why to increment resets if rest_type in NONE. If it is
>> true that hclge_reset_err_handle() is never called with reset_type
>> HNAE3_NONE_RESET it shouldn't be needed to have the
>> hclge_set_reset_pending() function.
> You are right, I felt off-track.
>
> @Jijie: how can 'reset_type' be set to an unsupported value?!? I don't
> see that in the code, short of a memory corruption on uninit problem.
> Are you sure you are not papering over a different issue here? At least
> some more info (either in the commit description or in a code comment)
> is IMHO needed. Otherwise you should probably catch that before
> hclge_reset_err_handle() time.

Thanks for reviewing the code.

In fact, we used hclge_set_reset_pendin() to check in entire reset path,
not just hclge_reset_err_handle().

But seems like overkill, I'll try to simplify the modification.

Thank you.

Jiejie Shao

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Jijie Shao]

on 2024/12/19 20:26, Jijie Shao wrote:
>
> on 2024/12/19 18:43, Paolo Abeni wrote:
>> On 12/19/24 11:11, Michal Swiatkowski wrote:
>>> On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
>>>> On 12/18/24 10:02, Michal Swiatkowski wrote:
>>>>> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>>>>>> From: Hao Lan <lanhao@huawei.com>
>>>>>>
>>>>>> When a reset type that is not supported by the driver is input, a 
>>>>>> reset
>>>>>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>>>>>> reset_pending. The driver does not have a mechanism to clear this 
>>>>>> type
>>>>>> of error. As a result, the driver considers that the reset is not
>>>>>> complete. This patch provides a mechanism to clear the
>>>>>> HNAE3_NONE_RESET flag and the parameter of
>>>>>> hnae3_ae_ops.set_default_reset_request is verified.
>>>>>>
>>>>>> The error message:
>>>>>> hns3 0000:39:01.0: cmd failed -16
>>>>>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>>>>>> hns3 0000:39:01.0: failed to reset VF stack
>>>>>> hns3 0000:39:01.0: failed to reset VF(4)
>>>>>> hns3 0000:39:01.0: prepare reset(2) wait done
>>>>>> hns3 0000:39:01.0 eth4: already uninitialized
>>>>>>
>>>>>> Use the crash tool to view struct hclgevf_dev:
>>>>>> struct hclgevf_dev {
>>>>>> ...
>>>>>>     default_reset_request = 0x20,
>>>>>>     reset_level = HNAE3_NONE_RESET,
>>>>>>     reset_pending = 0x100,
>>>>>>     reset_type = HNAE3_NONE_RESET,
>>>>>> ...
>>>>>> };
>>>>>>
>>>>>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in 
>>>>>> the hnae3_ae_ops")
>>>>>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>>>>>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>>>>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>>> I haven't signed-off this patch.
>>>>
>>>> Still no need to repost (yet) for this if the following points are
>>>> solved rapidly (as I may end-up merging the series and really 
>>>> adding my
>>>> SoB), but please avoid this kind of issue in the future.
>>>>
>>>>>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct 
>>>>>> hclge_dev *hdev)
>>>>>>           return false;
>>>>>>       } else if (hdev->rst_stats.reset_fail_cnt < 
>>>>>> MAX_RESET_FAIL_CNT) {
>>>>>>           hdev->rst_stats.reset_fail_cnt++;
>>>>>> -        set_bit(hdev->reset_type, &hdev->reset_pending);
>>>>>> +        hclge_set_reset_pending(hdev, hdev->reset_type);
>>>>> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that 
>>>>> there is
>>>>> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
>>>>>
>>>>> Maybe the check for NONE_RESET should be done in this else if 
>>>>> check to
>>>>> prevent reset_fail_cnt from increasing (and also solve the problem 
>>>>> with
>>>>> pending bit set)
>>>> @Michal: I don't understand your comment above. 
>>>> hclge_reset_err_handle()
>>>> handles attempted reset failures. I don't see it triggered when
>>>> reset_type == HNAE3_NONE_RESET.
>>>>
>>> Maybe I missed sth. The hclge_set_reset_pending() is added to check if
>>> reset type isn't HNAE3_NONE_RESET. If it is the set_bit isn't 
>>> called. It
>>> is the only place where hclge_set_reset_pending() is called with a
>>> variable, so I assumed the fix is for this place.
>>>
>>> This means that code can be reach here with HNAE3_NONE_RESET which is
>>> unclear for me why to increment resets if rest_type in NONE. If it is
>>> true that hclge_reset_err_handle() is never called with reset_type
>>> HNAE3_NONE_RESET it shouldn't be needed to have the
>>> hclge_set_reset_pending() function.
>> You are right, I felt off-track.
>>
>> @Jijie: how can 'reset_type' be set to an unsupported value?!? I don't
>> see that in the code, short of a memory corruption on uninit problem.
>> Are you sure you are not papering over a different issue here? At least
>> some more info (either in the commit description or in a code comment)
>> is IMHO needed. Otherwise you should probably catch that before
>> hclge_reset_err_handle() time.
>
> Thanks for reviewing the code.
>
> In fact, we used hclge_set_reset_pendin() to check in entire reset path,
> not just hclge_reset_err_handle().
>
> But seems like overkill, I'll try to simplify the modification.

Hi：

I decided not to change it.

We use hclge_set_def_reset_request() to ensure that unsupported reset types can be processed
and hclge_set_reset_pending() to ensure that HNAE3_NONE_RESET can be cleared.

Although it looks like hclge_set_reset_pending() cleans up HNAE3_NONE_RESET in hclge_reset_err_handle(),
In VF, HNAE3_NONE_RESET is checked and cleared in advance..

Thank you.

>
> Thank you.
>
> Jiejie Shao
>

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Michal Swiatkowski]

On Thu, Dec 19, 2024 at 10:41:53AM +0100, Paolo Abeni wrote:
> On 12/18/24 10:02, Michal Swiatkowski wrote:
> > On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
> >> From: Hao Lan <lanhao@huawei.com>
> >>
> >> When a reset type that is not supported by the driver is input, a reset
> >> pending flag bit of the HNAE3_NONE_RESET type is generated in
> >> reset_pending. The driver does not have a mechanism to clear this type
> >> of error. As a result, the driver considers that the reset is not
> >> complete. This patch provides a mechanism to clear the
> >> HNAE3_NONE_RESET flag and the parameter of
> >> hnae3_ae_ops.set_default_reset_request is verified.
> >>
> >> The error message:
> >> hns3 0000:39:01.0: cmd failed -16
> >> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
> >> hns3 0000:39:01.0: failed to reset VF stack
> >> hns3 0000:39:01.0: failed to reset VF(4)
> >> hns3 0000:39:01.0: prepare reset(2) wait done
> >> hns3 0000:39:01.0 eth4: already uninitialized
> >>
> >> Use the crash tool to view struct hclgevf_dev:
> >> struct hclgevf_dev {
> >> ...
> >> 	default_reset_request = 0x20,
> >> 	reset_level = HNAE3_NONE_RESET,
> >> 	reset_pending = 0x100,
> >> 	reset_type = HNAE3_NONE_RESET,
> >> ...
> >> };
> >>
> >> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
> >> Signed-off-by: Hao Lan <lanhao@huawei.com>
> >> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> I haven't signed-off this patch.
> 
> Still no need to repost (yet) for this if the following points are
> solved rapidly (as I may end-up merging the series and really adding my
> SoB), but please avoid this kind of issue in the future.
> 
> >> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
> >>  		return false;
> >>  	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
> >>  		hdev->rst_stats.reset_fail_cnt++;
> >> -		set_bit(hdev->reset_type, &hdev->reset_pending);
> >> +		hclge_set_reset_pending(hdev, hdev->reset_type);
> > Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
> > no reset? If yes, why in this case reset_fail_cnt++ is increasing?
> > 
> > Maybe the check for NONE_RESET should be done in this else if check to
> > prevent reset_fail_cnt from increasing (and also solve the problem with
> > pending bit set)
> 
> @Michal: I don't understand your comment above. hclge_reset_err_handle()
> handles attempted reset failures. I don't see it triggered when
> reset_type == HNAE3_NONE_RESET.
> 
> >> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
> >>  static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
> >>  					enum hnae3_reset_type rst_type)
> >>  {
> >> +#define HCLGE_SUPPORT_RESET_TYPE \
> >> +	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
> >> +	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
> >> +
> >>  	struct hclge_dev *hdev = ae_dev->priv;
> >>  
> >> +	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
> >> +		/* To prevent reset triggered by hclge_reset_event */
> >> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
> >> +		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
> >> +			 rst_type);
> >> +		return;
> >> +	}
> > Maybe (nit):
> > if (...) {
> > 	rst_type = 
> > 	dev_warn();
> > }
> > 
> > set_bit(rst_type, );
> > It is a little hard to follow with return in the if.
> 
> @Michal: I personally find the patch code quite readable, do you have
> strong opinions here?
> 

Sorry, I anwered and missed other replies :(

No strong opionion, only nit, I should mark that.

> >>  	set_bit(rst_type, &hdev->default_reset_request);
> >>  }
> >>  
> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> index 2f6ffb88e700..fd0abe37fdd7 100644
> >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> >> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
> >>  	return ret;
> >>  }
> >>  
> >> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
> >> +				      enum hnae3_reset_type reset_type)
> >> +{
> >> +	/* When an incorrect reset type is executed, the get_reset_level
> >> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
> >> +	 * type do not need to pending.
> >> +	 */
> >> +	if (reset_type != HNAE3_NONE_RESET)
> >> +		set_bit(reset_type, &hdev->reset_pending);
> >> +}
> > You already have a way to share the code between PF and VF, so please
> > move the same functions to common file in one direction up.
> 
> AFAICS this can't be shared short of a large refactor not suitable for
> net as the functions eligible for sharing operate on different structs
> with different layout (hclgevf_dev vs hclge_dev). Currently all the
> shared code operates on shared structs.
> 

Ok, didn't check that. Fine for me so.

> Cheers,
> 
> Paolo
> 

Re: [PATCH RESEND V2 net 1/7] net: hns3: fixed reset failure issues caused by the incorrect reset type

[Jijie Shao]

on 2024/12/19 17:41, Paolo Abeni wrote:
> On 12/18/24 10:02, Michal Swiatkowski wrote:
>> On Tue, Dec 17, 2024 at 09:08:33AM +0800, Jijie Shao wrote:
>>> From: Hao Lan <lanhao@huawei.com>
>>>
>>> When a reset type that is not supported by the driver is input, a reset
>>> pending flag bit of the HNAE3_NONE_RESET type is generated in
>>> reset_pending. The driver does not have a mechanism to clear this type
>>> of error. As a result, the driver considers that the reset is not
>>> complete. This patch provides a mechanism to clear the
>>> HNAE3_NONE_RESET flag and the parameter of
>>> hnae3_ae_ops.set_default_reset_request is verified.
>>>
>>> The error message:
>>> hns3 0000:39:01.0: cmd failed -16
>>> hns3 0000:39:01.0: hclge device re-init failed, VF is disabled!
>>> hns3 0000:39:01.0: failed to reset VF stack
>>> hns3 0000:39:01.0: failed to reset VF(4)
>>> hns3 0000:39:01.0: prepare reset(2) wait done
>>> hns3 0000:39:01.0 eth4: already uninitialized
>>>
>>> Use the crash tool to view struct hclgevf_dev:
>>> struct hclgevf_dev {
>>> ...
>>> 	default_reset_request = 0x20,
>>> 	reset_level = HNAE3_NONE_RESET,
>>> 	reset_pending = 0x100,
>>> 	reset_type = HNAE3_NONE_RESET,
>>> ...
>>> };
>>>
>>> Fixes: 720bd5837e37 ("net: hns3: add set_default_reset_request in the hnae3_ae_ops")
>>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> I haven't signed-off this patch.
>
> Still no need to repost (yet) for this if the following points are
> solved rapidly (as I may end-up merging the series and really adding my
> SoB), but please avoid this kind of issue in the future.

Sorry, the patch is fotmated from the patch that has been accpected.
So SOB is added automatically. I will delete the SOB in next version.

>
>>> @@ -4227,7 +4240,7 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev)
>>>   		return false;
>>>   	} else if (hdev->rst_stats.reset_fail_cnt < MAX_RESET_FAIL_CNT) {
>>>   		hdev->rst_stats.reset_fail_cnt++;
>>> -		set_bit(hdev->reset_type, &hdev->reset_pending);
>>> +		hclge_set_reset_pending(hdev, hdev->reset_type);
>> Sth is unclear for me here. Doesn't HNAE3_NONE_RESET mean that there is
>> no reset? If yes, why in this case reset_fail_cnt++ is increasing?
>>
>> Maybe the check for NONE_RESET should be done in this else if check to
>> prevent reset_fail_cnt from increasing (and also solve the problem with
>> pending bit set)
> @Michal: I don't understand your comment above. hclge_reset_err_handle()
> handles attempted reset failures. I don't see it triggered when
> reset_type == HNAE3_NONE_RESET.
>
>>> @@ -4470,8 +4483,20 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
>>>   static void hclge_set_def_reset_request(struct hnae3_ae_dev *ae_dev,
>>>   					enum hnae3_reset_type rst_type)
>>>   {
>>> +#define HCLGE_SUPPORT_RESET_TYPE \
>>> +	(BIT(HNAE3_FLR_RESET) | BIT(HNAE3_FUNC_RESET) | \
>>> +	BIT(HNAE3_GLOBAL_RESET) | BIT(HNAE3_IMP_RESET))
>>> +
>>>   	struct hclge_dev *hdev = ae_dev->priv;
>>>   
>>> +	if (!(BIT(rst_type) & HCLGE_SUPPORT_RESET_TYPE)) {
>>> +		/* To prevent reset triggered by hclge_reset_event */
>>> +		set_bit(HNAE3_NONE_RESET, &hdev->default_reset_request);
>>> +		dev_warn(&hdev->pdev->dev, "unsupported reset type %d\n",
>>> +			 rst_type);
>>> +		return;
>>> +	}
>> Maybe (nit):
>> if (...) {
>> 	rst_type =
>> 	dev_warn();
>> }
>>
>> set_bit(rst_type, );
>> It is a little hard to follow with return in the if.
> @Michal: I personally find the patch code quite readable, do you have
> strong opinions here?
>
>>>   	set_bit(rst_type, &hdev->default_reset_request);
>>>   }
>>>   
>>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>>> index 2f6ffb88e700..fd0abe37fdd7 100644
>>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>>> @@ -1393,6 +1393,17 @@ static int hclgevf_notify_roce_client(struct hclgevf_dev *hdev,
>>>   	return ret;
>>>   }
>>>   
>>> +static void hclgevf_set_reset_pending(struct hclgevf_dev *hdev,
>>> +				      enum hnae3_reset_type reset_type)
>>> +{
>>> +	/* When an incorrect reset type is executed, the get_reset_level
>>> +	 * function generates the HNAE3_NONE_RESET flag. As a result, this
>>> +	 * type do not need to pending.
>>> +	 */
>>> +	if (reset_type != HNAE3_NONE_RESET)
>>> +		set_bit(reset_type, &hdev->reset_pending);
>>> +}
>> You already have a way to share the code between PF and VF, so please
>> move the same functions to common file in one direction up.
> AFAICS this can't be shared short of a large refactor not suitable for
> net as the functions eligible for sharing operate on different structs
> with different layout (hclgevf_dev vs hclge_dev). Currently all the
> shared code operates on shared structs.
>
> Cheers,

Yes, Paolo is right, this function cannot be shared.

Thanks，
Jijie Shao

>
> Paolo
>
>

[PATCH RESEND V2 net 2/7] net: hns3: fix missing features due to dev->features configuration too early

[Jijie Shao]

From: Hao Lan <lanhao@huawei.com>

Currently, the netdev->features is configured in hns3_nic_set_features.
As a result, __netdev_update_features considers that there is no feature
difference, and the procedures of the real features are missing.

Fixes: 2a7556bb2b73 ("net: hns3: implement ndo_features_check ops for hns3 driver")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 43377a7b2426..a7e3b22f641c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2452,7 +2452,6 @@ static int hns3_nic_set_features(struct net_device *netdev,
 			return ret;
 	}
 
-	netdev->features = features;
 	return 0;
 }
 
-- 
2.33.0

Re: [PATCH RESEND V2 net 2/7] net: hns3: fix missing features due to dev->features configuration too early

[Michal Swiatkowski]

On Tue, Dec 17, 2024 at 09:08:34AM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@huawei.com>
> 
> Currently, the netdev->features is configured in hns3_nic_set_features.
> As a result, __netdev_update_features considers that there is no feature
> difference, and the procedures of the real features are missing.
> 
> Fixes: 2a7556bb2b73 ("net: hns3: implement ndo_features_check ops for hns3 driver")
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 43377a7b2426..a7e3b22f641c 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -2452,7 +2452,6 @@ static int hns3_nic_set_features(struct net_device *netdev,
>  			return ret;
>  	}
>  
> -	netdev->features = features;
>  	return 0;

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Thanks

>  }
>  
> -- 
> 2.33.0

[PATCH RESEND V2 net 3/7] net: hns3: Resolved the issue that the debugfs query result is inconsistent.

[Jijie Shao]

From: Hao Lan <lanhao@huawei.com>

This patch modifies the implementation of debugfs:

When the user process stops unexpectedly, not all data of the file system
is read. In this case, the save_buf pointer is not released. When the
user process is called next time, save_buf is used to copy the cached
data to the user space. As a result, the queried data is stale.

To solve this problem, this patch implements .open() and .release() handler
for debugfs file_operations. moving allocation buffer and execution
of the cmd to the .open() handler and freeing in to the .release() handler.
Allocate separate buffer for each reader and associate the buffer
with the file pointer.
When different user read processes no longer share the buffer,
the stale data problem is fixed.

Fixes: 5e69ea7ee2a6 ("net: hns3: refactor the debugfs process")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Guangwei Zhang <zhangwangwei6@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
ChangeLog:
v1 -> v2:
  - Fix a data inconsistency issue caused by simultaneous access of multiple readers,
    suggested by Jakub.
  v1: https://lore.kernel.org/all/20241107133023.3813095-1-shaojijie@huawei.com/
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h   |  3 -
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 96 ++++++-------------
 2 files changed, 31 insertions(+), 68 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 710a8f9f2248..12ba380eb701 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -916,9 +916,6 @@ struct hnae3_handle {
 
 	u8 netdev_flags;
 	struct dentry *hnae3_dbgfs;
-	/* protects concurrent contention between debugfs commands */
-	struct mutex dbgfs_lock;
-	char **dbgfs_buf;
 
 	/* Network interface message level enabled bits */
 	u32 msg_enable;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 807eb3bbb11c..9bbece25552b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -1260,69 +1260,55 @@ static int hns3_dbg_read_cmd(struct hns3_dbg_data *dbg_data,
 static ssize_t hns3_dbg_read(struct file *filp, char __user *buffer,
 			     size_t count, loff_t *ppos)
 {
-	struct hns3_dbg_data *dbg_data = filp->private_data;
+	char *buf = filp->private_data;
+
+	return simple_read_from_buffer(buffer, count, ppos, buf, strlen(buf));
+}
+
+static int hns3_dbg_open(struct inode *inode, struct file *filp)
+{
+	struct hns3_dbg_data *dbg_data = inode->i_private;
 	struct hnae3_handle *handle = dbg_data->handle;
 	struct hns3_nic_priv *priv = handle->priv;
-	ssize_t size = 0;
-	char **save_buf;
-	char *read_buf;
 	u32 index;
+	char *buf;
 	int ret;
 
+	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
+	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+		return -EBUSY;
+
 	ret = hns3_dbg_get_cmd_index(dbg_data, &index);
 	if (ret)
 		return ret;
 
-	mutex_lock(&handle->dbgfs_lock);
-	save_buf = &handle->dbgfs_buf[index];
-
-	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state)) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (*save_buf) {
-		read_buf = *save_buf;
-	} else {
-		read_buf = kvzalloc(hns3_dbg_cmd[index].buf_len, GFP_KERNEL);
-		if (!read_buf) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		/* save the buffer addr until the last read operation */
-		*save_buf = read_buf;
-
-		/* get data ready for the first time to read */
-		ret = hns3_dbg_read_cmd(dbg_data, hns3_dbg_cmd[index].cmd,
-					read_buf, hns3_dbg_cmd[index].buf_len);
-		if (ret)
-			goto out;
-	}
+	buf = kvzalloc(hns3_dbg_cmd[index].buf_len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
-	size = simple_read_from_buffer(buffer, count, ppos, read_buf,
-				       strlen(read_buf));
-	if (size > 0) {
-		mutex_unlock(&handle->dbgfs_lock);
-		return size;
+	ret = hns3_dbg_read_cmd(dbg_data, hns3_dbg_cmd[index].cmd,
+				buf, hns3_dbg_cmd[index].buf_len);
+	if (ret) {
+		kvfree(buf);
+		return ret;
 	}
 
-out:
-	/* free the buffer for the last read operation */
-	if (*save_buf) {
-		kvfree(*save_buf);
-		*save_buf = NULL;
-	}
+	filp->private_data = buf;
+	return 0;
+}
 
-	mutex_unlock(&handle->dbgfs_lock);
-	return ret;
+static int hns3_dbg_release(struct inode *inode, struct file *filp)
+{
+	kvfree(filp->private_data);
+	filp->private_data = NULL;
+	return 0;
 }
 
 static const struct file_operations hns3_dbg_fops = {
 	.owner = THIS_MODULE,
-	.open  = simple_open,
+	.open  = hns3_dbg_open,
 	.read  = hns3_dbg_read,
+	.release = hns3_dbg_release,
 };
 
 static int hns3_dbg_bd_file_init(struct hnae3_handle *handle, u32 cmd)
@@ -1379,13 +1365,6 @@ int hns3_dbg_init(struct hnae3_handle *handle)
 	int ret;
 	u32 i;
 
-	handle->dbgfs_buf = devm_kcalloc(&handle->pdev->dev,
-					 ARRAY_SIZE(hns3_dbg_cmd),
-					 sizeof(*handle->dbgfs_buf),
-					 GFP_KERNEL);
-	if (!handle->dbgfs_buf)
-		return -ENOMEM;
-
 	hns3_dbg_dentry[HNS3_DBG_DENTRY_COMMON].dentry =
 				debugfs_create_dir(name, hns3_dbgfs_root);
 	handle->hnae3_dbgfs = hns3_dbg_dentry[HNS3_DBG_DENTRY_COMMON].dentry;
@@ -1395,8 +1374,6 @@ int hns3_dbg_init(struct hnae3_handle *handle)
 			debugfs_create_dir(hns3_dbg_dentry[i].name,
 					   handle->hnae3_dbgfs);
 
-	mutex_init(&handle->dbgfs_lock);
-
 	for (i = 0; i < ARRAY_SIZE(hns3_dbg_cmd); i++) {
 		if ((hns3_dbg_cmd[i].cmd == HNAE3_DBG_CMD_TM_NODES &&
 		     ae_dev->dev_version <= HNAE3_DEVICE_VERSION_V2) ||
@@ -1425,24 +1402,13 @@ int hns3_dbg_init(struct hnae3_handle *handle)
 out:
 	debugfs_remove_recursive(handle->hnae3_dbgfs);
 	handle->hnae3_dbgfs = NULL;
-	mutex_destroy(&handle->dbgfs_lock);
 	return ret;
 }
 
 void hns3_dbg_uninit(struct hnae3_handle *handle)
 {
-	u32 i;
-
 	debugfs_remove_recursive(handle->hnae3_dbgfs);
 	handle->hnae3_dbgfs = NULL;
-
-	for (i = 0; i < ARRAY_SIZE(hns3_dbg_cmd); i++)
-		if (handle->dbgfs_buf[i]) {
-			kvfree(handle->dbgfs_buf[i]);
-			handle->dbgfs_buf[i] = NULL;
-		}
-
-	mutex_destroy(&handle->dbgfs_lock);
 }
 
 void hns3_dbg_register_debugfs(const char *debugfs_dir_name)
-- 
2.33.0

[PATCH RESEND V2 net 4/7] net: hns3: don't auto enable misc vector

[Jijie Shao]

From: Jian Shen <shenjian15@huawei.com>

Currently, there is a time window between misc irq enabled
and service task inited. If an interrupte is reported at
this time, it will cause warning like below:

[   16.324639] Call trace:
[   16.324641]  __queue_delayed_work+0xb8/0xe0
[   16.324643]  mod_delayed_work_on+0x78/0xd0
[   16.324655]  hclge_errhand_task_schedule+0x58/0x90 [hclge]
[   16.324662]  hclge_misc_irq_handle+0x168/0x240 [hclge]
[   16.324666]  __handle_irq_event_percpu+0x64/0x1e0
[   16.324667]  handle_irq_event+0x80/0x170
[   16.324670]  handle_fasteoi_edge_irq+0x110/0x2bc
[   16.324671]  __handle_domain_irq+0x84/0xfc
[   16.324673]  gic_handle_irq+0x88/0x2c0
[   16.324674]  el1_irq+0xb8/0x140
[   16.324677]  arch_cpu_idle+0x18/0x40
[   16.324679]  default_idle_call+0x5c/0x1bc
[   16.324682]  cpuidle_idle_call+0x18c/0x1c4
[   16.324684]  do_idle+0x174/0x17c
[   16.324685]  cpu_startup_entry+0x30/0x6c
[   16.324687]  secondary_start_kernel+0x1a4/0x280
[   16.324688] ---[ end trace 6aa0bff672a964aa ]---

So don't auto enable misc vector when request irq..

Fixes: 7be1b9f3e99f ("net: hns3: make hclge_service use delayed workqueue")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c  | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 7d44dc777dc5..db7845009252 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -6,6 +6,7 @@
 #include <linux/etherdevice.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -3770,7 +3771,7 @@ static int hclge_misc_irq_init(struct hclge_dev *hdev)
 	snprintf(hdev->misc_vector.name, HNAE3_INT_NAME_LEN, "%s-misc-%s",
 		 HCLGE_NAME, pci_name(hdev->pdev));
 	ret = request_irq(hdev->misc_vector.vector_irq, hclge_misc_irq_handle,
-			  0, hdev->misc_vector.name, hdev);
+			  IRQF_NO_AUTOEN, hdev->misc_vector.name, hdev);
 	if (ret) {
 		hclge_free_vector(hdev, 0);
 		dev_err(&hdev->pdev->dev, "request misc irq(%d) fail\n",
@@ -11906,9 +11907,6 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 
 	hclge_init_rxd_adv_layout(hdev);
 
-	/* Enable MISC vector(vector0) */
-	hclge_enable_vector(&hdev->misc_vector, true);
-
 	ret = hclge_init_wol(hdev);
 	if (ret)
 		dev_warn(&pdev->dev,
@@ -11921,6 +11919,10 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 	hclge_state_init(hdev);
 	hdev->last_reset_time = jiffies;
 
+	/* Enable MISC vector(vector0) */
+	enable_irq(hdev->misc_vector.vector_irq);
+	hclge_enable_vector(&hdev->misc_vector, true);
+
 	dev_info(&hdev->pdev->dev, "%s driver initialization finished.\n",
 		 HCLGE_DRIVER_NAME);
 
@@ -12326,7 +12328,7 @@ static void hclge_uninit_ae_dev(struct hnae3_ae_dev *ae_dev)
 
 	/* Disable MISC vector(vector0) */
 	hclge_enable_vector(&hdev->misc_vector, false);
-	synchronize_irq(hdev->misc_vector.vector_irq);
+	disable_irq(hdev->misc_vector.vector_irq);
 
 	/* Disable all hw interrupts */
 	hclge_config_mac_tnl_int(hdev, false);
-- 
2.33.0

[PATCH RESEND V2 net 5/7] net: hns3: initialize reset_timer before hclgevf_misc_irq_init()

[Jijie Shao]

From: Jian Shen <shenjian15@huawei.com>

Currently the misc irq is initialized before reset_timer setup. But
it will access the reset_timer in the irq handler. So initialize
the reset_timer earlier.

Fixes: ff200099d271 ("net: hns3: remove unnecessary work in hclgevf_main")
Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.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 fd0abe37fdd7..8739da317897 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -2313,6 +2313,7 @@ static void hclgevf_state_init(struct hclgevf_dev *hdev)
 	clear_bit(HCLGEVF_STATE_RST_FAIL, &hdev->state);
 
 	INIT_DELAYED_WORK(&hdev->service_task, hclgevf_service_task);
+	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
 
 	mutex_init(&hdev->mbx_resp.mbx_mutex);
 	sema_init(&hdev->reset_sem, 1);
@@ -3012,7 +3013,6 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
 		 HCLGEVF_DRIVER_NAME);
 
 	hclgevf_task_schedule(hdev, round_jiffies_relative(HZ));
-	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
 
 	return 0;
 
-- 
2.33.0

Re: [PATCH RESEND V2 net 5/7] net: hns3: initialize reset_timer before hclgevf_misc_irq_init()

[Michal Swiatkowski]

On Tue, Dec 17, 2024 at 09:08:37AM +0800, Jijie Shao wrote:
> From: Jian Shen <shenjian15@huawei.com>
> 
> Currently the misc irq is initialized before reset_timer setup. But
> it will access the reset_timer in the irq handler. So initialize
> the reset_timer earlier.
> 
> Fixes: ff200099d271 ("net: hns3: remove unnecessary work in hclgevf_main")
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.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 fd0abe37fdd7..8739da317897 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
> @@ -2313,6 +2313,7 @@ static void hclgevf_state_init(struct hclgevf_dev *hdev)
>  	clear_bit(HCLGEVF_STATE_RST_FAIL, &hdev->state);
>  
>  	INIT_DELAYED_WORK(&hdev->service_task, hclgevf_service_task);
Comment here that timer needs to be initialized before misc irq will be
nice, but that is onlu my impression.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Thanks
> +	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
>  
>  	mutex_init(&hdev->mbx_resp.mbx_mutex);
>  	sema_init(&hdev->reset_sem, 1);
> @@ -3012,7 +3013,6 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
>  		 HCLGEVF_DRIVER_NAME);
>  
>  	hclgevf_task_schedule(hdev, round_jiffies_relative(HZ));
> -	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
>  
>  	return 0;
>  
> -- 
> 2.33.0

Re: [PATCH RESEND V2 net 5/7] net: hns3: initialize reset_timer before hclgevf_misc_irq_init()

[Jijie Shao]

on 2024/12/18 17:20, Michal Swiatkowski wrote:
> On Tue, Dec 17, 2024 at 09:08:37AM +0800, Jijie Shao wrote:
>> From: Jian Shen <shenjian15@huawei.com>
>>
>> Currently the misc irq is initialized before reset_timer setup. But
>> it will access the reset_timer in the irq handler. So initialize
>> the reset_timer earlier.
>>
>> Fixes: ff200099d271 ("net: hns3: remove unnecessary work in hclgevf_main")
>> Signed-off-by: Jian Shen <shenjian15@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.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 fd0abe37fdd7..8739da317897 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
>> @@ -2313,6 +2313,7 @@ static void hclgevf_state_init(struct hclgevf_dev *hdev)
>>   	clear_bit(HCLGEVF_STATE_RST_FAIL, &hdev->state);
>>   
>>   	INIT_DELAYED_WORK(&hdev->service_task, hclgevf_service_task);
> Comment here that timer needs to be initialized before misc irq will be
> nice, but that is onlu my impression.


I'll add a comment in the next version.

Thanks,
Jijie Shao

>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>
> Thanks
>> +	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
>>   
>>   	mutex_init(&hdev->mbx_resp.mbx_mutex);
>>   	sema_init(&hdev->reset_sem, 1);
>> @@ -3012,7 +3013,6 @@ static int hclgevf_init_hdev(struct hclgevf_dev *hdev)
>>   		 HCLGEVF_DRIVER_NAME);
>>   
>>   	hclgevf_task_schedule(hdev, round_jiffies_relative(HZ));
>> -	timer_setup(&hdev->reset_timer, hclgevf_reset_timer, 0);
>>   
>>   	return 0;
>>   
>> -- 
>> 2.33.0

[PATCH RESEND V2 net 6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

[Jijie Shao]

From: Hao Lan <lanhao@huawei.com>

The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
1024-1279 are in different BAR space addresses. However,
hclge_fetch_pf_reg does not distinguish the tqp space information when
reading the tqp space information. When the number of TQPs is greater
than 1024, access bar space overwriting occurs.
The problem of different segments has been considered during the
initialization of tqp.io_base. Therefore, tqp.io_base is directly used
when the queue is read in hclge_fetch_pf_reg.

The error message:

Unable to handle kernel paging request at virtual address ffff800037200000
pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
lr : hclge_get_regs+0x84/0x1d0 [hclge]
Call trace:
 hclge_fetch_pf_reg+0x138/0x250 [hclge]
 hclge_get_regs+0x84/0x1d0 [hclge]
 hns3_get_regs+0x2c/0x50 [hns3]
 ethtool_get_regs+0xf4/0x270
 dev_ethtool+0x674/0x8a0
 dev_ioctl+0x270/0x36c
 sock_do_ioctl+0x110/0x2a0
 sock_ioctl+0x2ac/0x530
 __arm64_sys_ioctl+0xa8/0x100
 invoke_syscall+0x4c/0x124
 el0_svc_common.constprop.0+0x140/0x15c
 do_el0_svc+0x30/0xd0
 el0_svc+0x1c/0x2c
 el0_sync_handler+0xb0/0xb4
 el0_sync+0x168/0x180

Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
Signed-off-by: Hao Lan <lanhao@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
 .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
index 43c1c18fa81f..8c057192aae6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
@@ -510,9 +510,9 @@ static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
 static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
 			      struct hnae3_knic_private_info *kinfo)
 {
-#define HCLGE_RING_REG_OFFSET		0x200
 #define HCLGE_RING_INT_REG_OFFSET	0x4
 
+	struct hnae3_queue *tqp;
 	int i, j, reg_num;
 	int data_num_sum;
 	u32 *reg = data;
@@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
 	reg_num = ARRAY_SIZE(ring_reg_addr_list);
 	for (j = 0; j < kinfo->num_tqps; j++) {
 		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
+		tqp = kinfo->tqp[j];
 		for (i = 0; i < reg_num; i++)
-			*reg++ = hclge_read_dev(&hdev->hw,
-						ring_reg_addr_list[i] +
-						HCLGE_RING_REG_OFFSET * j);
+			*reg++ = readl_relaxed(tqp->io_base -
+					       HCLGE_TQP_REG_OFFSET +
+					       ring_reg_addr_list[i]);
 	}
 	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
index 6db415d8b917..7d9d9dbc7560 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
@@ -123,10 +123,10 @@ int hclgevf_get_regs_len(struct hnae3_handle *handle)
 void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
 		      void *data)
 {
-#define HCLGEVF_RING_REG_OFFSET		0x200
 #define HCLGEVF_RING_INT_REG_OFFSET	0x4
 
 	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
+	struct hnae3_queue *tqp;
 	int i, j, reg_um;
 	u32 *reg = data;
 
@@ -147,10 +147,11 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
 	reg_um = 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);
+		tqp = &hdev->htqp[j].q;
 		for (i = 0; i < reg_um; i++)
-			*reg++ = hclgevf_read_dev(&hdev->hw,
-						  ring_reg_addr_list[i] +
-						  HCLGEVF_RING_REG_OFFSET * j);
+			*reg++ = readl_relaxed(tqp->io_base -
+					       HCLGEVF_TQP_REG_OFFSET +
+					       ring_reg_addr_list[i]);
 	}
 
 	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
-- 
2.33.0

Re: [PATCH RESEND V2 net 6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

[Michal Swiatkowski]

On Tue, Dec 17, 2024 at 09:08:38AM +0800, Jijie Shao wrote:
> From: Hao Lan <lanhao@huawei.com>
> 
> The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
> 1024-1279 are in different BAR space addresses. However,
> hclge_fetch_pf_reg does not distinguish the tqp space information when
> reading the tqp space information. When the number of TQPs is greater
> than 1024, access bar space overwriting occurs.
> The problem of different segments has been considered during the
> initialization of tqp.io_base. Therefore, tqp.io_base is directly used
> when the queue is read in hclge_fetch_pf_reg.
> 
> The error message:
> 
> Unable to handle kernel paging request at virtual address ffff800037200000
> pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
> lr : hclge_get_regs+0x84/0x1d0 [hclge]
> Call trace:
>  hclge_fetch_pf_reg+0x138/0x250 [hclge]
>  hclge_get_regs+0x84/0x1d0 [hclge]
>  hns3_get_regs+0x2c/0x50 [hns3]
>  ethtool_get_regs+0xf4/0x270
>  dev_ethtool+0x674/0x8a0
>  dev_ioctl+0x270/0x36c
>  sock_do_ioctl+0x110/0x2a0
>  sock_ioctl+0x2ac/0x530
>  __arm64_sys_ioctl+0xa8/0x100
>  invoke_syscall+0x4c/0x124
>  el0_svc_common.constprop.0+0x140/0x15c
>  do_el0_svc+0x30/0xd0
>  el0_svc+0x1c/0x2c
>  el0_sync_handler+0xb0/0xb4
>  el0_sync+0x168/0x180
> 
> Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
> Signed-off-by: Hao Lan <lanhao@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
>  .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> index 43c1c18fa81f..8c057192aae6 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
> @@ -510,9 +510,9 @@ static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
>  static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>  			      struct hnae3_knic_private_info *kinfo)
>  {
> -#define HCLGE_RING_REG_OFFSET		0x200
>  #define HCLGE_RING_INT_REG_OFFSET	0x4
>  
> +	struct hnae3_queue *tqp;
>  	int i, j, reg_num;
>  	int data_num_sum;
>  	u32 *reg = data;
> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>  	for (j = 0; j < kinfo->num_tqps; j++) {
You can define struct hnae3_queue *tqp here to limit the scope
(same in VF case).
>  		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
> +		tqp = kinfo->tqp[j];
>  		for (i = 0; i < reg_num; i++)
> -			*reg++ = hclge_read_dev(&hdev->hw,
> -						ring_reg_addr_list[i] +
> -						HCLGE_RING_REG_OFFSET * j);
> +			*reg++ = readl_relaxed(tqp->io_base -
> +					       HCLGE_TQP_REG_OFFSET +
> +					       ring_reg_addr_list[i]);
>  	}
>  	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> index 6db415d8b917..7d9d9dbc7560 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
> @@ -123,10 +123,10 @@ int hclgevf_get_regs_len(struct hnae3_handle *handle)
>  void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>  		      void *data)
>  {
> -#define HCLGEVF_RING_REG_OFFSET		0x200
>  #define HCLGEVF_RING_INT_REG_OFFSET	0x4
>  
>  	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
> +	struct hnae3_queue *tqp;
>  	int i, j, reg_um;
>  	u32 *reg = data;
>  
> @@ -147,10 +147,11 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>  	reg_um = 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);
> +		tqp = &hdev->htqp[j].q;
>  		for (i = 0; i < reg_um; i++)
> -			*reg++ = hclgevf_read_dev(&hdev->hw,
> -						  ring_reg_addr_list[i] +
> -						  HCLGEVF_RING_REG_OFFSET * j);
> +			*reg++ = readl_relaxed(tqp->io_base -
> +					       HCLGEVF_TQP_REG_OFFSET +
> +					       ring_reg_addr_list[i]);
>  	}
>  
>  	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
> -- 
> 2.33.0

Re: [PATCH RESEND V2 net 6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

[Paolo Abeni]

On 12/18/24 10:29, Michal Swiatkowski wrote:
>> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>>  	for (j = 0; j < kinfo->num_tqps; j++) {
> You can define struct hnae3_queue *tqp here to limit the scope
> (same in VF case).

@Michal, please let me refer to prior feedback from Jakub:

https://lore.kernel.org/netdev/20241028163554.7dddff8b@kernel.org/

I also agree subjective stylistic feedback should be avoided unless the
style issue is really gross - in such a case the feedback should not be
subjective, so the original guidance still applies ;)

Thanks,

Paolo

Re: [PATCH RESEND V2 net 6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

[Michal Swiatkowski]

On Thu, Dec 19, 2024 at 10:51:08AM +0100, Paolo Abeni wrote:
> On 12/18/24 10:29, Michal Swiatkowski wrote:
> >> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
> >>  	reg_num = ARRAY_SIZE(ring_reg_addr_list);
> >>  	for (j = 0; j < kinfo->num_tqps; j++) {
> > You can define struct hnae3_queue *tqp here to limit the scope
> > (same in VF case).
> 
> @Michal, please let me refer to prior feedback from Jakub:
> 
> https://lore.kernel.org/netdev/20241028163554.7dddff8b@kernel.org/
> 
> I also agree subjective stylistic feedback should be avoided unless the
> style issue is really gross - in such a case the feedback should not be
> subjective, so the original guidance still applies ;)
> 

Sure, I thought sometimes there were a feedback about scoping, but maybe
not from the maintainers. I will drop such comments next time, thanks
for letting me know.

Side note is that by "You can define" I meant if you want, if you feel
so, not you have to (sorry, not a native speaker).
But I understand that this unnecessary slow down the process when there
is no other (valid) changes to do, so I won't do that next time.

Thanks

> Thanks,
> 
> Paolo
> 

Re: [PATCH RESEND V2 net 6/7] net: hns3: fixed hclge_fetch_pf_reg accesses bar space out of bounds issue

[Jijie Shao]

on 2024/12/18 17:29, Michal Swiatkowski wrote:
> On Tue, Dec 17, 2024 at 09:08:38AM +0800, Jijie Shao wrote:
>> From: Hao Lan <lanhao@huawei.com>
>>
>> The TQP BAR space is divided into two segments. TQPs 0-1023 and TQPs
>> 1024-1279 are in different BAR space addresses. However,
>> hclge_fetch_pf_reg does not distinguish the tqp space information when
>> reading the tqp space information. When the number of TQPs is greater
>> than 1024, access bar space overwriting occurs.
>> The problem of different segments has been considered during the
>> initialization of tqp.io_base. Therefore, tqp.io_base is directly used
>> when the queue is read in hclge_fetch_pf_reg.
>>
>> The error message:
>>
>> Unable to handle kernel paging request at virtual address ffff800037200000
>> pc : hclge_fetch_pf_reg+0x138/0x250 [hclge]
>> lr : hclge_get_regs+0x84/0x1d0 [hclge]
>> Call trace:
>>   hclge_fetch_pf_reg+0x138/0x250 [hclge]
>>   hclge_get_regs+0x84/0x1d0 [hclge]
>>   hns3_get_regs+0x2c/0x50 [hns3]
>>   ethtool_get_regs+0xf4/0x270
>>   dev_ethtool+0x674/0x8a0
>>   dev_ioctl+0x270/0x36c
>>   sock_do_ioctl+0x110/0x2a0
>>   sock_ioctl+0x2ac/0x530
>>   __arm64_sys_ioctl+0xa8/0x100
>>   invoke_syscall+0x4c/0x124
>>   el0_svc_common.constprop.0+0x140/0x15c
>>   do_el0_svc+0x30/0xd0
>>   el0_svc+0x1c/0x2c
>>   el0_sync_handler+0xb0/0xb4
>>   el0_sync+0x168/0x180
>>
>> Fixes: 939ccd107ffc ("net: hns3: move dump regs function to a separate file")
>> Signed-off-by: Hao Lan <lanhao@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c  | 9 +++++----
>>   .../net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c    | 9 +++++----
>>   2 files changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> index 43c1c18fa81f..8c057192aae6 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_regs.c
>> @@ -510,9 +510,9 @@ static int hclge_get_dfx_reg(struct hclge_dev *hdev, void *data)
>>   static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>   			      struct hnae3_knic_private_info *kinfo)
>>   {
>> -#define HCLGE_RING_REG_OFFSET		0x200
>>   #define HCLGE_RING_INT_REG_OFFSET	0x4
>>   
>> +	struct hnae3_queue *tqp;
>>   	int i, j, reg_num;
>>   	int data_num_sum;
>>   	u32 *reg = data;
>> @@ -533,10 +533,11 @@ static int hclge_fetch_pf_reg(struct hclge_dev *hdev, void *data,
>>   	reg_num = ARRAY_SIZE(ring_reg_addr_list);
>>   	for (j = 0; j < kinfo->num_tqps; j++) {
> You can define struct hnae3_queue *tqp here to limit the scope
> (same in VF case).

Hi:

To keep consistent with other code styles of the driver, this may not be changed.

Thank you.
Jijie Shao

>>   		reg += hclge_reg_get_tlv(HCLGE_REG_TAG_RING, reg_num, reg);
>> +		tqp = kinfo->tqp[j];
>>   		for (i = 0; i < reg_num; i++)
>> -			*reg++ = hclge_read_dev(&hdev->hw,
>> -						ring_reg_addr_list[i] +
>> -						HCLGE_RING_REG_OFFSET * j);
>> +			*reg++ = readl_relaxed(tqp->io_base -
>> +					       HCLGE_TQP_REG_OFFSET +
>> +					       ring_reg_addr_list[i]);
>>   	}
>>   	data_num_sum += (reg_num + HCLGE_REG_TLV_SPACE) * kinfo->num_tqps;
>>   
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> index 6db415d8b917..7d9d9dbc7560 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_regs.c
>> @@ -123,10 +123,10 @@ int hclgevf_get_regs_len(struct hnae3_handle *handle)
>>   void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>>   		      void *data)
>>   {
>> -#define HCLGEVF_RING_REG_OFFSET		0x200
>>   #define HCLGEVF_RING_INT_REG_OFFSET	0x4
>>   
>>   	struct hclgevf_dev *hdev = hclgevf_ae_get_hdev(handle);
>> +	struct hnae3_queue *tqp;
>>   	int i, j, reg_um;
>>   	u32 *reg = data;
>>   
>> @@ -147,10 +147,11 @@ void hclgevf_get_regs(struct hnae3_handle *handle, u32 *version,
>>   	reg_um = 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);
>> +		tqp = &hdev->htqp[j].q;
>>   		for (i = 0; i < reg_um; i++)
>> -			*reg++ = hclgevf_read_dev(&hdev->hw,
>> -						  ring_reg_addr_list[i] +
>> -						  HCLGEVF_RING_REG_OFFSET * j);
>> +			*reg++ = readl_relaxed(tqp->io_base -
>> +					       HCLGEVF_TQP_REG_OFFSET +
>> +					       ring_reg_addr_list[i]);
>>   	}
>>   
>>   	reg_um = ARRAY_SIZE(tqp_intr_reg_addr_list);
>> -- 
>> 2.33.0

[PATCH RESEND V2 net 7/7] net: hns3: fix kernel crash when 1588 is sent on HIP08 devices

[Jijie Shao]

From: Jie Wang <wangjie125@huawei.com>

Currently, HIP08 devices does not register the ptp devices, so the
hdev->ptp is NULL. But the tx process would still try to set hardware time
stamp info with SKBTX_HW_TSTAMP flag and cause a kernel crash.

[  128.087798] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
...
[  128.280251] pc : hclge_ptp_set_tx_info+0x2c/0x140 [hclge]
[  128.286600] lr : hclge_ptp_set_tx_info+0x20/0x140 [hclge]
[  128.292938] sp : ffff800059b93140
[  128.297200] x29: ffff800059b93140 x28: 0000000000003280
[  128.303455] x27: ffff800020d48280 x26: ffff0cb9dc814080
[  128.309715] x25: ffff0cb9cde93fa0 x24: 0000000000000001
[  128.315969] x23: 0000000000000000 x22: 0000000000000194
[  128.322219] x21: ffff0cd94f986000 x20: 0000000000000000
[  128.328462] x19: ffff0cb9d2a166c0 x18: 0000000000000000
[  128.334698] x17: 0000000000000000 x16: ffffcf1fc523ed24
[  128.340934] x15: 0000ffffd530a518 x14: 0000000000000000
[  128.347162] x13: ffff0cd6bdb31310 x12: 0000000000000368
[  128.353388] x11: ffff0cb9cfbc7070 x10: ffff2cf55dd11e02
[  128.359606] x9 : ffffcf1f85a212b4 x8 : ffff0cd7cf27dab0
[  128.365831] x7 : 0000000000000a20 x6 : ffff0cd7cf27d000
[  128.372040] x5 : 0000000000000000 x4 : 000000000000ffff
[  128.378243] x3 : 0000000000000400 x2 : ffffcf1f85a21294
[  128.384437] x1 : ffff0cb9db520080 x0 : ffff0cb9db500080
[  128.390626] Call trace:
[  128.393964]  hclge_ptp_set_tx_info+0x2c/0x140 [hclge]
[  128.399893]  hns3_nic_net_xmit+0x39c/0x4c4 [hns3]
[  128.405468]  xmit_one.constprop.0+0xc4/0x200
[  128.410600]  dev_hard_start_xmit+0x54/0xf0
[  128.415556]  sch_direct_xmit+0xe8/0x634
[  128.420246]  __dev_queue_xmit+0x224/0xc70
[  128.425101]  dev_queue_xmit+0x1c/0x40
[  128.429608]  ovs_vport_send+0xac/0x1a0 [openvswitch]
[  128.435409]  do_output+0x60/0x17c [openvswitch]
[  128.440770]  do_execute_actions+0x898/0x8c4 [openvswitch]
[  128.446993]  ovs_execute_actions+0x64/0xf0 [openvswitch]
[  128.453129]  ovs_dp_process_packet+0xa0/0x224 [openvswitch]
[  128.459530]  ovs_vport_receive+0x7c/0xfc [openvswitch]
[  128.465497]  internal_dev_xmit+0x34/0xb0 [openvswitch]
[  128.471460]  xmit_one.constprop.0+0xc4/0x200
[  128.476561]  dev_hard_start_xmit+0x54/0xf0
[  128.481489]  __dev_queue_xmit+0x968/0xc70
[  128.486330]  dev_queue_xmit+0x1c/0x40
[  128.490856]  ip_finish_output2+0x250/0x570
[  128.495810]  __ip_finish_output+0x170/0x1e0
[  128.500832]  ip_finish_output+0x3c/0xf0
[  128.505504]  ip_output+0xbc/0x160
[  128.509654]  ip_send_skb+0x58/0xd4
[  128.513892]  udp_send_skb+0x12c/0x354
[  128.518387]  udp_sendmsg+0x7a8/0x9c0
[  128.522793]  inet_sendmsg+0x4c/0x8c
[  128.527116]  __sock_sendmsg+0x48/0x80
[  128.531609]  __sys_sendto+0x124/0x164
[  128.536099]  __arm64_sys_sendto+0x30/0x5c
[  128.540935]  invoke_syscall+0x50/0x130
[  128.545508]  el0_svc_common.constprop.0+0x10c/0x124
[  128.551205]  do_el0_svc+0x34/0xdc
[  128.555347]  el0_svc+0x20/0x30
[  128.559227]  el0_sync_handler+0xb8/0xc0
[  128.563883]  el0_sync+0x160/0x180

Fixes: 0bf5eb788512 ("net: hns3: add support for PTP")
Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
index 5505caea88e9..bab16c2191b2 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
@@ -58,6 +58,9 @@ bool hclge_ptp_set_tx_info(struct hnae3_handle *handle, struct sk_buff *skb)
 	struct hclge_dev *hdev = vport->back;
 	struct hclge_ptp *ptp = hdev->ptp;
 
+	if (!ptp)
+		return false;
+
 	if (!test_bit(HCLGE_PTP_FLAG_TX_EN, &ptp->flags) ||
 	    test_and_set_bit(HCLGE_STATE_PTP_TX_HANDLING, &hdev->state)) {
 		ptp->tx_skipped++;
-- 
2.33.0

Re: [PATCH RESEND V2 net 7/7] net: hns3: fix kernel crash when 1588 is sent on HIP08 devices

[Michal Swiatkowski]

On Tue, Dec 17, 2024 at 09:08:39AM +0800, Jijie Shao wrote:
> From: Jie Wang <wangjie125@huawei.com>
> 
> Currently, HIP08 devices does not register the ptp devices, so the
> hdev->ptp is NULL. But the tx process would still try to set hardware time
> stamp info with SKBTX_HW_TSTAMP flag and cause a kernel crash.
> 
> [  128.087798] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
> ...
> [  128.280251] pc : hclge_ptp_set_tx_info+0x2c/0x140 [hclge]
> [  128.286600] lr : hclge_ptp_set_tx_info+0x20/0x140 [hclge]
> [  128.292938] sp : ffff800059b93140
> [  128.297200] x29: ffff800059b93140 x28: 0000000000003280
> [  128.303455] x27: ffff800020d48280 x26: ffff0cb9dc814080
> [  128.309715] x25: ffff0cb9cde93fa0 x24: 0000000000000001
> [  128.315969] x23: 0000000000000000 x22: 0000000000000194
> [  128.322219] x21: ffff0cd94f986000 x20: 0000000000000000
> [  128.328462] x19: ffff0cb9d2a166c0 x18: 0000000000000000
> [  128.334698] x17: 0000000000000000 x16: ffffcf1fc523ed24
> [  128.340934] x15: 0000ffffd530a518 x14: 0000000000000000
> [  128.347162] x13: ffff0cd6bdb31310 x12: 0000000000000368
> [  128.353388] x11: ffff0cb9cfbc7070 x10: ffff2cf55dd11e02
> [  128.359606] x9 : ffffcf1f85a212b4 x8 : ffff0cd7cf27dab0
> [  128.365831] x7 : 0000000000000a20 x6 : ffff0cd7cf27d000
> [  128.372040] x5 : 0000000000000000 x4 : 000000000000ffff
> [  128.378243] x3 : 0000000000000400 x2 : ffffcf1f85a21294
> [  128.384437] x1 : ffff0cb9db520080 x0 : ffff0cb9db500080
> [  128.390626] Call trace:
> [  128.393964]  hclge_ptp_set_tx_info+0x2c/0x140 [hclge]
> [  128.399893]  hns3_nic_net_xmit+0x39c/0x4c4 [hns3]
> [  128.405468]  xmit_one.constprop.0+0xc4/0x200
> [  128.410600]  dev_hard_start_xmit+0x54/0xf0
> [  128.415556]  sch_direct_xmit+0xe8/0x634
> [  128.420246]  __dev_queue_xmit+0x224/0xc70
> [  128.425101]  dev_queue_xmit+0x1c/0x40
> [  128.429608]  ovs_vport_send+0xac/0x1a0 [openvswitch]
> [  128.435409]  do_output+0x60/0x17c [openvswitch]
> [  128.440770]  do_execute_actions+0x898/0x8c4 [openvswitch]
> [  128.446993]  ovs_execute_actions+0x64/0xf0 [openvswitch]
> [  128.453129]  ovs_dp_process_packet+0xa0/0x224 [openvswitch]
> [  128.459530]  ovs_vport_receive+0x7c/0xfc [openvswitch]
> [  128.465497]  internal_dev_xmit+0x34/0xb0 [openvswitch]
> [  128.471460]  xmit_one.constprop.0+0xc4/0x200
> [  128.476561]  dev_hard_start_xmit+0x54/0xf0
> [  128.481489]  __dev_queue_xmit+0x968/0xc70
> [  128.486330]  dev_queue_xmit+0x1c/0x40
> [  128.490856]  ip_finish_output2+0x250/0x570
> [  128.495810]  __ip_finish_output+0x170/0x1e0
> [  128.500832]  ip_finish_output+0x3c/0xf0
> [  128.505504]  ip_output+0xbc/0x160
> [  128.509654]  ip_send_skb+0x58/0xd4
> [  128.513892]  udp_send_skb+0x12c/0x354
> [  128.518387]  udp_sendmsg+0x7a8/0x9c0
> [  128.522793]  inet_sendmsg+0x4c/0x8c
> [  128.527116]  __sock_sendmsg+0x48/0x80
> [  128.531609]  __sys_sendto+0x124/0x164
> [  128.536099]  __arm64_sys_sendto+0x30/0x5c
> [  128.540935]  invoke_syscall+0x50/0x130
> [  128.545508]  el0_svc_common.constprop.0+0x10c/0x124
> [  128.551205]  do_el0_svc+0x34/0xdc
> [  128.555347]  el0_svc+0x20/0x30
> [  128.559227]  el0_sync_handler+0xb8/0xc0
> [  128.563883]  el0_sync+0x160/0x180
> 
> Fixes: 0bf5eb788512 ("net: hns3: add support for PTP")
> Signed-off-by: Jie Wang <wangjie125@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
> index 5505caea88e9..bab16c2191b2 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_ptp.c
> @@ -58,6 +58,9 @@ bool hclge_ptp_set_tx_info(struct hnae3_handle *handle, struct sk_buff *skb)
>  	struct hclge_dev *hdev = vport->back;
>  	struct hclge_ptp *ptp = hdev->ptp;
>  
> +	if (!ptp)
> +		return false;
> +
>  	if (!test_bit(HCLGE_PTP_FLAG_TX_EN, &ptp->flags) ||
>  	    test_and_set_bit(HCLGE_STATE_PTP_TX_HANDLING, &hdev->state)) {
>  		ptp->tx_skipped++;

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
Thanks

> -- 
> 2.33.0