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

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

[Jijie Shao]

This patchset includes 2 fixes:
1. Patch1 fixes an incorrect function return value.
2. Patch2 fixes the issue of lost reset concurrent protection after
  seq_file reconstruction in debugfs.

Note: Compared to the previous version, this patchset has removed 2 patches
and added 1 new patch, so it is being resent as V1.

previous patchset:
https://lore.kernel.org/all/20250917122954.1265844-4-shaojijie@huawei.com/
Patch 'use user configuration after hardware reset when using kernel PHY'
will be sent separately.

Jijie Shao (2):
  net: hns3: return error code when function fails
  net: hns3: fix null pointer in debugfs issue

 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 67 ++++++++++++++-----
 .../hisilicon/hns3/hns3pf/hclge_debugfs.c     |  6 ++
 .../hisilicon/hns3/hns3pf/hclge_main.c        |  3 +-
 .../hisilicon/hns3/hns3pf/hclge_mdio.c        |  9 ++-
 .../hisilicon/hns3/hns3pf/hclge_mdio.h        |  2 +-
 5 files changed, 65 insertions(+), 22 deletions(-)

-- 
2.33.0

[PATCH net 1/2] net: hns3: return error code when function fails

[Jijie Shao]

Currently, in hclge_mii_ioctl(), the operation to
read the PHY register (SIOCGMIIREG) always returns 0.

This patch changes the return type of hclge_read_phy_reg(),
returning an error code when the function fails.

Fixes: 024712f51e57 ("net: hns3: add ioctl support for imp-controlled PHYs")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 +--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c | 9 ++++++---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.h | 2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 9d34d28ff168..782bb48c9f3d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -9429,8 +9429,7 @@ static int hclge_mii_ioctl(struct hclge_dev *hdev, struct ifreq *ifr, int cmd)
 		/* this command reads phy id and register at the same time */
 		fallthrough;
 	case SIOCGMIIREG:
-		data->val_out = hclge_read_phy_reg(hdev, data->reg_num);
-		return 0;
+		return hclge_read_phy_reg(hdev, data->reg_num, &data->val_out);
 
 	case SIOCSMIIREG:
 		return hclge_write_phy_reg(hdev, data->reg_num, data->val_in);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
index 96553109f44c..cf881108fa57 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.c
@@ -274,7 +274,7 @@ void hclge_mac_stop_phy(struct hclge_dev *hdev)
 	phy_stop(phydev);
 }
 
-u16 hclge_read_phy_reg(struct hclge_dev *hdev, u16 reg_addr)
+int hclge_read_phy_reg(struct hclge_dev *hdev, u16 reg_addr, u16 *val)
 {
 	struct hclge_phy_reg_cmd *req;
 	struct hclge_desc desc;
@@ -286,11 +286,14 @@ u16 hclge_read_phy_reg(struct hclge_dev *hdev, u16 reg_addr)
 	req->reg_addr = cpu_to_le16(reg_addr);
 
 	ret = hclge_cmd_send(&hdev->hw, &desc, 1);
-	if (ret)
+	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"failed to read phy reg, ret = %d.\n", ret);
+		return ret;
+	}
 
-	return le16_to_cpu(req->reg_val);
+	*val = le16_to_cpu(req->reg_val);
+	return 0;
 }
 
 int hclge_write_phy_reg(struct hclge_dev *hdev, u16 reg_addr, u16 val)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.h
index 4200d0b6d931..21d434c82475 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mdio.h
@@ -13,7 +13,7 @@ int hclge_mac_connect_phy(struct hnae3_handle *handle);
 void hclge_mac_disconnect_phy(struct hnae3_handle *handle);
 void hclge_mac_start_phy(struct hclge_dev *hdev);
 void hclge_mac_stop_phy(struct hclge_dev *hdev);
-u16 hclge_read_phy_reg(struct hclge_dev *hdev, u16 reg_addr);
+int hclge_read_phy_reg(struct hclge_dev *hdev, u16 reg_addr, u16 *val);
 int hclge_write_phy_reg(struct hclge_dev *hdev, u16 reg_addr, u16 val);
 
 #endif
-- 
2.33.0

[PATCH net 2/2] net: hns3: fix null pointer in debugfs issue

[Jijie Shao]

Currently, when debugfs and reset are executed concurrently,
some resources are released during the reset process,
which may cause debugfs to read null pointers or other anomalies.

Therefore, in this patch, interception protection has been added
to debugfs operations that are sensitive to reset.

Fixes: eced3d1c41db ("net: hns3: use seq_file for files in queue/ in debugfs")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3_debugfs.c    | 67 ++++++++++++++-----
 .../hisilicon/hns3/hns3pf/hclge_debugfs.c     |  6 ++
 2 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
index 4cce4f4ba6b0..aa0f8a6cd9d6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
@@ -383,6 +383,15 @@ static const char * const dim_state_str[] = { "START", "IN_PROG", "APPLY" };
 static const char * const
 dim_tune_stat_str[] = { "ON_TOP", "TIRED", "RIGHT", "LEFT" };
 
+static bool hns3_dbg_is_device_busy(struct hns3_nic_priv *priv)
+{
+	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
+	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+		return true;
+
+	return false;
+}
+
 static void hns3_get_coal_info(struct hns3_enet_tqp_vector *tqp_vector,
 			       struct seq_file *s, int i, bool is_tx)
 {
@@ -428,13 +437,16 @@ static void hns3_get_coal_info(struct hns3_enet_tqp_vector *tqp_vector,
 	}
 }
 
-static void hns3_dump_coal_info(struct seq_file *s, bool is_tx)
+static int hns3_dump_coal_info(struct seq_file *s, bool is_tx)
 {
 	struct hnae3_handle *h = hnae3_seq_file_to_handle(s);
 	struct hns3_enet_tqp_vector *tqp_vector;
 	struct hns3_nic_priv *priv = h->priv;
 	unsigned int i;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	seq_printf(s, "%s interrupt coalesce info:\n", is_tx ? "tx" : "rx");
 
 	seq_puts(s, "VEC_ID  ALGO_STATE  PROFILE_ID  CQE_MODE  TUNE_STATE  ");
@@ -442,18 +454,26 @@ static void hns3_dump_coal_info(struct seq_file *s, bool is_tx)
 	seq_puts(s, "HW_GL  HW_QL\n");
 
 	for (i = 0; i < priv->vector_num; i++) {
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
+
 		tqp_vector = &priv->tqp_vector[i];
 		hns3_get_coal_info(tqp_vector, s, i, is_tx);
 	}
+
+	return 0;
 }
 
 static int hns3_dbg_coal_info(struct seq_file *s, void *data)
 {
-	hns3_dump_coal_info(s, true);
-	seq_puts(s, "\n");
-	hns3_dump_coal_info(s, false);
+	int ret;
 
-	return 0;
+	ret = hns3_dump_coal_info(s, true);
+	if (ret)
+		return ret;
+
+	seq_puts(s, "\n");
+	return hns3_dump_coal_info(s, false);
 }
 
 static void hns3_dump_rx_queue_info(struct hns3_enet_ring *ring,
@@ -498,6 +518,9 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	if (!priv->ring) {
 		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
 		return -EFAULT;
@@ -511,8 +534,7 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
 		 * to prevent reference to invalid memory. And need to ensure
 		 * that the following code is executed within 100ms.
 		 */
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+		if (hns3_dbg_is_device_busy(priv))
 			return -EPERM;
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
@@ -563,6 +585,9 @@ static int hns3_dbg_tx_queue_info(struct seq_file *s, void *data)
 	struct hns3_enet_ring *ring;
 	u32 i;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	if (!priv->ring) {
 		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
 		return -EFAULT;
@@ -576,8 +601,7 @@ static int hns3_dbg_tx_queue_info(struct seq_file *s, void *data)
 		 * to prevent reference to invalid memory. And need to ensure
 		 * that the following code is executed within 100ms.
 		 */
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+		if (hns3_dbg_is_device_busy(priv))
 			return -EPERM;
 
 		ring = &priv->ring[i];
@@ -596,6 +620,9 @@ static int hns3_dbg_queue_map(struct seq_file *s, void *data)
 	if (!h->ae_algo->ops->get_global_queue_id)
 		return -EOPNOTSUPP;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	seq_puts(s, "local_queue_id  global_queue_id  vector_id\n");
 
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
@@ -643,6 +670,9 @@ static int hns3_dbg_rx_bd_info(struct seq_file *s, void *private)
 	struct hns3_desc *desc;
 	unsigned int i;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	if (data->qid >= h->kinfo.num_tqps) {
 		dev_err(&h->pdev->dev, "queue%u is not in use\n", data->qid);
 		return -EINVAL;
@@ -655,8 +685,10 @@ static int hns3_dbg_rx_bd_info(struct seq_file *s, void *private)
 
 	ring = &priv->ring[data->qid + data->handle->kinfo.num_tqps];
 	for (i = 0; i < ring->desc_num; i++) {
-		desc = &ring->desc[i];
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_rx_bd_info(priv, desc, s, i);
 	}
 
@@ -688,6 +720,9 @@ static int hns3_dbg_tx_bd_info(struct seq_file *s, void *private)
 	struct hns3_desc *desc;
 	unsigned int i;
 
+	if (hns3_dbg_is_device_busy(priv))
+		return -EBUSY;
+
 	if (data->qid >= h->kinfo.num_tqps) {
 		dev_err(&h->pdev->dev, "queue%u is not in use\n", data->qid);
 		return -EINVAL;
@@ -700,8 +735,10 @@ static int hns3_dbg_tx_bd_info(struct seq_file *s, void *private)
 
 	ring = &priv->ring[data->qid];
 	for (i = 0; i < ring->desc_num; i++) {
-		desc = &ring->desc[i];
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
+		desc = &ring->desc[i];
 		hns3_dump_tx_bd_info(desc, s, i);
 	}
 
@@ -804,9 +841,8 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
 	seq_puts(s, "POOL_SIZE(PAGE_NUM)  ORDER  NUMA_ID  MAX_LEN\n");
 
 	for (i = 0; i < h->kinfo.num_tqps; i++) {
-		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
-			return -EPERM;
+		if (hns3_dbg_is_device_busy(priv))
+			return -EBUSY;
 
 		ring = &priv->ring[(u32)(i + h->kinfo.num_tqps)];
 		hns3_dump_page_pool_info(ring, s, i);
@@ -821,8 +857,7 @@ static int hns3_dbg_bd_info_show(struct seq_file *s, void *private)
 	struct hnae3_handle *h = data->handle;
 	struct hns3_nic_priv *priv = h->priv;
 
-	if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
-	    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
+	if (hns3_dbg_is_device_busy(priv))
 		return -EBUSY;
 
 	if (data->cmd == HNAE3_DBG_CMD_TX_BD)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
index b76d25074e99..b658077b9d50 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
@@ -2470,6 +2470,9 @@ static int hclge_dbg_dump_umv_info(struct seq_file *s, void *data)
 	struct hclge_vport *vport;
 	u8 i;
 
+	if (test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state))
+		return -EBUSY;
+
 	seq_printf(s, "num_alloc_vport   : %u\n", hdev->num_alloc_vport);
 	seq_printf(s, "max_umv_size     : %u\n", hdev->max_umv_size);
 	seq_printf(s, "wanted_umv_size  : %u\n", hdev->wanted_umv_size);
@@ -2680,6 +2683,9 @@ static int hclge_dbg_dump_vlan_offload_config(struct hclge_dev *hdev,
 	int ret;
 	u8 i;
 
+	if (test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state))
+		return -EBUSY;
+
 	seq_puts(s, "FUNC_ID  PVID  ACCEPT_TAG1  ACCEPT_TAG2 ACCEPT_UNTAG1  ");
 	seq_puts(s, "ACCEPT_UNTAG2  INSERT_TAG1  INSERT_TAG2  SHIFT_TAG  ");
 	seq_puts(s, "STRIP_TAG1  STRIP_TAG2  DROP_TAG1  DROP_TAG2  ");
-- 
2.33.0

Re: [PATCH net 2/2] net: hns3: fix null pointer in debugfs issue

[Jakub Kicinski]

On Thu, 23 Oct 2025 21:13:38 +0800 Jijie Shao wrote:
> Currently, when debugfs and reset are executed concurrently,
> some resources are released during the reset process,
> which may cause debugfs to read null pointers or other anomalies.
> 
> Therefore, in this patch, interception protection has been added
> to debugfs operations that are sensitive to reset.

You need to explain what prevents the state from changing immediately
after you did the bit check. With no obvious locking in place I don't
see how this reliably fixes the issue.

Re: [PATCH net 2/2] net: hns3: fix null pointer in debugfs issue

[Jijie Shao]

on 2025/10/28 8:54, Jakub Kicinski wrote:
> On Thu, 23 Oct 2025 21:13:38 +0800 Jijie Shao wrote:
>> Currently, when debugfs and reset are executed concurrently,
>> some resources are released during the reset process,
>> which may cause debugfs to read null pointers or other anomalies.
>>
>> Therefore, in this patch, interception protection has been added
>> to debugfs operations that are sensitive to reset.
> You need to explain what prevents the state from changing immediately
> after you did the bit check. With no obvious locking in place I don't
> see how this reliably fixes the issue.

In July, we used seqfile to refactor debugfs.

Before the refactoring, all debugfs operations would check the reset status
(HNS3_NIC_STATE_INITED and HNS3_NIC_STATE_RESETTING) in the entry function.
After the refactoring, the entry function was removed, which led to the loss of protection.

This patch restores the protection behavior that existed before the refactoring.
Now our tests have already detected the null pointer issue.

As for the problem you mentioned, we have been discussing it recently.
There is a small time gap, checking the status before reading from debugfs is fine,
but there could still be issues if the device enters the reset state during the read process:

check state pass
	debugfs read start...
		do reset
			debugfs read end
			
Currently, we are still assessing the risk and discussing solutions for this issue.
After adding the entry protection, executing debugfs and reset concurrently has not
resulted in null pointers or other exceptions.

Thanks,
Jijie Shao

Re: [PATCH net 0/2] There are some bugfix for the HNS3 ethernet driver

[Alexander Lobakin]

From: Jijie Shao <shaojijie@huawei.com>
Date: Thu, 23 Oct 2025 21:13:36 +0800

> This patchset includes 2 fixes:
> 1. Patch1 fixes an incorrect function return value.
> 2. Patch2 fixes the issue of lost reset concurrent protection after
>   seq_file reconstruction in debugfs.
> 
> Note: Compared to the previous version, this patchset has removed 2 patches
> and added 1 new patch, so it is being resent as V1.
> 
> previous patchset:
> https://lore.kernel.org/all/20250917122954.1265844-4-shaojijie@huawei.com/
> Patch 'use user configuration after hardware reset when using kernel PHY'
> will be sent separately.

For the series:

Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>

> 
> Jijie Shao (2):
>   net: hns3: return error code when function fails
>   net: hns3: fix null pointer in debugfs issue
Thanks,
Olek

Re: [PATCH net 0/2] There are some bugfix for the HNS3 ethernet driver

[patchwork-bot+netdevbpf]

Hello:

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

On Thu, 23 Oct 2025 21:13:36 +0800 you wrote:
> This patchset includes 2 fixes:
> 1. Patch1 fixes an incorrect function return value.
> 2. Patch2 fixes the issue of lost reset concurrent protection after
>   seq_file reconstruction in debugfs.
> 
> Note: Compared to the previous version, this patchset has removed 2 patches
> and added 1 new patch, so it is being resent as V1.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: hns3: return error code when function fails
    https://git.kernel.org/netdev/net/c/03ca7c8c42be
  - [net,2/2] net: hns3: fix null pointer in debugfs issue
    (no matching commit)

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