[PATCH net-next 00/11] net: hns3: some code optimizations & bugfixes & features

[PATCH net-next 00/11] net: hns3: some code optimizations & bugfixes & features

[Huazhong Tan]

This patch-set includes code optimizations, bugfixes and features for
the HNS3 ethernet controller driver.

[patch 1/11] checks reset status before setting channel.

[patch 2/11] adds a NULL pointer checking.

[patch 3/11] removes reset level upgrading when current reset fails.

[patch 4/11] fixes a bug related to IRQ vector number initialization.

[patch 5/11] fixes a GFP flags errors when holding spin_lock.

[patch 6/11] modifies firmware version format.

[patch 7/11] adds some print information.

[patch 8/11 - 9/11] adds two code optimizations about interrupt handler
and work task.

[patch 10/11] adds support for using order 1 pages with a 4K buffer.

[patch 11/11] adds a detection about the reason of IMP errors.

Guangbin Huang (1):
  net: hns3: add a check for get_reset_level

Huazhong Tan (1):
  net: hns3: remove upgrade reset level when reset fail

Jian Shen (1):
  net: hns3: add reset checking before set channels

Weihang Li (1):
  net: hns3: add support for handling IMP error

Yonglong Liu (2):
  net: hns3: fix mis-counting IRQ vector numbers issue
  net: hns3: adds debug messages to identify eth down cause

Yufeng Mo (2):
  net: hns3: change GFP flag during lock period
  net: hns3: modify firmware version display format

Yunsheng Lin (3):
  net: hns3: add interrupt affinity support for misc interrupt
  net: hns3: make hclge_service use delayed workqueue
  net: hns3: Add support for using order 1 pages with a 4K buffer

 drivers/net/ethernet/hisilicon/hns3/hnae3.h        |  10 ++
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c    |  39 ++++-
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.h    |  15 +-
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c |  41 ++++-
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c |  10 +-
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c |  14 ++
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c |  37 ++++-
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h |   4 +
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 165 ++++++++++++++-------
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  16 +-
 .../ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c   |  11 +-
 .../ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c  |  12 +-
 12 files changed, 298 insertions(+), 76 deletions(-)

-- 
2.7.4

[PATCH net-next 01/11] net: hns3: add reset checking before set channels

[Huazhong Tan]

From: Jian Shen <shenjian15@huawei.com>

hns3_set_channels() should check the resetting status firstly,
since the device will reinitialize when resetting. If the
reset has not completed, the hns3_set_channels() may access
invalid memory.

Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 69f7ef8..08af782 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -4378,6 +4378,9 @@ int hns3_set_channels(struct net_device *netdev,
 	u16 org_tqp_num;
 	int ret;
 
+	if (hns3_nic_resetting(netdev))
+		return -EBUSY;
+
 	if (ch->rx_count || ch->tx_count)
 		return -EINVAL;
 
-- 
2.7.4

[PATCH net-next 02/11] net: hns3: add a check for get_reset_level

[Huazhong Tan]

From: Guangbin Huang <huangguangbin@huawei.com>

For some cases, ops->get_reset_level may not be implemented, so we
should check whether it is NULL before calling get_reset_level.

Signed-off-by: Guangbin Huang <huangguangbin@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 08af782..4d58c53 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -1963,7 +1963,7 @@ static pci_ers_result_t hns3_slot_reset(struct pci_dev *pdev)
 
 	ops = ae_dev->ops;
 	/* request the reset */
-	if (ops->reset_event) {
+	if (ops->reset_event && ops->get_reset_level) {
 		if (ae_dev->hw_err_reset_req) {
 			reset_type = ops->get_reset_level(ae_dev,
 						&ae_dev->hw_err_reset_req);
-- 
2.7.4

[PATCH net-next 03/11] net: hns3: remove upgrade reset level when reset fail

[Huazhong Tan]

Currently, hclge_reset_err_handle() will assert a global reset
when the failing count is smaller than MAX_RESET_FAIL_CNT, which
will affect other running functions.

So this patch removes this upgrading, and uses re-scheduling reset
task to do it.

Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
Reviewed-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 28 +++++++---------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 3fde5471..3c64d70 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3305,7 +3305,7 @@ static int hclge_reset_prepare_wait(struct hclge_dev *hdev)
 	return ret;
 }
 
-static bool hclge_reset_err_handle(struct hclge_dev *hdev, bool is_timeout)
+static bool hclge_reset_err_handle(struct hclge_dev *hdev)
 {
 #define MAX_RESET_FAIL_CNT 5
 
@@ -3322,20 +3322,11 @@ static bool hclge_reset_err_handle(struct hclge_dev *hdev, bool is_timeout)
 		return false;
 	} else if (hdev->reset_fail_cnt < MAX_RESET_FAIL_CNT) {
 		hdev->reset_fail_cnt++;
-		if (is_timeout) {
-			set_bit(hdev->reset_type, &hdev->reset_pending);
-			dev_info(&hdev->pdev->dev,
-				 "re-schedule to wait for hw reset done\n");
-			return true;
-		}
-
-		dev_info(&hdev->pdev->dev, "Upgrade reset level\n");
-		hclge_clear_reset_cause(hdev);
-		set_bit(HNAE3_GLOBAL_RESET, &hdev->default_reset_request);
-		mod_timer(&hdev->reset_timer,
-			  jiffies + HCLGE_RESET_INTERVAL);
-
-		return false;
+		set_bit(hdev->reset_type, &hdev->reset_pending);
+		dev_info(&hdev->pdev->dev,
+			 "re-schedule reset task(%d)\n",
+			 hdev->reset_fail_cnt);
+		return true;
 	}
 
 	hclge_clear_reset_cause(hdev);
@@ -3382,7 +3373,6 @@ static int hclge_reset_stack(struct hclge_dev *hdev)
 static void hclge_reset(struct hclge_dev *hdev)
 {
 	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(hdev->pdev);
-	bool is_timeout = false;
 	int ret;
 
 	/* Initialize ae_dev reset status as well, in case enet layer wants to
@@ -3410,10 +3400,8 @@ static void hclge_reset(struct hclge_dev *hdev)
 	if (ret)
 		goto err_reset;
 
-	if (hclge_reset_wait(hdev)) {
-		is_timeout = true;
+	if (hclge_reset_wait(hdev))
 		goto err_reset;
-	}
 
 	hdev->rst_stats.hw_reset_done_cnt++;
 
@@ -3465,7 +3453,7 @@ static void hclge_reset(struct hclge_dev *hdev)
 err_reset_lock:
 	rtnl_unlock();
 err_reset:
-	if (hclge_reset_err_handle(hdev, is_timeout))
+	if (hclge_reset_err_handle(hdev))
 		hclge_reset_task_schedule(hdev);
 }
 
-- 
2.7.4

[PATCH net-next 04/11] net: hns3: fix mis-counting IRQ vector numbers issue

[Huazhong Tan]

From: Yonglong Liu <liuyonglong@huawei.com>

The num_msi_left means the vector numbers of NIC, but if the
PF supported RoCE, it contains the vector numbers of NIC and
RoCE(Not expected).

This may cause interrupts lost in some case, because of the
NIC module used the vector resources which belongs to RoCE.

This patch corrects the value of num_msi_left to be equals to
the vector numbers of NIC, and adjust the default tqp numbers
according to the value of num_msi_left.

Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine & Compatibility Layer Support")
Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c   |  5 ++++-
 drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 12 ++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 3c64d70..a59d13f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1470,13 +1470,16 @@ static int hclge_vport_setup(struct hclge_vport *vport, u16 num_tqps)
 {
 	struct hnae3_handle *nic = &vport->nic;
 	struct hclge_dev *hdev = vport->back;
+	u16 alloc_tqps;
 	int ret;
 
 	nic->pdev = hdev->pdev;
 	nic->ae_algo = &ae_algo;
 	nic->numa_node_mask = hdev->numa_node_mask;
 
-	ret = hclge_knic_setup(vport, num_tqps,
+	alloc_tqps = min_t(u16, hdev->roce_base_msix_offset - 1, num_tqps);
+
+	ret = hclge_knic_setup(vport, alloc_tqps,
 			       hdev->num_tx_desc, hdev->num_rx_desc);
 	if (ret)
 		dev_err(&hdev->pdev->dev, "knic setup failed %d\n", ret);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index a13a0e1..db84782 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -287,6 +287,14 @@ static int hclgevf_get_queue_info(struct hclgevf_dev *hdev)
 	memcpy(&hdev->rss_size_max, &resp_msg[2], sizeof(u16));
 	memcpy(&hdev->rx_buf_len, &resp_msg[4], sizeof(u16));
 
+	/* if irq is not enough, let tqps have the same value of irqs,
+	 * to make sure one irq just bind to one tqp, this can improve
+	 * the performance
+	 */
+	hdev->num_tqps = min_t(u16, hdev->roce_base_msix_offset - 1,
+			       hdev->num_tqps);
+	hdev->rss_size_max = min_t(u16, hdev->rss_size_max, hdev->num_tqps);
+
 	return 0;
 }
 
@@ -2208,7 +2216,7 @@ static int hclgevf_init_msi(struct hclgevf_dev *hdev)
 	int vectors;
 	int i;
 
-	if (hnae3_get_bit(hdev->ae_dev->flag, HNAE3_DEV_SUPPORT_ROCE_B))
+	if (hnae3_dev_roce_supported(hdev))
 		vectors = pci_alloc_irq_vectors(pdev,
 						hdev->roce_base_msix_offset + 1,
 						hdev->num_msi,
@@ -2495,7 +2503,7 @@ static int hclgevf_query_vf_resource(struct hclgevf_dev *hdev)
 
 	req = (struct hclgevf_query_res_cmd *)desc.data;
 
-	if (hnae3_get_bit(hdev->ae_dev->flag, HNAE3_DEV_SUPPORT_ROCE_B)) {
+	if (hnae3_dev_roce_supported(hdev)) {
 		hdev->roce_base_msix_offset =
 		hnae3_get_field(__le16_to_cpu(req->msixcap_localid_ba_rocee),
 				HCLGEVF_MSIX_OFT_ROCEE_M,
-- 
2.7.4

[PATCH net-next 05/11] net: hns3: change GFP flag during lock period

[Huazhong Tan]

From: Yufeng Mo <moyufeng@huawei.com>

When allocating memory, the GFP_KERNEL cannot be used during the
spin_lock period. This is because it may cause scheduling when holding
spin_lock. This patch changes GFP flag to GFP_ATOMIC in this case.

Fixes: dd74f815dd41 ("net: hns3: Add support for rule add/delete for flow director")
Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
Signed-off-by: lipeng 00277521 <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index a59d13f..f345095 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -5799,7 +5799,7 @@ static int hclge_add_fd_entry_by_arfs(struct hnae3_handle *handle, u16 queue_id,
 			return -ENOSPC;
 		}
 
-		rule = kzalloc(sizeof(*rule), GFP_KERNEL);
+		rule = kzalloc(sizeof(*rule), GFP_ATOMIC);
 		if (!rule) {
 			spin_unlock_bh(&hdev->fd_rule_lock);
 
-- 
2.7.4

[PATCH net-next 06/11] net: hns3: modify firmware version display format

[Huazhong Tan]

From: Yufeng Mo <moyufeng@huawei.com>

This patch modifies firmware version display format in
hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
some optimizations for firmware version display format.

Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h              |  9 +++++++++
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c       | 15 +++++++++++++--
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c   | 10 +++++++++-
 drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11 +++++++++--
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index 48c7b70..a4624db 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -179,6 +179,15 @@ struct hnae3_vector_info {
 #define HNAE3_RING_GL_RX 0
 #define HNAE3_RING_GL_TX 1
 
+#define HNAE3_FW_VERSION_BYTE3_SHIFT	24
+#define HNAE3_FW_VERSION_BYTE3_MASK	GENMASK(31, 24)
+#define HNAE3_FW_VERSION_BYTE2_SHIFT	16
+#define HNAE3_FW_VERSION_BYTE2_MASK	GENMASK(23, 16)
+#define HNAE3_FW_VERSION_BYTE1_SHIFT	8
+#define HNAE3_FW_VERSION_BYTE1_MASK	GENMASK(15, 8)
+#define HNAE3_FW_VERSION_BYTE0_SHIFT	0
+#define HNAE3_FW_VERSION_BYTE0_MASK	GENMASK(7, 0)
+
 struct hnae3_ring_chain_node {
 	struct hnae3_ring_chain_node *next;
 	u32 tqp_index;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index 5bff98a..e71c92b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -527,6 +527,7 @@ static void hns3_get_drvinfo(struct net_device *netdev,
 {
 	struct hns3_nic_priv *priv = netdev_priv(netdev);
 	struct hnae3_handle *h = priv->ae_handle;
+	u32 fw_version;
 
 	if (!h->ae_algo->ops->get_fw_version) {
 		netdev_err(netdev, "could not get fw version!\n");
@@ -545,8 +546,18 @@ static void hns3_get_drvinfo(struct net_device *netdev,
 		sizeof(drvinfo->bus_info));
 	drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
 
-	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version), "0x%08x",
-		 priv->ae_handle->ae_algo->ops->get_fw_version(h));
+	fw_version = priv->ae_handle->ae_algo->ops->get_fw_version(h);
+
+	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
+		 "%lu.%lu.%lu.%lu",
+		 hnae3_get_field(fw_version, HNAE3_FW_VERSION_BYTE3_MASK,
+				 HNAE3_FW_VERSION_BYTE3_SHIFT),
+		 hnae3_get_field(fw_version, HNAE3_FW_VERSION_BYTE2_MASK,
+				 HNAE3_FW_VERSION_BYTE2_SHIFT),
+		 hnae3_get_field(fw_version, HNAE3_FW_VERSION_BYTE1_MASK,
+				 HNAE3_FW_VERSION_BYTE1_SHIFT),
+		 hnae3_get_field(fw_version, HNAE3_FW_VERSION_BYTE0_MASK,
+				 HNAE3_FW_VERSION_BYTE0_SHIFT));
 }
 
 static u32 hns3_get_link(struct net_device *netdev)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
index 22f6acd..c2320bf 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
@@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
 	}
 	hdev->fw_version = version;
 
-	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n", version);
+	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE3_MASK,
+				     HNAE3_FW_VERSION_BYTE3_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE2_MASK,
+				     HNAE3_FW_VERSION_BYTE2_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE1_MASK,
+				     HNAE3_FW_VERSION_BYTE1_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE0_MASK,
+				     HNAE3_FW_VERSION_BYTE0_SHIFT));
 
 	return 0;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
index 652b796..004125b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
@@ -405,8 +405,15 @@ int hclgevf_cmd_init(struct hclgevf_dev *hdev)
 	}
 	hdev->fw_version = version;
 
-	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n", version);
-
+	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE3_MASK,
+				     HNAE3_FW_VERSION_BYTE3_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE2_MASK,
+				     HNAE3_FW_VERSION_BYTE2_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE1_MASK,
+				     HNAE3_FW_VERSION_BYTE1_SHIFT),
+		     hnae3_get_field(version, HNAE3_FW_VERSION_BYTE0_MASK,
+				     HNAE3_FW_VERSION_BYTE0_SHIFT));
 	return 0;
 
 err_cmd_init:
-- 
2.7.4

[PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[Huazhong Tan]

From: Yonglong Liu <liuyonglong@huawei.com>

Some times just see the eth interface have been down/up via
dmesg, but can not know why the eth down. So adds some debug
messages to identify the cause for this.

Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c    | 24 ++++++++++++++++++++
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 26 ++++++++++++++++++++++
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 14 ++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 4d58c53..cff5d59 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -459,6 +459,10 @@ static int hns3_nic_net_open(struct net_device *netdev)
 		h->ae_algo->ops->set_timer_task(priv->ae_handle, true);
 
 	hns3_config_xps(priv);
+
+	if (netif_msg_ifup(h))
+		netdev_info(netdev, "net open\n");
+
 	return 0;
 }
 
@@ -519,6 +523,9 @@ static int hns3_nic_net_stop(struct net_device *netdev)
 	if (test_and_set_bit(HNS3_NIC_STATE_DOWN, &priv->state))
 		return 0;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev, "net stop\n");
+
 	if (h->ae_algo->ops->set_timer_task)
 		h->ae_algo->ops->set_timer_task(priv->ae_handle, false);
 
@@ -1550,6 +1557,9 @@ static int hns3_setup_tc(struct net_device *netdev, void *type_data)
 	h = hns3_get_handle(netdev);
 	kinfo = &h->kinfo;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev, "setup tc: num_tc=%d\n", tc);
+
 	return (kinfo->dcb_ops && kinfo->dcb_ops->setup_tc) ?
 		kinfo->dcb_ops->setup_tc(h, tc, prio_tc) : -EOPNOTSUPP;
 }
@@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan,
 	struct hnae3_handle *h = hns3_get_handle(netdev);
 	int ret = -EIO;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev,
+			    "set vf vlan: vf=%d, vlan=%d, qos=%d, vlan_proto=%d\n",
+			    vf, vlan, qos, vlan_proto);
+
 	if (h->ae_algo->ops->set_vf_vlan_filter)
 		ret = h->ae_algo->ops->set_vf_vlan_filter(h, vf, vlan,
 							  qos, vlan_proto);
@@ -1611,6 +1626,10 @@ static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu)
 	if (!h->ae_algo->ops->set_mtu)
 		return -EOPNOTSUPP;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev, "change mtu from %d to %d\n",
+			    netdev->mtu, new_mtu);
+
 	ret = h->ae_algo->ops->set_mtu(h, new_mtu);
 	if (ret)
 		netdev_err(netdev, "failed to change MTU in hardware %d\n",
@@ -4395,6 +4414,11 @@ int hns3_set_channels(struct net_device *netdev,
 	if (kinfo->rss_size == new_tqp_num)
 		return 0;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev,
+			    "set channels: tqp_num=%d, rxfh=%d\n",
+			    new_tqp_num, rxfh_configured);
+
 	ret = hns3_reset_notify(h, HNAE3_DOWN_CLIENT);
 	if (ret)
 		return ret;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index e71c92b..edb9845 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -311,6 +311,9 @@ static void hns3_self_test(struct net_device *ndev,
 	if (eth_test->flags != ETH_TEST_FL_OFFLINE)
 		return;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(ndev, "self test start\n");
+
 	st_param[HNAE3_LOOP_APP][0] = HNAE3_LOOP_APP;
 	st_param[HNAE3_LOOP_APP][1] =
 			h->flags & HNAE3_SUPPORT_APP_LOOPBACK;
@@ -374,6 +377,9 @@ static void hns3_self_test(struct net_device *ndev,
 
 	if (if_running)
 		ndev->netdev_ops->ndo_open(ndev);
+
+	if (netif_msg_ifdown(h))
+		netdev_info(ndev, "self test end\n");
 }
 
 static int hns3_get_sset_count(struct net_device *netdev, int stringset)
@@ -604,6 +610,11 @@ static int hns3_set_pauseparam(struct net_device *netdev,
 {
 	struct hnae3_handle *h = hns3_get_handle(netdev);
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev,
+			    "set pauseparam: autoneg=%d, rx:%d, tx:%d\n",
+			    param->autoneg, param->rx_pause, param->tx_pause);
+
 	if (h->ae_algo->ops->set_pauseparam)
 		return h->ae_algo->ops->set_pauseparam(h, param->autoneg,
 						       param->rx_pause,
@@ -743,6 +754,13 @@ static int hns3_set_link_ksettings(struct net_device *netdev,
 	if (cmd->base.speed == SPEED_1000 && cmd->base.duplex == DUPLEX_HALF)
 		return -EINVAL;
 
+	if (netif_msg_ifdown(handle))
+		netdev_info(netdev,
+			    "set link(%s): autoneg=%d, speed=%d, duplex=%d\n",
+			    netdev->phydev ? "phy" : "mac",
+			    cmd->base.autoneg, cmd->base.speed,
+			    cmd->base.duplex);
+
 	/* Only support ksettings_set for netdev with phy attached for now */
 	if (netdev->phydev)
 		return phy_ethtool_ksettings_set(netdev->phydev, cmd);
@@ -984,6 +1002,10 @@ static int hns3_nway_reset(struct net_device *netdev)
 		return -EINVAL;
 	}
 
+	if (netif_msg_ifdown(handle))
+		netdev_info(netdev, "nway reset (using %s)\n",
+			    phy ? "phy" : "mac");
+
 	if (phy)
 		return genphy_restart_aneg(phy);
 
@@ -1308,6 +1330,10 @@ static int hns3_set_fecparam(struct net_device *netdev,
 	if (!ops->set_fec)
 		return -EOPNOTSUPP;
 	fec_mode = eth_to_loc_fec(fec->fec);
+
+	if (netif_msg_ifdown(handle))
+		netdev_info(netdev, "set fecparam: mode=%d\n", fec_mode);
+
 	return ops->set_fec(handle, fec_mode);
 }
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
index bac4ce1..133e7c6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
@@ -201,6 +201,7 @@ static int hclge_client_setup_tc(struct hclge_dev *hdev)
 static int hclge_ieee_setets(struct hnae3_handle *h, struct ieee_ets *ets)
 {
 	struct hclge_vport *vport = hclge_get_vport(h);
+	struct net_device *netdev = h->kinfo.netdev;
 	struct hclge_dev *hdev = vport->back;
 	bool map_changed = false;
 	u8 num_tc = 0;
@@ -215,6 +216,9 @@ static int hclge_ieee_setets(struct hnae3_handle *h, struct ieee_ets *ets)
 		return ret;
 
 	if (map_changed) {
+		if (netif_msg_ifdown(h))
+			netdev_info(netdev, "set ets\n");
+
 		ret = hclge_notify_client(hdev, HNAE3_DOWN_CLIENT);
 		if (ret)
 			return ret;
@@ -300,6 +304,7 @@ static int hclge_ieee_getpfc(struct hnae3_handle *h, struct ieee_pfc *pfc)
 static int hclge_ieee_setpfc(struct hnae3_handle *h, struct ieee_pfc *pfc)
 {
 	struct hclge_vport *vport = hclge_get_vport(h);
+	struct net_device *netdev = h->kinfo.netdev;
 	struct hclge_dev *hdev = vport->back;
 	u8 i, j, pfc_map, *prio_tc;
 
@@ -325,6 +330,11 @@ static int hclge_ieee_setpfc(struct hnae3_handle *h, struct ieee_pfc *pfc)
 	hdev->tm_info.hw_pfc_map = pfc_map;
 	hdev->tm_info.pfc_en = pfc->pfc_en;
 
+	if (netif_msg_ifdown(h))
+		netdev_info(netdev,
+			    "set pfc: pfc_en=%d, pfc_map=%d, num_tc=%d\n",
+			    pfc->pfc_en, pfc_map, hdev->tm_info.num_tc);
+
 	hclge_tm_pfc_info_update(hdev);
 
 	return hclge_pause_setup_hw(hdev, false);
@@ -345,8 +355,12 @@ static u8 hclge_getdcbx(struct hnae3_handle *h)
 static u8 hclge_setdcbx(struct hnae3_handle *h, u8 mode)
 {
 	struct hclge_vport *vport = hclge_get_vport(h);
+	struct net_device *netdev = h->kinfo.netdev;
 	struct hclge_dev *hdev = vport->back;
 
+	if (netif_msg_drv(h))
+		netdev_info(netdev, "set dcbx: mode=%d\n", mode);
+
 	/* No support for LLD_MANAGED modes or CEE */
 	if ((mode & DCB_CAP_DCBX_LLD_MANAGED) ||
 	    (mode & DCB_CAP_DCBX_VER_CEE) ||
-- 
2.7.4

[PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Huazhong Tan]

From: Yunsheng Lin <linyunsheng@huawei.com>

The misc interrupt is used to schedule the reset and mailbox
subtask, and a 1 sec timer is used to schedule the service
subtask, which does periodic work.

This patch sets the above three subtask's affinity using the
misc interrupt' affinity.

Also this patch setups a affinity notify for misc interrupt to
allow user to change the above three subtask's affinity.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 59 ++++++++++++++++++++--
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  4 ++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index f345095..fe45986 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1270,6 +1270,12 @@ static int hclge_configure(struct hclge_dev *hdev)
 
 	hclge_init_kdump_kernel_config(hdev);
 
+	/* Set the init affinity based on pci func number */
+	i = cpumask_weight(cpumask_of_node(dev_to_node(&hdev->pdev->dev)));
+	i = i ? PCI_FUNC(hdev->pdev->devfn) % i : 0;
+	cpumask_set_cpu(cpumask_local_spread(i, dev_to_node(&hdev->pdev->dev)),
+			&hdev->affinity_mask);
+
 	return ret;
 }
 
@@ -2502,14 +2508,16 @@ static void hclge_mbx_task_schedule(struct hclge_dev *hdev)
 {
 	if (!test_bit(HCLGE_STATE_CMD_DISABLE, &hdev->state) &&
 	    !test_and_set_bit(HCLGE_STATE_MBX_SERVICE_SCHED, &hdev->state))
-		schedule_work(&hdev->mbx_service_task);
+		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
+			      &hdev->mbx_service_task);
 }
 
 static void hclge_reset_task_schedule(struct hclge_dev *hdev)
 {
 	if (!test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
 	    !test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state))
-		schedule_work(&hdev->rst_service_task);
+		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
+			      &hdev->rst_service_task);
 }
 
 static void hclge_task_schedule(struct hclge_dev *hdev)
@@ -2517,7 +2525,8 @@ static void hclge_task_schedule(struct hclge_dev *hdev)
 	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
 	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
 	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
-		(void)schedule_work(&hdev->service_task);
+		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
+			      &hdev->service_task);
 }
 
 static int hclge_get_mac_link_status(struct hclge_dev *hdev)
@@ -2921,6 +2930,39 @@ static void hclge_get_misc_vector(struct hclge_dev *hdev)
 	hdev->num_msi_used += 1;
 }
 
+static void hclge_irq_affinity_notify(struct irq_affinity_notify *notify,
+				      const cpumask_t *mask)
+{
+	struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
+					      affinity_notify);
+
+	cpumask_copy(&hdev->affinity_mask, mask);
+	del_timer_sync(&hdev->service_timer);
+	hdev->service_timer.expires = jiffies + HZ;
+	add_timer_on(&hdev->service_timer, cpumask_first(&hdev->affinity_mask));
+}
+
+static void hclge_irq_affinity_release(struct kref *ref)
+{
+}
+
+static void hclge_misc_affinity_setup(struct hclge_dev *hdev)
+{
+	irq_set_affinity_hint(hdev->misc_vector.vector_irq,
+			      &hdev->affinity_mask);
+
+	hdev->affinity_notify.notify = hclge_irq_affinity_notify;
+	hdev->affinity_notify.release = hclge_irq_affinity_release;
+	irq_set_affinity_notifier(hdev->misc_vector.vector_irq,
+				  &hdev->affinity_notify);
+}
+
+static void hclge_misc_affinity_teardown(struct hclge_dev *hdev)
+{
+	irq_set_affinity_notifier(hdev->misc_vector.vector_irq, NULL);
+	irq_set_affinity_hint(hdev->misc_vector.vector_irq, NULL);
+}
+
 static int hclge_misc_irq_init(struct hclge_dev *hdev)
 {
 	int ret;
@@ -6151,7 +6193,10 @@ static void hclge_set_timer_task(struct hnae3_handle *handle, bool enable)
 	struct hclge_dev *hdev = vport->back;
 
 	if (enable) {
-		mod_timer(&hdev->service_timer, jiffies + HZ);
+		del_timer_sync(&hdev->service_timer);
+		hdev->service_timer.expires = jiffies + HZ;
+		add_timer_on(&hdev->service_timer,
+			     cpumask_first(&hdev->affinity_mask));
 	} else {
 		del_timer_sync(&hdev->service_timer);
 		cancel_work_sync(&hdev->service_task);
@@ -8809,6 +8854,11 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 	INIT_WORK(&hdev->rst_service_task, hclge_reset_service_task);
 	INIT_WORK(&hdev->mbx_service_task, hclge_mailbox_service_task);
 
+	/* Setup affinity after service timer setup because add_timer_on
+	 * is called in affinity notify.
+	 */
+	hclge_misc_affinity_setup(hdev);
+
 	hclge_clear_all_event_cause(hdev);
 	hclge_clear_resetting_state(hdev);
 
@@ -8970,6 +9020,7 @@ static void hclge_uninit_ae_dev(struct hnae3_ae_dev *ae_dev)
 	struct hclge_dev *hdev = ae_dev->priv;
 	struct hclge_mac *mac = &hdev->hw.mac;
 
+	hclge_misc_affinity_teardown(hdev);
 	hclge_state_uninit(hdev);
 
 	if (mac->phydev)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 6a12285..14df23c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -864,6 +864,10 @@ struct hclge_dev {
 
 	DECLARE_KFIFO(mac_tnl_log, struct hclge_mac_tnl_stats,
 		      HCLGE_MAC_TNL_LOG_SIZE);
+
+	/* affinity mask and notify for misc interrupt */
+	cpumask_t affinity_mask;
+	struct irq_affinity_notify affinity_notify;
 };
 
 /* VPort level vlan tag configuration for TX direction */
-- 
2.7.4

[PATCH net-next 09/11] net: hns3: make hclge_service use delayed workqueue

[Huazhong Tan]

From: Yunsheng Lin <linyunsheng@huawei.com>

Use delayed work instead of using timers to trigger the
hclge_serive.

Simplify the code with one less middle function and in order
to support misc irq affinity.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Reviewed-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 60 ++++++++--------------
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  3 +-
 2 files changed, 22 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index fe45986..45acbc9 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -2524,9 +2524,13 @@ static void hclge_task_schedule(struct hclge_dev *hdev)
 {
 	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
 	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
-	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
-		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
-			      &hdev->service_task);
+	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state)) {
+		hdev->hw_stats.stats_timer++;
+		hdev->fd_arfs_expire_timer++;
+		mod_delayed_work_on(cpumask_first(&hdev->affinity_mask),
+				    system_wq, &hdev->service_task,
+				    round_jiffies_relative(HZ));
+	}
 }
 
 static int hclge_get_mac_link_status(struct hclge_dev *hdev)
@@ -2741,25 +2745,6 @@ static int hclge_get_status(struct hnae3_handle *handle)
 	return hdev->hw.mac.link;
 }
 
-static void hclge_service_timer(struct timer_list *t)
-{
-	struct hclge_dev *hdev = from_timer(hdev, t, service_timer);
-
-	mod_timer(&hdev->service_timer, jiffies + HZ);
-	hdev->hw_stats.stats_timer++;
-	hdev->fd_arfs_expire_timer++;
-	hclge_task_schedule(hdev);
-}
-
-static void hclge_service_complete(struct hclge_dev *hdev)
-{
-	WARN_ON(!test_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state));
-
-	/* Flush memory before next watchdog */
-	smp_mb__before_atomic();
-	clear_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state);
-}
-
 static u32 hclge_check_event_cause(struct hclge_dev *hdev, u32 *clearval)
 {
 	u32 rst_src_reg, cmdq_src_reg, msix_src_reg;
@@ -2937,9 +2922,6 @@ static void hclge_irq_affinity_notify(struct irq_affinity_notify *notify,
 					      affinity_notify);
 
 	cpumask_copy(&hdev->affinity_mask, mask);
-	del_timer_sync(&hdev->service_timer);
-	hdev->service_timer.expires = jiffies + HZ;
-	add_timer_on(&hdev->service_timer, cpumask_first(&hdev->affinity_mask));
 }
 
 static void hclge_irq_affinity_release(struct kref *ref)
@@ -3639,7 +3621,9 @@ static void hclge_update_vport_alive(struct hclge_dev *hdev)
 static void hclge_service_task(struct work_struct *work)
 {
 	struct hclge_dev *hdev =
-		container_of(work, struct hclge_dev, service_task);
+		container_of(work, struct hclge_dev, service_task.work);
+
+	clear_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state);
 
 	if (hdev->hw_stats.stats_timer >= HCLGE_STATS_TIMER_INTERVAL) {
 		hclge_update_stats_for_all(hdev);
@@ -3654,7 +3638,8 @@ static void hclge_service_task(struct work_struct *work)
 		hclge_rfs_filter_expire(hdev);
 		hdev->fd_arfs_expire_timer = 0;
 	}
-	hclge_service_complete(hdev);
+
+	hclge_task_schedule(hdev);
 }
 
 struct hclge_vport *hclge_get_vport(struct hnae3_handle *handle)
@@ -6193,13 +6178,13 @@ static void hclge_set_timer_task(struct hnae3_handle *handle, bool enable)
 	struct hclge_dev *hdev = vport->back;
 
 	if (enable) {
-		del_timer_sync(&hdev->service_timer);
-		hdev->service_timer.expires = jiffies + HZ;
-		add_timer_on(&hdev->service_timer,
-			     cpumask_first(&hdev->affinity_mask));
+		hclge_task_schedule(hdev);
 	} else {
-		del_timer_sync(&hdev->service_timer);
-		cancel_work_sync(&hdev->service_task);
+		/* Set the DOWN flag here to disable the service to be
+		 * scheduled again
+		 */
+		set_bit(HCLGE_STATE_DOWN, &hdev->state);
+		cancel_delayed_work_sync(&hdev->service_task);
 		clear_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state);
 	}
 }
@@ -8638,12 +8623,10 @@ static void hclge_state_uninit(struct hclge_dev *hdev)
 	set_bit(HCLGE_STATE_DOWN, &hdev->state);
 	set_bit(HCLGE_STATE_REMOVING, &hdev->state);
 
-	if (hdev->service_timer.function)
-		del_timer_sync(&hdev->service_timer);
 	if (hdev->reset_timer.function)
 		del_timer_sync(&hdev->reset_timer);
-	if (hdev->service_task.func)
-		cancel_work_sync(&hdev->service_task);
+	if (hdev->service_task.work.func)
+		cancel_delayed_work_sync(&hdev->service_task);
 	if (hdev->rst_service_task.func)
 		cancel_work_sync(&hdev->rst_service_task);
 	if (hdev->mbx_service_task.func)
@@ -8848,9 +8831,8 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 
 	hclge_dcb_ops_set(hdev);
 
-	timer_setup(&hdev->service_timer, hclge_service_timer, 0);
 	timer_setup(&hdev->reset_timer, hclge_reset_timer, 0);
-	INIT_WORK(&hdev->service_task, hclge_service_task);
+	INIT_DELAYED_WORK(&hdev->service_task, hclge_service_task);
 	INIT_WORK(&hdev->rst_service_task, hclge_reset_service_task);
 	INIT_WORK(&hdev->mbx_service_task, hclge_mailbox_service_task);
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 14df23c..688e425 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -806,9 +806,8 @@ struct hclge_dev {
 	u16 adminq_work_limit; /* Num of admin receive queue desc to process */
 	unsigned long service_timer_period;
 	unsigned long service_timer_previous;
-	struct timer_list service_timer;
 	struct timer_list reset_timer;
-	struct work_struct service_task;
+	struct delayed_work service_task;
 	struct work_struct rst_service_task;
 	struct work_struct mbx_service_task;
 
-- 
2.7.4

[PATCH net-next 10/11] net: hns3: Add support for using order 1 pages with a 4K buffer

[Huazhong Tan]

From: Yunsheng Lin <linyunsheng@huawei.com>

Hardware supports 0.5K, 1K, 2K, 4K RX buffer size, the
RX buffer can not be reused because the hns3_page_order
return 0 when page size and RX buffer size are both 4096.

So this patch changes the hns3_page_order to return 1 when
RX buffer is greater than half of the page size and page size
is less the 8192, and dev_alloc_pages has already been used
to allocate the compound page for RX buffer.

This patch also changes hnae3_* to hns3_* for page order
and RX buffer size calculation because they are used in
hns3 module.

Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
Reviewed-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 +++++-----
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.h | 15 ++++++++++++---
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index cff5d59..56af4be 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2086,7 +2086,7 @@ static void hns3_set_default_feature(struct net_device *netdev)
 static int hns3_alloc_buffer(struct hns3_enet_ring *ring,
 			     struct hns3_desc_cb *cb)
 {
-	unsigned int order = hnae3_page_order(ring);
+	unsigned int order = hns3_page_order(ring);
 	struct page *p;
 
 	p = dev_alloc_pages(order);
@@ -2097,7 +2097,7 @@ static int hns3_alloc_buffer(struct hns3_enet_ring *ring,
 	cb->page_offset = 0;
 	cb->reuse_flag = 0;
 	cb->buf  = page_address(p);
-	cb->length = hnae3_page_size(ring);
+	cb->length = hns3_page_size(ring);
 	cb->type = DESC_TYPE_PAGE;
 
 	return 0;
@@ -2400,7 +2400,7 @@ static void hns3_nic_reuse_page(struct sk_buff *skb, int i,
 {
 	struct hns3_desc *desc = &ring->desc[ring->next_to_clean];
 	int size = le16_to_cpu(desc->rx.size);
-	u32 truesize = hnae3_buf_size(ring);
+	u32 truesize = hns3_buf_size(ring);
 
 	skb_add_rx_frag(skb, i, desc_cb->priv, desc_cb->page_offset + pull_len,
 			size - pull_len, truesize);
@@ -2415,7 +2415,7 @@ static void hns3_nic_reuse_page(struct sk_buff *skb, int i,
 	/* Move offset up to the next cache line */
 	desc_cb->page_offset += truesize;
 
-	if (desc_cb->page_offset + truesize <= hnae3_page_size(ring)) {
+	if (desc_cb->page_offset + truesize <= hns3_page_size(ring)) {
 		desc_cb->reuse_flag = 1;
 		/* Bump ref count on page before it is given */
 		get_page(desc_cb->priv);
@@ -2697,7 +2697,7 @@ static int hns3_add_frag(struct hns3_enet_ring *ring, struct hns3_desc *desc,
 		}
 
 		if (ring->tail_skb) {
-			head_skb->truesize += hnae3_buf_size(ring);
+			head_skb->truesize += hns3_buf_size(ring);
 			head_skb->data_len += le16_to_cpu(desc->rx.size);
 			head_skb->len += le16_to_cpu(desc->rx.size);
 			skb = ring->tail_skb;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
index 848b866..1a17856 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.h
@@ -608,9 +608,18 @@ static inline bool hns3_nic_resetting(struct net_device *netdev)
 
 #define tx_ring_data(priv, idx) ((priv)->ring_data[idx])
 
-#define hnae3_buf_size(_ring) ((_ring)->buf_size)
-#define hnae3_page_order(_ring) (get_order(hnae3_buf_size(_ring)))
-#define hnae3_page_size(_ring) (PAGE_SIZE << (u32)hnae3_page_order(_ring))
+#define hns3_buf_size(_ring) ((_ring)->buf_size)
+
+static inline unsigned int hns3_page_order(struct hns3_enet_ring *ring)
+{
+#if (PAGE_SIZE < 8192)
+	if (ring->buf_size > (PAGE_SIZE / 2))
+		return 1;
+#endif
+	return 0;
+}
+
+#define hns3_page_size(_ring) (PAGE_SIZE << hns3_page_order(_ring))
 
 /* iterator for handling rings in ring group */
 #define hns3_for_each_ring(pos, head) \
-- 
2.7.4

[PATCH net-next 11/11] net: hns3: add support for handling IMP error

[Huazhong Tan]

From: Weihang Li <liweihang@hisilicon.com>

When IMP goes errors, the hardware reports a RAS to the driver,
the driver record this kind of error. Then a IMP reset will happen,
the driver checks the reason and takes the corresponding action
when doing IMP reset.

So this patch adds imp_err_state field to the struct hclge_dev
to record the error type, and handle_imp_error ops to handle it.

Signed-off-by: Weihang Li <liweihang@hisilicon.com>
Signed-off-by: Peng Li <lipeng321@huawei.com>
Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h        |  1 +
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c | 37 ++++++++++++++++++++--
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h |  4 +++
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 29 +++++++++++++++++
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  9 ++++++
 5 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index a4624db..3a1d6cc 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -515,6 +515,7 @@ struct hnae3_ae_ops {
 	int (*mac_connect_phy)(struct hnae3_handle *handle);
 	void (*mac_disconnect_phy)(struct hnae3_handle *handle);
 	void (*restore_vlan_table)(struct hnae3_handle *handle);
+	void (*handle_imp_error)(struct hnae3_handle *handle);
 };
 
 struct hnae3_dcb_ops {
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
index 0a72438..25df66d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.c
@@ -683,6 +683,28 @@ static int hclge_cmd_query_error(struct hclge_dev *hdev,
 	return ret;
 }
 
+static int hclge_check_imp_poison_err(struct hclge_dev *hdev)
+{
+	struct device *dev = &hdev->pdev->dev;
+	int ras_status;
+	int ret = false;
+
+	ras_status = hclge_read_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG);
+	if (ras_status & HCLGE_RAS_IMP_RD_POISON_MASK) {
+		set_bit(HCLGE_IMP_RD_POISON, &hdev->imp_err_state);
+		/* This error will be handle by IMP reset */
+		dev_info(dev, "IMP RD poison detected!\n");
+		ret = true;
+	} else if (ras_status & HCLGE_RAS_IMP_CMDQ_ERR_MASK) {
+		set_bit(HCLGE_IMP_CMDQ_ERROR, &hdev->imp_err_state);
+		/* This error will be handle by IMP reset */
+		dev_info(dev, "IMP CMDQ error detected!\n");
+		ret = true;
+	}
+
+	return ret;
+}
+
 static int hclge_clear_mac_tnl_int(struct hclge_dev *hdev)
 {
 	struct hclge_desc desc;
@@ -1321,10 +1343,12 @@ static int hclge_handle_pf_ras_error(struct hclge_dev *hdev,
 	/* log PPU(RCB) errors */
 	desc_data = (__le32 *)&desc[3];
 	status = le32_to_cpu(*desc_data) & HCLGE_PPU_PF_INT_RAS_MASK;
-	if (status)
+	if (status) {
 		hclge_log_error(dev, "PPU_PF_ABNORMAL_INT_ST0",
 				&hclge_ppu_pf_abnormal_int[0], status,
 				&ae_dev->hw_err_reset_req);
+		hdev->ppu_poison_ras_err = true;
+	}
 
 	/* clear all PF RAS errors */
 	hclge_cmd_reuse_desc(&desc[0], false);
@@ -1632,6 +1656,7 @@ pci_ers_result_t hclge_handle_hw_ras_error(struct hnae3_ae_dev *ae_dev)
 	struct hclge_dev *hdev = ae_dev->priv;
 	struct device *dev = &hdev->pdev->dev;
 	u32 status;
+	int ret;
 
 	if (!test_bit(HCLGE_STATE_SERVICE_INITED, &hdev->state)) {
 		dev_err(dev,
@@ -1639,6 +1664,9 @@ pci_ers_result_t hclge_handle_hw_ras_error(struct hnae3_ae_dev *ae_dev)
 		return PCI_ERS_RESULT_NONE;
 	}
 
+	if (hclge_check_imp_poison_err(hdev))
+		return PCI_ERS_RESULT_RECOVERED;
+
 	status = hclge_read_dev(&hdev->hw, HCLGE_RAS_PF_OTHER_INT_STS_REG);
 
 	if (status & HCLGE_RAS_REG_NFE_MASK ||
@@ -1652,7 +1680,12 @@ pci_ers_result_t hclge_handle_hw_ras_error(struct hnae3_ae_dev *ae_dev)
 		dev_warn(dev,
 			 "HNS Non-Fatal RAS error(status=0x%x) identified\n",
 			 status);
-		hclge_handle_all_ras_errors(hdev);
+		ret = hclge_handle_all_ras_errors(hdev);
+		if (ret) {
+			ret = hclge_check_imp_poison_err(hdev);
+			if (ret)
+				return PCI_ERS_RESULT_RECOVERED;
+		}
 	}
 
 	/* Handling Non-fatal Rocee RAS errors */
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h
index 7ea8bb2..4839fc4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_err.h
@@ -12,6 +12,10 @@
 #define HCLGE_PF_MSIX_INT_MIN_BD_NUM	4
 
 #define HCLGE_RAS_PF_OTHER_INT_STS_REG   0x20B00
+#define HCLGE_RAS_IMP_RD_POISON_MASK \
+	BIT(HCLGE_VECTOR0_IMP_RD_POISON_B)
+#define HCLGE_RAS_IMP_CMDQ_ERR_MASK \
+	BIT(HCLGE_VECTOR0_IMP_CMDQ_ERR_B)
 #define HCLGE_RAS_REG_NFE_MASK   0xFF00
 #define HCLGE_RAS_REG_ROCEE_ERR_MASK   0x3000000
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 45acbc9..36a2b65 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3281,6 +3281,7 @@ static int hclge_reset_prepare_wait(struct hclge_dev *hdev)
 {
 #define HCLGE_RESET_SYNC_TIME 100
 
+	struct hnae3_handle *handle = &hdev->vport[0].nic;
 	u32 reg_val;
 	int ret = 0;
 
@@ -3315,6 +3316,8 @@ static int hclge_reset_prepare_wait(struct hclge_dev *hdev)
 		hdev->rst_stats.flr_rst_cnt++;
 		break;
 	case HNAE3_IMP_RESET:
+		if (handle && handle->ae_algo->ops->handle_imp_error)
+			handle->ae_algo->ops->handle_imp_error(handle);
 		reg_val = hclge_read_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG);
 		hclge_write_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG,
 				BIT(HCLGE_VECTOR0_IMP_RESET_INT_B) | reg_val);
@@ -3517,6 +3520,9 @@ static void hclge_reset_event(struct pci_dev *pdev, struct hnae3_handle *handle)
 	else if (time_after(jiffies, (hdev->last_reset_time + 4 * 5 * HZ)))
 		hdev->reset_level = HNAE3_FUNC_RESET;
 
+	if (hdev->ppu_poison_ras_err)
+		hdev->ppu_poison_ras_err = false;
+
 	dev_info(&hdev->pdev->dev, "received reset event , reset type is %d",
 		 hdev->reset_level);
 
@@ -3545,6 +3551,27 @@ static void hclge_reset_timer(struct timer_list *t)
 	hclge_reset_event(hdev->pdev, NULL);
 }
 
+void hclge_handle_imp_error(struct hnae3_handle *handle)
+{
+	struct hclge_vport *vport = hclge_get_vport(handle);
+	struct hclge_dev *hdev = vport->back;
+	u32 reg_val;
+
+	if (test_and_clear_bit(HCLGE_IMP_RD_POISON, &hdev->imp_err_state)) {
+		dev_err(&hdev->pdev->dev, "Detected IMP RD poison!\n");
+		reg_val = hclge_read_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG) &
+			  ~BIT(HCLGE_VECTOR0_IMP_RD_POISON_B);
+		hclge_write_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG, reg_val);
+	}
+
+	if (test_and_clear_bit(HCLGE_IMP_CMDQ_ERROR, &hdev->imp_err_state)) {
+		dev_err(&hdev->pdev->dev, "Detected IMP CMDQ error!\n");
+		reg_val = hclge_read_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG) &
+			  ~BIT(HCLGE_VECTOR0_IMP_CMDQ_ERR_B);
+		hclge_write_dev(&hdev->hw, HCLGE_PF_OTHER_INT_REG, reg_val);
+	}
+}
+
 static void hclge_reset_subtask(struct hclge_dev *hdev)
 {
 	struct hnae3_ae_dev *ae_dev = pci_get_drvdata(hdev->pdev);
@@ -3560,6 +3587,7 @@ static void hclge_reset_subtask(struct hclge_dev *hdev)
 	 */
 	hdev->last_reset_time = jiffies;
 	hdev->reset_type = hclge_get_reset_level(ae_dev, &hdev->reset_pending);
+
 	if (hdev->reset_type != HNAE3_NONE_RESET)
 		hclge_reset(hdev);
 
@@ -9516,6 +9544,7 @@ static const struct hnae3_ae_ops hclge_ops = {
 	.mac_connect_phy = hclge_mac_connect_phy,
 	.mac_disconnect_phy = hclge_mac_disconnect_phy,
 	.restore_vlan_table = hclge_restore_vlan_table,
+	.handle_imp_error = hclge_handle_imp_error,
 };
 
 static struct hnae3_ae_algo ae_algo = {
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 688e425..7b7ba30 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -178,6 +178,8 @@ enum HLCGE_PORT_TYPE {
 #define HCLGE_VECTOR0_RX_CMDQ_INT_B	1
 
 #define HCLGE_VECTOR0_IMP_RESET_INT_B	1
+#define HCLGE_VECTOR0_IMP_CMDQ_ERR_B	4
+#define HCLGE_VECTOR0_IMP_RD_POISON_B	5
 
 #define HCLGE_MAC_DEFAULT_FRAME \
 	(ETH_HLEN + ETH_FCS_LEN + 2 * VLAN_HLEN + ETH_DATA_LEN)
@@ -676,6 +678,11 @@ enum HCLGE_MAC_ADDR_TYPE {
 	HCLGE_MAC_ADDR_MC
 };
 
+enum HCLGE_IMP_ERR_TYPE {
+	HCLGE_IMP_RD_POISON,
+	HCLGE_IMP_CMDQ_ERROR,
+};
+
 struct hclge_vport_vlan_cfg {
 	struct list_head node;
 	int hd_tbl_status;
@@ -777,6 +784,8 @@ struct hclge_dev {
 	u8 tc_num_last_time;
 	enum hclge_fc_mode fc_mode_last_time;
 	u8 support_sfp_query;
+	bool ppu_poison_ras_err;
+	unsigned long imp_err_state;
 
 #define HCLGE_FLAG_TC_BASE_SCH_MODE		1
 #define HCLGE_FLAG_VNET_BASE_SCH_MODE		2
-- 
2.7.4

Re: [PATCH net-next 04/11] net: hns3: fix mis-counting IRQ vector numbers issue

[Saeed Mahameed]

On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> From: Yonglong Liu <liuyonglong@huawei.com>
> 
> The num_msi_left means the vector numbers of NIC, but if the
> PF supported RoCE, it contains the vector numbers of NIC and
> RoCE(Not expected).
> 
> This may cause interrupts lost in some case, because of the
> NIC module used the vector resources which belongs to RoCE.
> 
> This patch corrects the value of num_msi_left to be equals to
> the vector numbers of NIC, and adjust the default tqp numbers
> according to the value of num_msi_left.
> 
> Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine &
> Compatibility Layer Support")
> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c   |  5 ++++-
>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 12
> ++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 3c64d70..a59d13f 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -1470,13 +1470,16 @@ static int hclge_vport_setup(struct
> hclge_vport *vport, u16 num_tqps)
>  {
>  	struct hnae3_handle *nic = &vport->nic;
>  	struct hclge_dev *hdev = vport->back;
> +	u16 alloc_tqps;
>  	int ret;
>  
>  	nic->pdev = hdev->pdev;
>  	nic->ae_algo = &ae_algo;
>  	nic->numa_node_mask = hdev->numa_node_mask;
>  
> -	ret = hclge_knic_setup(vport, num_tqps,
> +	alloc_tqps = min_t(u16, hdev->roce_base_msix_offset - 1, 


Why do you need the extra alloc_tqps ? just overwrite num_tqps, the
original value is not needed afterwards.

> num_tqps);
> +
> +	ret = hclge_knic_setup(vport, alloc_tqps,
>  			       hdev->num_tx_desc, hdev->num_rx_desc);
>  	if (ret)
>  		dev_err(&hdev->pdev->dev, "knic setup failed %d\n",
> ret);
> 

Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format

[Saeed Mahameed]

On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> From: Yufeng Mo <moyufeng@huawei.com>
> 
> This patch modifies firmware version display format in
> hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
> some optimizations for firmware version display format.
> 
> Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hnae3.h              |  9
> +++++++++
>  drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c       | 15
> +++++++++++++--
>  drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c   | 10
> +++++++++-
>  drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
> +++++++++--
>  4 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> index 48c7b70..a4624db 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
> @@ -179,6 +179,15 @@ struct hnae3_vector_info {
>  #define HNAE3_RING_GL_RX 0
>  #define HNAE3_RING_GL_TX 1
>  
> +#define HNAE3_FW_VERSION_BYTE3_SHIFT	24
> +#define HNAE3_FW_VERSION_BYTE3_MASK	GENMASK(31, 24)
> +#define HNAE3_FW_VERSION_BYTE2_SHIFT	16
> +#define HNAE3_FW_VERSION_BYTE2_MASK	GENMASK(23, 16)
> +#define HNAE3_FW_VERSION_BYTE1_SHIFT	8
> +#define HNAE3_FW_VERSION_BYTE1_MASK	GENMASK(15, 8)
> +#define HNAE3_FW_VERSION_BYTE0_SHIFT	0
> +#define HNAE3_FW_VERSION_BYTE0_MASK	GENMASK(7, 0)
> +
>  struct hnae3_ring_chain_node {
>  	struct hnae3_ring_chain_node *next;
>  	u32 tqp_index;
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index 5bff98a..e71c92b 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -527,6 +527,7 @@ static void hns3_get_drvinfo(struct net_device
> *netdev,
>  {
>  	struct hns3_nic_priv *priv = netdev_priv(netdev);
>  	struct hnae3_handle *h = priv->ae_handle;
> +	u32 fw_version;
>  
>  	if (!h->ae_algo->ops->get_fw_version) {
>  		netdev_err(netdev, "could not get fw version!\n");
> @@ -545,8 +546,18 @@ static void hns3_get_drvinfo(struct net_device
> *netdev,
>  		sizeof(drvinfo->bus_info));
>  	drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
>  
> -	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
> "0x%08x",
> -		 priv->ae_handle->ae_algo->ops->get_fw_version(h));
> +	fw_version = priv->ae_handle->ae_algo->ops->get_fw_version(h);
> +
> +	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
> +		 "%lu.%lu.%lu.%lu",
> +		 hnae3_get_field(fw_version,
> HNAE3_FW_VERSION_BYTE3_MASK,
> +				 HNAE3_FW_VERSION_BYTE3_SHIFT),
> +		 hnae3_get_field(fw_version,
> HNAE3_FW_VERSION_BYTE2_MASK,
> +				 HNAE3_FW_VERSION_BYTE2_SHIFT),
> +		 hnae3_get_field(fw_version,
> HNAE3_FW_VERSION_BYTE1_MASK,
> +				 HNAE3_FW_VERSION_BYTE1_SHIFT),
> +		 hnae3_get_field(fw_version,
> HNAE3_FW_VERSION_BYTE0_MASK,
> +				 HNAE3_FW_VERSION_BYTE0_SHIFT));
>  }
>  
>  static u32 hns3_get_link(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> index 22f6acd..c2320bf 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> @@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
>  	}
>  	hdev->fw_version = version;
>  
> -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
> version);
> +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE3_MASK,
> +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE2_MASK,
> +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE1_MASK,
> +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE0_MASK,
> +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
>  

Device name/string will not be printed now, what happens if i have
multiple devices ? at least print the device name as it was before

>  	return 0;
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
> index 652b796..004125b 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
> @@ -405,8 +405,15 @@ int hclgevf_cmd_init(struct hclgevf_dev *hdev)
>  	}
>  	hdev->fw_version = version;
>  
> -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
> version);
> -
> +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE3_MASK,
> +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE2_MASK,
> +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE1_MASK,
> +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
> +		     hnae3_get_field(version,
> HNAE3_FW_VERSION_BYTE0_MASK,
> +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
>  	return 0;
>  

Same.

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[Saeed Mahameed]

On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> From: Yonglong Liu <liuyonglong@huawei.com>
> 
> Some times just see the eth interface have been down/up via
> dmesg, but can not know why the eth down. So adds some debug
> messages to identify the cause for this.
> 

I really don't like this. your default msg lvl has NETIF_MSG_IFDOWN
turned on .. dumping every single operation that happens on your device
by default to kernel log is too much ! 

We should really consider using trace buffers with well defined
structures for vendor specific events. so we can use bpf filters and
state of the art tools for netdev debugging.

> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3_enet.c    | 24
> ++++++++++++++++++++
>  drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 26
> ++++++++++++++++++++++
>  .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 14 ++++++++++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 4d58c53..cff5d59 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -459,6 +459,10 @@ static int hns3_nic_net_open(struct net_device
> *netdev)
>  		h->ae_algo->ops->set_timer_task(priv->ae_handle, true);
>  
>  	hns3_config_xps(priv);
> +
> +	if (netif_msg_ifup(h))
> +		netdev_info(netdev, "net open\n");
> +
>  	return 0;
>  }
>  
> @@ -519,6 +523,9 @@ static int hns3_nic_net_stop(struct net_device
> *netdev)
>  	if (test_and_set_bit(HNS3_NIC_STATE_DOWN, &priv->state))
>  		return 0;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev, "net stop\n");
> +
>  	if (h->ae_algo->ops->set_timer_task)
>  		h->ae_algo->ops->set_timer_task(priv->ae_handle,
> false);
>  
> @@ -1550,6 +1557,9 @@ static int hns3_setup_tc(struct net_device
> *netdev, void *type_data)
>  	h = hns3_get_handle(netdev);
>  	kinfo = &h->kinfo;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev, "setup tc: num_tc=%d\n", tc);
> +
>  	return (kinfo->dcb_ops && kinfo->dcb_ops->setup_tc) ?
>  		kinfo->dcb_ops->setup_tc(h, tc, prio_tc) : -EOPNOTSUPP;
>  }
> @@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct
> net_device *netdev, int vf, u16 vlan,
>  	struct hnae3_handle *h = hns3_get_handle(netdev);
>  	int ret = -EIO;
>  
> +	if (netif_msg_ifdown(h))

why msg_ifdown ? looks like netif_msg_drv is more appropriate, for many
of the cases in this patch.

> +		netdev_info(netdev,
> +			    "set vf vlan: vf=%d, vlan=%d, qos=%d,
> vlan_proto=%d\n",
> +			    vf, vlan, qos, vlan_proto);
> +
>  	if (h->ae_algo->ops->set_vf_vlan_filter)
>  		ret = h->ae_algo->ops->set_vf_vlan_filter(h, vf, vlan,
>  							  qos,
> vlan_proto);
> @@ -1611,6 +1626,10 @@ static int hns3_nic_change_mtu(struct
> net_device *netdev, int new_mtu)
>  	if (!h->ae_algo->ops->set_mtu)
>  		return -EOPNOTSUPP;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev, "change mtu from %d to %d\n",
> +			    netdev->mtu, new_mtu);
> +
>  	ret = h->ae_algo->ops->set_mtu(h, new_mtu);
>  	if (ret)
>  		netdev_err(netdev, "failed to change MTU in hardware
> %d\n",
> @@ -4395,6 +4414,11 @@ int hns3_set_channels(struct net_device
> *netdev,
>  	if (kinfo->rss_size == new_tqp_num)
>  		return 0;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev,
> +			    "set channels: tqp_num=%d, rxfh=%d\n",
> +			    new_tqp_num, rxfh_configured);
> +
>  	ret = hns3_reset_notify(h, HNAE3_DOWN_CLIENT);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> index e71c92b..edb9845 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
> @@ -311,6 +311,9 @@ static void hns3_self_test(struct net_device
> *ndev,
>  	if (eth_test->flags != ETH_TEST_FL_OFFLINE)
>  		return;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(ndev, "self test start\n");
> +
>  	st_param[HNAE3_LOOP_APP][0] = HNAE3_LOOP_APP;
>  	st_param[HNAE3_LOOP_APP][1] =
>  			h->flags & HNAE3_SUPPORT_APP_LOOPBACK;
> @@ -374,6 +377,9 @@ static void hns3_self_test(struct net_device
> *ndev,
>  
>  	if (if_running)
>  		ndev->netdev_ops->ndo_open(ndev);
> +
> +	if (netif_msg_ifdown(h))
> +		netdev_info(ndev, "self test end\n");
>  }
>  
>  static int hns3_get_sset_count(struct net_device *netdev, int
> stringset)
> @@ -604,6 +610,11 @@ static int hns3_set_pauseparam(struct net_device
> *netdev,
>  {
>  	struct hnae3_handle *h = hns3_get_handle(netdev);
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev,
> +			    "set pauseparam: autoneg=%d, rx:%d,
> tx:%d\n",
> +			    param->autoneg, param->rx_pause, param-
> >tx_pause);
> +
>  	if (h->ae_algo->ops->set_pauseparam)
>  		return h->ae_algo->ops->set_pauseparam(h, param-
> >autoneg,
>  						       param->rx_pause,
> @@ -743,6 +754,13 @@ static int hns3_set_link_ksettings(struct
> net_device *netdev,
>  	if (cmd->base.speed == SPEED_1000 && cmd->base.duplex ==
> DUPLEX_HALF)
>  		return -EINVAL;
>  
> +	if (netif_msg_ifdown(handle))
> +		netdev_info(netdev,
> +			    "set link(%s): autoneg=%d, speed=%d,
> duplex=%d\n",
> +			    netdev->phydev ? "phy" : "mac",
> +			    cmd->base.autoneg, cmd->base.speed,
> +			    cmd->base.duplex);
> +
>  	/* Only support ksettings_set for netdev with phy attached for
> now */
>  	if (netdev->phydev)
>  		return phy_ethtool_ksettings_set(netdev->phydev, cmd);
> @@ -984,6 +1002,10 @@ static int hns3_nway_reset(struct net_device
> *netdev)
>  		return -EINVAL;
>  	}
>  
> +	if (netif_msg_ifdown(handle))
> +		netdev_info(netdev, "nway reset (using %s)\n",
> +			    phy ? "phy" : "mac");
> +
>  	if (phy)
>  		return genphy_restart_aneg(phy);
>  
> @@ -1308,6 +1330,10 @@ static int hns3_set_fecparam(struct net_device
> *netdev,
>  	if (!ops->set_fec)
>  		return -EOPNOTSUPP;
>  	fec_mode = eth_to_loc_fec(fec->fec);
> +
> +	if (netif_msg_ifdown(handle))
> +		netdev_info(netdev, "set fecparam: mode=%d\n",
> fec_mode);
> +
>  	return ops->set_fec(handle, fec_mode);
>  }
>  
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
> index bac4ce1..133e7c6 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
> @@ -201,6 +201,7 @@ static int hclge_client_setup_tc(struct hclge_dev
> *hdev)
>  static int hclge_ieee_setets(struct hnae3_handle *h, struct ieee_ets
> *ets)
>  {
>  	struct hclge_vport *vport = hclge_get_vport(h);
> +	struct net_device *netdev = h->kinfo.netdev;
>  	struct hclge_dev *hdev = vport->back;
>  	bool map_changed = false;
>  	u8 num_tc = 0;
> @@ -215,6 +216,9 @@ static int hclge_ieee_setets(struct hnae3_handle
> *h, struct ieee_ets *ets)
>  		return ret;
>  
>  	if (map_changed) {
> +		if (netif_msg_ifdown(h))
> +			netdev_info(netdev, "set ets\n");
> +
>  		ret = hclge_notify_client(hdev, HNAE3_DOWN_CLIENT);
>  		if (ret)
>  			return ret;
> @@ -300,6 +304,7 @@ static int hclge_ieee_getpfc(struct hnae3_handle
> *h, struct ieee_pfc *pfc)
>  static int hclge_ieee_setpfc(struct hnae3_handle *h, struct ieee_pfc
> *pfc)
>  {
>  	struct hclge_vport *vport = hclge_get_vport(h);
> +	struct net_device *netdev = h->kinfo.netdev;
>  	struct hclge_dev *hdev = vport->back;
>  	u8 i, j, pfc_map, *prio_tc;
>  
> @@ -325,6 +330,11 @@ static int hclge_ieee_setpfc(struct hnae3_handle
> *h, struct ieee_pfc *pfc)
>  	hdev->tm_info.hw_pfc_map = pfc_map;
>  	hdev->tm_info.pfc_en = pfc->pfc_en;
>  
> +	if (netif_msg_ifdown(h))
> +		netdev_info(netdev,
> +			    "set pfc: pfc_en=%d, pfc_map=%d,
> num_tc=%d\n",
> +			    pfc->pfc_en, pfc_map, hdev-
> >tm_info.num_tc);
> +
>  	hclge_tm_pfc_info_update(hdev);
>  
>  	return hclge_pause_setup_hw(hdev, false);
> @@ -345,8 +355,12 @@ static u8 hclge_getdcbx(struct hnae3_handle *h)
>  static u8 hclge_setdcbx(struct hnae3_handle *h, u8 mode)
>  {
>  	struct hclge_vport *vport = hclge_get_vport(h);
> +	struct net_device *netdev = h->kinfo.netdev;
>  	struct hclge_dev *hdev = vport->back;
>  
> +	if (netif_msg_drv(h))
> +		netdev_info(netdev, "set dcbx: mode=%d\n", mode);
> +
>  	/* No support for LLD_MANAGED modes or CEE */
>  	if ((mode & DCB_CAP_DCBX_LLD_MANAGED) ||
>  	    (mode & DCB_CAP_DCBX_VER_CEE) ||

Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Saeed Mahameed]

On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> From: Yunsheng Lin <linyunsheng@huawei.com>
> 
> The misc interrupt is used to schedule the reset and mailbox
> subtask, and a 1 sec timer is used to schedule the service
> subtask, which does periodic work.
> 
> This patch sets the above three subtask's affinity using the
> misc interrupt' affinity.
> 
> Also this patch setups a affinity notify for misc interrupt to
> allow user to change the above three subtask's affinity.
> 
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 59
> ++++++++++++++++++++--
>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  4 ++
>  2 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index f345095..fe45986 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -1270,6 +1270,12 @@ static int hclge_configure(struct hclge_dev
> *hdev)
>  
>  	hclge_init_kdump_kernel_config(hdev);
>  
> +	/* Set the init affinity based on pci func number */
> +	i = cpumask_weight(cpumask_of_node(dev_to_node(&hdev->pdev-
> >dev)));
> +	i = i ? PCI_FUNC(hdev->pdev->devfn) % i : 0;
> +	cpumask_set_cpu(cpumask_local_spread(i, dev_to_node(&hdev-
> >pdev->dev)),
> +			&hdev->affinity_mask);
> +
>  	return ret;
>  }
>  
> @@ -2502,14 +2508,16 @@ static void hclge_mbx_task_schedule(struct
> hclge_dev *hdev)
>  {
>  	if (!test_bit(HCLGE_STATE_CMD_DISABLE, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_MBX_SERVICE_SCHED, &hdev-
> >state))
> -		schedule_work(&hdev->mbx_service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
> system_wq,
> +			      &hdev->mbx_service_task);
>  }
>  
>  static void hclge_reset_task_schedule(struct hclge_dev *hdev)
>  {
>  	if (!test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev-
> >state))
> -		schedule_work(&hdev->rst_service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
> system_wq,
> +			      &hdev->rst_service_task);
>  }
>  
>  static void hclge_task_schedule(struct hclge_dev *hdev)
> @@ -2517,7 +2525,8 @@ static void hclge_task_schedule(struct
> hclge_dev *hdev)
>  	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
>  	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
> -		(void)schedule_work(&hdev->service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
> system_wq,
> +			      &hdev->service_task);
>  }
>  
>  static int hclge_get_mac_link_status(struct hclge_dev *hdev)
> @@ -2921,6 +2930,39 @@ static void hclge_get_misc_vector(struct
> hclge_dev *hdev)
>  	hdev->num_msi_used += 1;
>  }
>  
> +static void hclge_irq_affinity_notify(struct irq_affinity_notify
> *notify,
> +				      const cpumask_t *mask)
> +{
> +	struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
> +					      affinity_notify);
> +
> +	cpumask_copy(&hdev->affinity_mask, mask);
> +	del_timer_sync(&hdev->service_timer);
> +	hdev->service_timer.expires = jiffies + HZ;
> +	add_timer_on(&hdev->service_timer, cpumask_first(&hdev-
> >affinity_mask));
> +}

I don't see any relation between your misc irq vector and &hdev-
>service_timer, to me this looks like an abuse of the irq affinity API
to allow the user to move the service timer affinity.

> +
> +static void hclge_irq_affinity_release(struct kref *ref)
> +{
> +}
> +
> +static void hclge_misc_affinity_setup(struct hclge_dev *hdev)
> +{
> +	irq_set_affinity_hint(hdev->misc_vector.vector_irq,
> +			      &hdev->affinity_mask);
> +
> +	hdev->affinity_notify.notify = hclge_irq_affinity_notify;
> +	hdev->affinity_notify.release = hclge_irq_affinity_release;
> +	irq_set_affinity_notifier(hdev->misc_vector.vector_irq,
> +				  &hdev->affinity_notify);
> +}
> +
> +static void hclge_misc_affinity_teardown(struct hclge_dev *hdev)
> +{
> +	irq_set_affinity_notifier(hdev->misc_vector.vector_irq, NULL);
> +	irq_set_affinity_hint(hdev->misc_vector.vector_irq, NULL);
> +}
> +
>  static int hclge_misc_irq_init(struct hclge_dev *hdev)
>  {
>  	int ret;
> @@ -6151,7 +6193,10 @@ static void hclge_set_timer_task(struct
> hnae3_handle *handle, bool enable)
>  	struct hclge_dev *hdev = vport->back;
>  
>  	if (enable) {
> -		mod_timer(&hdev->service_timer, jiffies + HZ);
> +		del_timer_sync(&hdev->service_timer);
> +		hdev->service_timer.expires = jiffies + HZ;
> +		add_timer_on(&hdev->service_timer,
> +			     cpumask_first(&hdev->affinity_mask));
>  	} else {
>  		del_timer_sync(&hdev->service_timer);
>  		cancel_work_sync(&hdev->service_task);
> @@ -8809,6 +8854,11 @@ static int hclge_init_ae_dev(struct
> hnae3_ae_dev *ae_dev)
>  	INIT_WORK(&hdev->rst_service_task, hclge_reset_service_task);
>  	INIT_WORK(&hdev->mbx_service_task, hclge_mailbox_service_task);
>  
> +	/* Setup affinity after service timer setup because
> add_timer_on
> +	 * is called in affinity notify.
> +	 */
> +	hclge_misc_affinity_setup(hdev);
> +
>  	hclge_clear_all_event_cause(hdev);
>  	hclge_clear_resetting_state(hdev);
>  
> @@ -8970,6 +9020,7 @@ static void hclge_uninit_ae_dev(struct
> hnae3_ae_dev *ae_dev)
>  	struct hclge_dev *hdev = ae_dev->priv;
>  	struct hclge_mac *mac = &hdev->hw.mac;
>  
> +	hclge_misc_affinity_teardown(hdev);
>  	hclge_state_uninit(hdev);
>  
>  	if (mac->phydev)
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> index 6a12285..14df23c 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> @@ -864,6 +864,10 @@ struct hclge_dev {
>  
>  	DECLARE_KFIFO(mac_tnl_log, struct hclge_mac_tnl_stats,
>  		      HCLGE_MAC_TNL_LOG_SIZE);
> +
> +	/* affinity mask and notify for misc interrupt */
> +	cpumask_t affinity_mask;
> +	struct irq_affinity_notify affinity_notify;
>  };
>  
>  /* VPort level vlan tag configuration for TX direction */

Re: [PATCH net-next 04/11] net: hns3: fix mis-counting IRQ vector numbers issue

[tanhuazhong]

On 2019/7/25 2:28, Saeed Mahameed wrote:
> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>> From: Yonglong Liu <liuyonglong@huawei.com>
>>
>> The num_msi_left means the vector numbers of NIC, but if the
>> PF supported RoCE, it contains the vector numbers of NIC and
>> RoCE(Not expected).
>>
>> This may cause interrupts lost in some case, because of the
>> NIC module used the vector resources which belongs to RoCE.
>>
>> This patch corrects the value of num_msi_left to be equals to
>> the vector numbers of NIC, and adjust the default tqp numbers
>> according to the value of num_msi_left.
>>
>> Fixes: 46a3df9f9718 ("net: hns3: Add HNS3 Acceleration Engine &
>> Compatibility Layer Support")
>> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
>> Signed-off-by: Peng Li <lipeng321@huawei.com>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c   |  5 ++++-
>>   drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 12
>> ++++++++++--
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> index 3c64d70..a59d13f 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> @@ -1470,13 +1470,16 @@ static int hclge_vport_setup(struct
>> hclge_vport *vport, u16 num_tqps)
>>   {
>>   	struct hnae3_handle *nic = &vport->nic;
>>   	struct hclge_dev *hdev = vport->back;
>> +	u16 alloc_tqps;
>>   	int ret;
>>   
>>   	nic->pdev = hdev->pdev;
>>   	nic->ae_algo = &ae_algo;
>>   	nic->numa_node_mask = hdev->numa_node_mask;
>>   
>> -	ret = hclge_knic_setup(vport, num_tqps,
>> +	alloc_tqps = min_t(u16, hdev->roce_base_msix_offset - 1,
> 
> 
> Why do you need the extra alloc_tqps ? just overwrite num_tqps, the
> original value is not needed afterwards.
> 

Yes, using num_tqps is better.
I will remove the extra alloc_tqps in V2.
Thanks.

>> num_tqps);
>> +
>> +	ret = hclge_knic_setup(vport, alloc_tqps,
>>   			       hdev->num_tx_desc, hdev->num_rx_desc);
>>   	if (ret)
>>   		dev_err(&hdev->pdev->dev, "knic setup failed %d\n",
>> ret);
>>

Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Yunsheng Lin]

On 2019/7/25 3:24, Saeed Mahameed wrote:
> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> The misc interrupt is used to schedule the reset and mailbox
>> subtask, and a 1 sec timer is used to schedule the service
>> subtask, which does periodic work.
>>
>> This patch sets the above three subtask's affinity using the
>> misc interrupt' affinity.
>>
>> Also this patch setups a affinity notify for misc interrupt to
>> allow user to change the above three subtask's affinity.
>>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> Signed-off-by: Peng Li <lipeng321@huawei.com>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>> ---
>>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 59
>> ++++++++++++++++++++--
>>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  4 ++
>>  2 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> index f345095..fe45986 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> @@ -1270,6 +1270,12 @@ static int hclge_configure(struct hclge_dev
>> *hdev)
>>  
>>  	hclge_init_kdump_kernel_config(hdev);
>>  
>> +	/* Set the init affinity based on pci func number */
>> +	i = cpumask_weight(cpumask_of_node(dev_to_node(&hdev->pdev-
>>> dev)));
>> +	i = i ? PCI_FUNC(hdev->pdev->devfn) % i : 0;
>> +	cpumask_set_cpu(cpumask_local_spread(i, dev_to_node(&hdev-
>>> pdev->dev)),
>> +			&hdev->affinity_mask);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -2502,14 +2508,16 @@ static void hclge_mbx_task_schedule(struct
>> hclge_dev *hdev)
>>  {
>>  	if (!test_bit(HCLGE_STATE_CMD_DISABLE, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_MBX_SERVICE_SCHED, &hdev-
>>> state))
>> -		schedule_work(&hdev->mbx_service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
>> system_wq,
>> +			      &hdev->mbx_service_task);
>>  }
>>  
>>  static void hclge_reset_task_schedule(struct hclge_dev *hdev)
>>  {
>>  	if (!test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev-
>>> state))
>> -		schedule_work(&hdev->rst_service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
>> system_wq,
>> +			      &hdev->rst_service_task);
>>  }
>>  
>>  static void hclge_task_schedule(struct hclge_dev *hdev)
>> @@ -2517,7 +2525,8 @@ static void hclge_task_schedule(struct
>> hclge_dev *hdev)
>>  	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
>>  	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
>> -		(void)schedule_work(&hdev->service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask),
>> system_wq,
>> +			      &hdev->service_task);
>>  }
>>  
>>  static int hclge_get_mac_link_status(struct hclge_dev *hdev)
>> @@ -2921,6 +2930,39 @@ static void hclge_get_misc_vector(struct
>> hclge_dev *hdev)
>>  	hdev->num_msi_used += 1;
>>  }
>>  
>> +static void hclge_irq_affinity_notify(struct irq_affinity_notify
>> *notify,
>> +				      const cpumask_t *mask)
>> +{
>> +	struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
>> +					      affinity_notify);
>> +
>> +	cpumask_copy(&hdev->affinity_mask, mask);
>> +	del_timer_sync(&hdev->service_timer);
>> +	hdev->service_timer.expires = jiffies + HZ;
>> +	add_timer_on(&hdev->service_timer, cpumask_first(&hdev-
>>> affinity_mask));
>> +}
> 
> I don't see any relation between your misc irq vector and &hdev-
>> service_timer, to me this looks like an abuse of the irq affinity API
> to allow the user to move the service timer affinity.

Hi, thanks for reviewing.

hdev->service_timer is used to schedule the periodic work
queue hdev->service_task， we want all the management work
queue including hdev->service_task to bind to the same cpu
to improve cache and power efficiency, it is better to move
service timer affinity according to that.

The hdev->service_task is changed to delay work queue in
next patch " net: hns3: make hclge_service use delayed workqueue",
So the affinity in the timer of the delay work queue is automatically
set to the affinity of the delay work queue, we will move the
"make hclge_service use delayed workqueue" patch before the
"add interrupt affinity support for misc interrupt" patch, so
we do not have to set service timer affinity explicitly.

Also, There is there work queues(mbx_service_task, service_task,
rst_service_task) in the hns3 driver, we plan to combine them
to one or two workqueue to improve efficiency and readability.

> 
>> +
>> +static void hclge_irq_affinity_release(struct kref *ref)
>> +{
>> +}
>> +
>> +static void hclge_misc_affinity_setup(struct hclge_dev *hdev)
>> +{
>> +	irq_set_affinity_hint(hdev->misc_vector.vector_irq,
>> +			      &hdev->affinity_mask);
>> +
>> +	hdev->affinity_notify.notify = hclge_irq_affinity_notify;
>> +	hdev->affinity_notify.release = hclge_irq_affinity_release;
>> +	irq_set_affinity_notifier(hdev->misc_vector.vector_irq,
>> +				  &hdev->affinity_notify);
>> +}
>> +
>> +static void hclge_misc_affinity_teardown(struct hclge_dev *hdev)
>> +{
>> +	irq_set_affinity_notifier(hdev->misc_vector.vector_irq, NULL);
>> +	irq_set_affinity_hint(hdev->misc_vector.vector_irq, NULL);
>> +}
>> +
>>  static int hclge_misc_irq_init(struct hclge_dev *hdev)
>>  {
>>  	int ret;
>> @@ -6151,7 +6193,10 @@ static void hclge_set_timer_task(struct
>> hnae3_handle *handle, bool enable)
>>  	struct hclge_dev *hdev = vport->back;
>>  
>>  	if (enable) {
>> -		mod_timer(&hdev->service_timer, jiffies + HZ);
>> +		del_timer_sync(&hdev->service_timer);
>> +		hdev->service_timer.expires = jiffies + HZ;
>> +		add_timer_on(&hdev->service_timer,
>> +			     cpumask_first(&hdev->affinity_mask));
>>  	} else {
>>  		del_timer_sync(&hdev->service_timer);
>>  		cancel_work_sync(&hdev->service_task);
>> @@ -8809,6 +8854,11 @@ static int hclge_init_ae_dev(struct
>> hnae3_ae_dev *ae_dev)
>>  	INIT_WORK(&hdev->rst_service_task, hclge_reset_service_task);
>>  	INIT_WORK(&hdev->mbx_service_task, hclge_mailbox_service_task);
>>  
>> +	/* Setup affinity after service timer setup because
>> add_timer_on
>> +	 * is called in affinity notify.
>> +	 */
>> +	hclge_misc_affinity_setup(hdev);
>> +
>>  	hclge_clear_all_event_cause(hdev);
>>  	hclge_clear_resetting_state(hdev);
>>  
>> @@ -8970,6 +9020,7 @@ static void hclge_uninit_ae_dev(struct
>> hnae3_ae_dev *ae_dev)
>>  	struct hclge_dev *hdev = ae_dev->priv;
>>  	struct hclge_mac *mac = &hdev->hw.mac;
>>  
>> +	hclge_misc_affinity_teardown(hdev);
>>  	hclge_state_uninit(hdev);
>>  
>>  	if (mac->phydev)
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
>> index 6a12285..14df23c 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
>> @@ -864,6 +864,10 @@ struct hclge_dev {
>>  
>>  	DECLARE_KFIFO(mac_tnl_log, struct hclge_mac_tnl_stats,
>>  		      HCLGE_MAC_TNL_LOG_SIZE);
>> +
>> +	/* affinity mask and notify for misc interrupt */
>> +	cpumask_t affinity_mask;
>> +	struct irq_affinity_notify affinity_notify;
>>  };
>>  
>>  /* VPort level vlan tag configuration for TX direction */

Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format

[tanhuazhong]

On 2019/7/25 2:34, Saeed Mahameed wrote:
> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>> From: Yufeng Mo <moyufeng@huawei.com>
>>
>> This patch modifies firmware version display format in
>> hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
>> some optimizations for firmware version display format.
>>
>> Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
>> Signed-off-by: Peng Li <lipeng321@huawei.com>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hnae3.h              |  9
>> +++++++++
>>   drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c       | 15
>> +++++++++++++--
>>   drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c   | 10
>> +++++++++-
>>   drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
>> +++++++++--
>>   4 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> index 48c7b70..a4624db 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> @@ -179,6 +179,15 @@ struct hnae3_vector_info {
>>   #define HNAE3_RING_GL_RX 0
>>   #define HNAE3_RING_GL_TX 1
>>   
>> +#define HNAE3_FW_VERSION_BYTE3_SHIFT	24
>> +#define HNAE3_FW_VERSION_BYTE3_MASK	GENMASK(31, 24)
>> +#define HNAE3_FW_VERSION_BYTE2_SHIFT	16
>> +#define HNAE3_FW_VERSION_BYTE2_MASK	GENMASK(23, 16)
>> +#define HNAE3_FW_VERSION_BYTE1_SHIFT	8
>> +#define HNAE3_FW_VERSION_BYTE1_MASK	GENMASK(15, 8)
>> +#define HNAE3_FW_VERSION_BYTE0_SHIFT	0
>> +#define HNAE3_FW_VERSION_BYTE0_MASK	GENMASK(7, 0)
>> +
>>   struct hnae3_ring_chain_node {
>>   	struct hnae3_ring_chain_node *next;
>>   	u32 tqp_index;
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> index 5bff98a..e71c92b 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> @@ -527,6 +527,7 @@ static void hns3_get_drvinfo(struct net_device
>> *netdev,
>>   {
>>   	struct hns3_nic_priv *priv = netdev_priv(netdev);
>>   	struct hnae3_handle *h = priv->ae_handle;
>> +	u32 fw_version;
>>   
>>   	if (!h->ae_algo->ops->get_fw_version) {
>>   		netdev_err(netdev, "could not get fw version!\n");
>> @@ -545,8 +546,18 @@ static void hns3_get_drvinfo(struct net_device
>> *netdev,
>>   		sizeof(drvinfo->bus_info));
>>   	drvinfo->bus_info[ETHTOOL_BUSINFO_LEN - 1] = '\0';
>>   
>> -	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>> "0x%08x",
>> -		 priv->ae_handle->ae_algo->ops->get_fw_version(h));
>> +	fw_version = priv->ae_handle->ae_algo->ops->get_fw_version(h);
>> +
>> +	snprintf(drvinfo->fw_version, sizeof(drvinfo->fw_version),
>> +		 "%lu.%lu.%lu.%lu",
>> +		 hnae3_get_field(fw_version,
>> HNAE3_FW_VERSION_BYTE3_MASK,
>> +				 HNAE3_FW_VERSION_BYTE3_SHIFT),
>> +		 hnae3_get_field(fw_version,
>> HNAE3_FW_VERSION_BYTE2_MASK,
>> +				 HNAE3_FW_VERSION_BYTE2_SHIFT),
>> +		 hnae3_get_field(fw_version,
>> HNAE3_FW_VERSION_BYTE1_MASK,
>> +				 HNAE3_FW_VERSION_BYTE1_SHIFT),
>> +		 hnae3_get_field(fw_version,
>> HNAE3_FW_VERSION_BYTE0_MASK,
>> +				 HNAE3_FW_VERSION_BYTE0_SHIFT));
>>   }
>>   
>>   static u32 hns3_get_link(struct net_device *netdev)
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>> index 22f6acd..c2320bf 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>> @@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
>>   	}
>>   	hdev->fw_version = version;
>>   
>> -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
>> version);
>> +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE3_MASK,
>> +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE2_MASK,
>> +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE1_MASK,
>> +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE0_MASK,
>> +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
>>   
> 
> Device name/string will not be printed now, what happens if i have
> multiple devices ? at least print the device name as it was before
>
Since on each board we only have one firmware, the firmware
version is same per device, and will not change when running.
So pr_info_once() looks good for this case.

BTW, maybe we should change below print in the end of
hclge_init_ae_dev(), use dev_info() instead of pr_info(),
then we can know that which device has already initialized.
I will send other patch to do that, is it acceptable for you?

"pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);"

Thanks.

>>   	return 0;
>>   
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
>> index 652b796..004125b 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c
>> @@ -405,8 +405,15 @@ int hclgevf_cmd_init(struct hclgevf_dev *hdev)
>>   	}
>>   	hdev->fw_version = version;
>>   
>> -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
>> version);
>> -
>> +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE3_MASK,
>> +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE2_MASK,
>> +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE1_MASK,
>> +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
>> +		     hnae3_get_field(version,
>> HNAE3_FW_VERSION_BYTE0_MASK,
>> +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
>>   	return 0;
>>   
> 
> Same.
> 

Same
:)

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[liuyonglong]

On 2019/7/25 3:12, Saeed Mahameed wrote:
> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>> From: Yonglong Liu <liuyonglong@huawei.com>
>>
>> Some times just see the eth interface have been down/up via
>> dmesg, but can not know why the eth down. So adds some debug
>> messages to identify the cause for this.
>>
> 
> I really don't like this. your default msg lvl has NETIF_MSG_IFDOWN
> turned on .. dumping every single operation that happens on your device
> by default to kernel log is too much ! 
> 
> We should really consider using trace buffers with well defined
> structures for vendor specific events. so we can use bpf filters and
> state of the art tools for netdev debugging.
> 

We do this because we can just see a link down message in dmesg, and had
take a long time to found the cause of link down, just because another
user changed the settings.

We can change the net_open/net_stop/dcbnl_ops to msg_drv (not default
turned on),  and want to keep the others default print to kernel log,
is it acceptable?

>> @@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct
>> net_device *netdev, int vf, u16 vlan,
>>  	struct hnae3_handle *h = hns3_get_handle(netdev);
>>  	int ret = -EIO;
>>  
>> +	if (netif_msg_ifdown(h))
> 
> why msg_ifdown ? looks like netif_msg_drv is more appropriate, for many
> of the cases in this patch.
> 

This operation may cause link down, so we use msg_ifdown.

>> +		netdev_info(netdev,
>> +			    "set vf vlan: vf=%d, vlan=%d, qos=%d,
>> vlan_proto=%d\n",
>> +			    vf, vlan, qos, vlan_proto);
>> +
>>  	if (h->ae_algo->ops->set_vf_vlan_filter)
>>  		ret = h->ae_algo->ops->set_vf_vlan_filter(h, vf, vlan,
>>  							  qos,
>> vlan_proto);
>> @@ -1611,6 +1626,10 @@ static int hns3_nic_change_mtu(struct
>> net_device *netdev, int new_mtu)
>>  	if (!h->ae_algo->ops->set_mtu)
>>  		return -EOPNOTSUPP;
>>  
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(netdev, "change mtu from %d to %d\n",
>> +			    netdev->mtu, new_mtu);
>> +
>>  	ret = h->ae_algo->ops->set_mtu(h, new_mtu);
>>  	if (ret)
>>  		netdev_err(netdev, "failed to change MTU in hardware
>> %d\n",
>> @@ -4395,6 +4414,11 @@ int hns3_set_channels(struct net_device
>> *netdev,
>>  	if (kinfo->rss_size == new_tqp_num)
>>  		return 0;
>>  
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(netdev,
>> +			    "set channels: tqp_num=%d, rxfh=%d\n",
>> +			    new_tqp_num, rxfh_configured);
>> +
>>  	ret = hns3_reset_notify(h, HNAE3_DOWN_CLIENT);
>>  	if (ret)
>>  		return ret;
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> index e71c92b..edb9845 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> @@ -311,6 +311,9 @@ static void hns3_self_test(struct net_device
>> *ndev,
>>  	if (eth_test->flags != ETH_TEST_FL_OFFLINE)
>>  		return;
>>  
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(ndev, "self test start\n");
>> +
>>  	st_param[HNAE3_LOOP_APP][0] = HNAE3_LOOP_APP;
>>  	st_param[HNAE3_LOOP_APP][1] =
>>  			h->flags & HNAE3_SUPPORT_APP_LOOPBACK;
>> @@ -374,6 +377,9 @@ static void hns3_self_test(struct net_device
>> *ndev,
>>  
>>  	if (if_running)
>>  		ndev->netdev_ops->ndo_open(ndev);
>> +
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(ndev, "self test end\n");
>>  }
>>  
>>  static int hns3_get_sset_count(struct net_device *netdev, int
>> stringset)
>> @@ -604,6 +610,11 @@ static int hns3_set_pauseparam(struct net_device
>> *netdev,
>>  {
>>  	struct hnae3_handle *h = hns3_get_handle(netdev);
>>  
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(netdev,
>> +			    "set pauseparam: autoneg=%d, rx:%d,
>> tx:%d\n",
>> +			    param->autoneg, param->rx_pause, param-
>>> tx_pause);
>> +
>>  	if (h->ae_algo->ops->set_pauseparam)
>>  		return h->ae_algo->ops->set_pauseparam(h, param-
>>> autoneg,
>>  						       param->rx_pause,
>> @@ -743,6 +754,13 @@ static int hns3_set_link_ksettings(struct
>> net_device *netdev,
>>  	if (cmd->base.speed == SPEED_1000 && cmd->base.duplex ==
>> DUPLEX_HALF)
>>  		return -EINVAL;
>>  
>> +	if (netif_msg_ifdown(handle))
>> +		netdev_info(netdev,
>> +			    "set link(%s): autoneg=%d, speed=%d,
>> duplex=%d\n",
>> +			    netdev->phydev ? "phy" : "mac",
>> +			    cmd->base.autoneg, cmd->base.speed,
>> +			    cmd->base.duplex);
>> +
>>  	/* Only support ksettings_set for netdev with phy attached for
>> now */
>>  	if (netdev->phydev)
>>  		return phy_ethtool_ksettings_set(netdev->phydev, cmd);
>> @@ -984,6 +1002,10 @@ static int hns3_nway_reset(struct net_device
>> *netdev)
>>  		return -EINVAL;
>>  	}
>>  
>> +	if (netif_msg_ifdown(handle))
>> +		netdev_info(netdev, "nway reset (using %s)\n",
>> +			    phy ? "phy" : "mac");
>> +
>>  	if (phy)
>>  		return genphy_restart_aneg(phy);
>>  
>> @@ -1308,6 +1330,10 @@ static int hns3_set_fecparam(struct net_device
>> *netdev,
>>  	if (!ops->set_fec)
>>  		return -EOPNOTSUPP;
>>  	fec_mode = eth_to_loc_fec(fec->fec);
>> +
>> +	if (netif_msg_ifdown(handle))
>> +		netdev_info(netdev, "set fecparam: mode=%d\n",
>> fec_mode);
>> +
>>  	return ops->set_fec(handle, fec_mode);
>>  }
>>  
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
>> index bac4ce1..133e7c6 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c
>> @@ -201,6 +201,7 @@ static int hclge_client_setup_tc(struct hclge_dev
>> *hdev)
>>  static int hclge_ieee_setets(struct hnae3_handle *h, struct ieee_ets
>> *ets)
>>  {
>>  	struct hclge_vport *vport = hclge_get_vport(h);
>> +	struct net_device *netdev = h->kinfo.netdev;
>>  	struct hclge_dev *hdev = vport->back;
>>  	bool map_changed = false;
>>  	u8 num_tc = 0;
>> @@ -215,6 +216,9 @@ static int hclge_ieee_setets(struct hnae3_handle
>> *h, struct ieee_ets *ets)
>>  		return ret;
>>  
>>  	if (map_changed) {
>> +		if (netif_msg_ifdown(h))
>> +			netdev_info(netdev, "set ets\n");
>> +
>>  		ret = hclge_notify_client(hdev, HNAE3_DOWN_CLIENT);
>>  		if (ret)
>>  			return ret;
>> @@ -300,6 +304,7 @@ static int hclge_ieee_getpfc(struct hnae3_handle
>> *h, struct ieee_pfc *pfc)
>>  static int hclge_ieee_setpfc(struct hnae3_handle *h, struct ieee_pfc
>> *pfc)
>>  {
>>  	struct hclge_vport *vport = hclge_get_vport(h);
>> +	struct net_device *netdev = h->kinfo.netdev;
>>  	struct hclge_dev *hdev = vport->back;
>>  	u8 i, j, pfc_map, *prio_tc;
>>  
>> @@ -325,6 +330,11 @@ static int hclge_ieee_setpfc(struct hnae3_handle
>> *h, struct ieee_pfc *pfc)
>>  	hdev->tm_info.hw_pfc_map = pfc_map;
>>  	hdev->tm_info.pfc_en = pfc->pfc_en;
>>  
>> +	if (netif_msg_ifdown(h))
>> +		netdev_info(netdev,
>> +			    "set pfc: pfc_en=%d, pfc_map=%d,
>> num_tc=%d\n",
>> +			    pfc->pfc_en, pfc_map, hdev-
>>> tm_info.num_tc);
>> +
>>  	hclge_tm_pfc_info_update(hdev);
>>  
>>  	return hclge_pause_setup_hw(hdev, false);
>> @@ -345,8 +355,12 @@ static u8 hclge_getdcbx(struct hnae3_handle *h)
>>  static u8 hclge_setdcbx(struct hnae3_handle *h, u8 mode)
>>  {
>>  	struct hclge_vport *vport = hclge_get_vport(h);
>> +	struct net_device *netdev = h->kinfo.netdev;
>>  	struct hclge_dev *hdev = vport->back;
>>  
>> +	if (netif_msg_drv(h))
>> +		netdev_info(netdev, "set dcbx: mode=%d\n", mode);
>> +
>>  	/* No support for LLD_MANAGED modes or CEE */
>>  	if ((mode & DCB_CAP_DCBX_LLD_MANAGED) ||
>>  	    (mode & DCB_CAP_DCBX_VER_CEE) ||

Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format

[Saeed Mahameed]

On Thu, 2019-07-25 at 10:34 +0800, tanhuazhong wrote:
> 
> On 2019/7/25 2:34, Saeed Mahameed wrote:
> > On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> > > From: Yufeng Mo <moyufeng@huawei.com>
> > > 
> > > This patch modifies firmware version display format in
> > > hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
> > > some optimizations for firmware version display format.
> > > 
> > > Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
> > > Signed-off-by: Peng Li <lipeng321@huawei.com>
> > > Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> > > ---
> > >   drivers/net/ethernet/hisilicon/hns3/hnae3.h              |  9
> > > +++++++++
> > >   drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c       | 15
> > > +++++++++++++--
> > >   drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c   | 10
> > > +++++++++-
> > >   drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
> > > +++++++++--
> > >   4 files changed, 40 insertions(+), 5 deletions(-)
> > > 
> > > 

[...]

> > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
> > > @@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
> > >   	}
> > >   	hdev->fw_version = version;
> > >   
> > > -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
> > > version);
> > > +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
> > > +		     hnae3_get_field(version,
> > > HNAE3_FW_VERSION_BYTE3_MASK,
> > > +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
> > > +		     hnae3_get_field(version,
> > > HNAE3_FW_VERSION_BYTE2_MASK,
> > > +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
> > > +		     hnae3_get_field(version,
> > > HNAE3_FW_VERSION_BYTE1_MASK,
> > > +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
> > > +		     hnae3_get_field(version,
> > > HNAE3_FW_VERSION_BYTE0_MASK,
> > > +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
> > >   
> > 
> > Device name/string will not be printed now, what happens if i have
> > multiple devices ? at least print the device name as it was before
> > 
> Since on each board we only have one firmware, the firmware
> version is same per device, and will not change when running.
> So pr_info_once() looks good for this case.
> 

boards change too often to have such static assumption.

> BTW, maybe we should change below print in the end of
> hclge_init_ae_dev(), use dev_info() instead of pr_info(),
> then we can know that which device has already initialized.
> I will send other patch to do that, is it acceptable for you?
> 
> "pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);"
> 

I would avoid using pr_info when i can ! if you have the option to
print with dev information as it was before that is preferable.

Thanks,
Saeed.

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[Saeed Mahameed]

On Thu, 2019-07-25 at 20:28 +0800, liuyonglong wrote:
> 
> On 2019/7/25 3:12, Saeed Mahameed wrote:
> > On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> > > From: Yonglong Liu <liuyonglong@huawei.com>
> > > 
> > > Some times just see the eth interface have been down/up via
> > > dmesg, but can not know why the eth down. So adds some debug
> > > messages to identify the cause for this.
> > > 
> > 
> > I really don't like this. your default msg lvl has NETIF_MSG_IFDOWN
> > turned on .. dumping every single operation that happens on your
> > device
> > by default to kernel log is too much ! 
> > 
> > We should really consider using trace buffers with well defined
> > structures for vendor specific events. so we can use bpf filters
> > and
> > state of the art tools for netdev debugging.
> > 
> 
> We do this because we can just see a link down message in dmesg, and
> had
> take a long time to found the cause of link down, just because
> another
> user changed the settings.
> 
> We can change the net_open/net_stop/dcbnl_ops to msg_drv (not default
> turned on),  and want to keep the others default print to kernel log,
> is it acceptable?
> 

acceptable as long as debug information are kept off by default and
your driver doens't spam the kernel log.

you should use dynamic debug [1] and/or "off by default" msg lvls for
debugging information..

I couldn't find any rules regarding what to put in kernel log, Maybe
someone can share ?. but i vaguely remember that the recommendation
for device drivers is to put nothing, only error/warning messages.

[1] 
https://www.kernel.org/doc/html/v4.15/admin-guide/dynamic-debug-howto.html

> > > @@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct
> > > net_device *netdev, int vf, u16 vlan,
> > >  	struct hnae3_handle *h = hns3_get_handle(netdev);
> > >  	int ret = -EIO;
> > >  
> > > +	if (netif_msg_ifdown(h))
> > 
> > why msg_ifdown ? looks like netif_msg_drv is more appropriate, for
> > many
> > of the cases in this patch.
> > 
> 
> This operation may cause link down, so we use msg_ifdown.
> 

ifdown isn't link down.. 

to be honest, I couldn't find any documentation explaining how/when to
use msg lvls, (i didn't look too deep though), by looking at other
drivers, my interpretations is:

ifdup (open/boot up flow)
ifdwon (close/teardown flow)
drv (driver based or dynamic flows) 
etc .. 

-Saeed.

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[Jakub Kicinski]

On Thu, 25 Jul 2019 21:59:08 +0000, Saeed Mahameed wrote:
> I couldn't find any rules regarding what to put in kernel log, Maybe
> someone can share ?. but i vaguely remember that the recommendation
> for device drivers is to put nothing, only error/warning messages.

FWIW my understanding is also that only error/warning messages should
be printed. IOW things which should "never happen".

There are some historical exceptions. Probe logs for instance may be
useful, because its not trivial to get to the device if probe fails.

Another one is ethtool flashing, if it takes time we used to print into
logs some message like "please wait patiently". But since Jiri added
the progress messages in devlink that's no longer necessary.

For the messages which are basically printing the name of the function
or name of the function and their args - we have ftrace.

That's my $0.02 :)

Re: [PATCH net-next 06/11] net: hns3: modify firmware version display format

[tanhuazhong]

On 2019/7/26 5:32, Saeed Mahameed wrote:
> On Thu, 2019-07-25 at 10:34 +0800, tanhuazhong wrote:
>>
>> On 2019/7/25 2:34, Saeed Mahameed wrote:
>>> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>>>> From: Yufeng Mo <moyufeng@huawei.com>
>>>>
>>>> This patch modifies firmware version display format in
>>>> hclge(vf)_cmd_init() and hns3_get_drvinfo(). Also, adds
>>>> some optimizations for firmware version display format.
>>>>
>>>> Signed-off-by: Yufeng Mo <moyufeng@huawei.com>
>>>> Signed-off-by: Peng Li <lipeng321@huawei.com>
>>>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>>>> ---
>>>>    drivers/net/ethernet/hisilicon/hns3/hnae3.h              |  9
>>>> +++++++++
>>>>    drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c       | 15
>>>> +++++++++++++--
>>>>    drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c   | 10
>>>> +++++++++-
>>>>    drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_cmd.c | 11
>>>> +++++++++--
>>>>    4 files changed, 40 insertions(+), 5 deletions(-)
>>>>
>>>>
> 
> [...]
> 
>>>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>>>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_cmd.c
>>>> @@ -419,7 +419,15 @@ int hclge_cmd_init(struct hclge_dev *hdev)
>>>>    	}
>>>>    	hdev->fw_version = version;
>>>>    
>>>> -	dev_info(&hdev->pdev->dev, "The firmware version is %08x\n",
>>>> version);
>>>> +	pr_info_once("The firmware version is %lu.%lu.%lu.%lu\n",
>>>> +		     hnae3_get_field(version,
>>>> HNAE3_FW_VERSION_BYTE3_MASK,
>>>> +				     HNAE3_FW_VERSION_BYTE3_SHIFT),
>>>> +		     hnae3_get_field(version,
>>>> HNAE3_FW_VERSION_BYTE2_MASK,
>>>> +				     HNAE3_FW_VERSION_BYTE2_SHIFT),
>>>> +		     hnae3_get_field(version,
>>>> HNAE3_FW_VERSION_BYTE1_MASK,
>>>> +				     HNAE3_FW_VERSION_BYTE1_SHIFT),
>>>> +		     hnae3_get_field(version,
>>>> HNAE3_FW_VERSION_BYTE0_MASK,
>>>> +				     HNAE3_FW_VERSION_BYTE0_SHIFT));
>>>>    
>>>
>>> Device name/string will not be printed now, what happens if i have
>>> multiple devices ? at least print the device name as it was before
>>>
>> Since on each board we only have one firmware, the firmware
>> version is same per device, and will not change when running.
>> So pr_info_once() looks good for this case.
>>
> 
> boards change too often to have such static assumption.

Ok, I will use dev_info instead of pr_info here.

> 
>> BTW, maybe we should change below print in the end of
>> hclge_init_ae_dev(), use dev_info() instead of pr_info(),
>> then we can know that which device has already initialized.
>> I will send other patch to do that, is it acceptable for you?
>>
>> "pr_info("%s driver initialization finished.\n", HCLGE_DRIVER_NAME);"
>>
> 
> I would avoid using pr_info when i can ! if you have the option to
> print with dev information as it was before that is preferable.
> 
> Thanks,
> Saeed.
> 

Thanks,
Huazhong.

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[liuyonglong]

We will change all of them to netif_msg_drv() which is default off
Thanks for your reply!

On 2019/7/26 5:59, Saeed Mahameed wrote:
> On Thu, 2019-07-25 at 20:28 +0800, liuyonglong wrote:
>>
>> On 2019/7/25 3:12, Saeed Mahameed wrote:
>>> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>>>> From: Yonglong Liu <liuyonglong@huawei.com>
>>>>
>>>> Some times just see the eth interface have been down/up via
>>>> dmesg, but can not know why the eth down. So adds some debug
>>>> messages to identify the cause for this.
>>>>
>>>
>>> I really don't like this. your default msg lvl has NETIF_MSG_IFDOWN
>>> turned on .. dumping every single operation that happens on your
>>> device
>>> by default to kernel log is too much ! 
>>>
>>> We should really consider using trace buffers with well defined
>>> structures for vendor specific events. so we can use bpf filters
>>> and
>>> state of the art tools for netdev debugging.
>>>
>>
>> We do this because we can just see a link down message in dmesg, and
>> had
>> take a long time to found the cause of link down, just because
>> another
>> user changed the settings.
>>
>> We can change the net_open/net_stop/dcbnl_ops to msg_drv (not default
>> turned on),  and want to keep the others default print to kernel log,
>> is it acceptable?
>>
> 
> acceptable as long as debug information are kept off by default and
> your driver doens't spam the kernel log.
> 
> you should use dynamic debug [1] and/or "off by default" msg lvls for
> debugging information..
> 
> I couldn't find any rules regarding what to put in kernel log, Maybe
> someone can share ?. but i vaguely remember that the recommendation
> for device drivers is to put nothing, only error/warning messages.
> 
> [1] 
> https://www.kernel.org/doc/html/v4.15/admin-guide/dynamic-debug-howto.html
> 
>>>> @@ -1593,6 +1603,11 @@ static int hns3_ndo_set_vf_vlan(struct
>>>> net_device *netdev, int vf, u16 vlan,
>>>>  	struct hnae3_handle *h = hns3_get_handle(netdev);
>>>>  	int ret = -EIO;
>>>>  
>>>> +	if (netif_msg_ifdown(h))
>>>
>>> why msg_ifdown ? looks like netif_msg_drv is more appropriate, for
>>> many
>>> of the cases in this patch.
>>>
>>
>> This operation may cause link down, so we use msg_ifdown.
>>
> 
> ifdown isn't link down.. 
> 
> to be honest, I couldn't find any documentation explaining how/when to
> use msg lvls, (i didn't look too deep though), by looking at other
> drivers, my interpretations is:
> 
> ifdup (open/boot up flow)
> ifdwon (close/teardown flow)
> drv (driver based or dynamic flows) 
> etc .. 
> 
> -Saeed.
> 

Re: [PATCH net-next 07/11] net: hns3: adds debug messages to identify eth down cause

[liuyonglong]

As Saeed said, we will use netif_msg_drv() which is default off, this
can be easily open with ethtool.
Thanks for your reply!

On 2019/7/26 9:28, Jakub Kicinski wrote:
> On Thu, 25 Jul 2019 21:59:08 +0000, Saeed Mahameed wrote:
>> I couldn't find any rules regarding what to put in kernel log, Maybe
>> someone can share ?. but i vaguely remember that the recommendation
>> for device drivers is to put nothing, only error/warning messages.
> 
> FWIW my understanding is also that only error/warning messages should
> be printed. IOW things which should "never happen".
> 
> There are some historical exceptions. Probe logs for instance may be
> useful, because its not trivial to get to the device if probe fails.
> 
> Another one is ethtool flashing, if it takes time we used to print into
> logs some message like "please wait patiently". But since Jiri added
> the progress messages in devlink that's no longer necessary.
> 
> For the messages which are basically printing the name of the function
> or name of the function and their args - we have ftrace.
> 
> That's my $0.02 :)
> 
> .
> 

Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Saeed Mahameed]

On Thu, 2019-07-25 at 10:05 +0800, Yunsheng Lin wrote:
> On 2019/7/25 3:24, Saeed Mahameed wrote:
> > On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
> > > From: Yunsheng Lin <linyunsheng@huawei.com>
> > > 

[...]
> > > 
> > > +static void hclge_irq_affinity_notify(struct irq_affinity_notify
> > > *notify,
> > > +				      const cpumask_t *mask)
> > > +{
> > > +	struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
> > > +					      affinity_notify);
> > > +
> > > +	cpumask_copy(&hdev->affinity_mask, mask);
> > > +	del_timer_sync(&hdev->service_timer);
> > > +	hdev->service_timer.expires = jiffies + HZ;
> > > +	add_timer_on(&hdev->service_timer, cpumask_first(&hdev-
> > > > affinity_mask));
> > > +}
> > 
> > I don't see any relation between your misc irq vector and &hdev-
> > > service_timer, to me this looks like an abuse of the irq affinity
> > > API
> > to allow the user to move the service timer affinity.
> 
> Hi, thanks for reviewing.
> 
> hdev->service_timer is used to schedule the periodic work
> queue hdev->service_task， we want all the management work
> queue including hdev->service_task to bind to the same cpu
> to improve cache and power efficiency, it is better to move
> service timer affinity according to that.
> 
> The hdev->service_task is changed to delay work queue in
> next patch " net: hns3: make hclge_service use delayed workqueue",
> So the affinity in the timer of the delay work queue is automatically
> set to the affinity of the delay work queue, we will move the
> "make hclge_service use delayed workqueue" patch before the
> "add interrupt affinity support for misc interrupt" patch, so
> we do not have to set service timer affinity explicitly.
> 
> Also, There is there work queues(mbx_service_task, service_task,
> rst_service_task) in the hns3 driver, we plan to combine them
> to one or two workqueue to improve efficiency and readability.
> 

So just to make it clear, you have 3 deferred works, 2 are triggered by
the misc irq and 1 is periodic, you want them all on the same core and
you want to control their affinity via the irq affinity ? for works #1
and  #2 you get that for free since the irq is triggering them, but for
work #3 (the periodic one) you need to manually move it when the irq
affinity changes.

I guess i am ok with this, since moving the irq affinity isn't only
required by the periodic work but also for the other works that the irq
is actually triggering (so it is not an actual abuse, sort of .. )

RE: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Salil Mehta]

> From: tanhuazhong
> Sent: Wednesday, July 24, 2019 4:19 AM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Salil Mehta
> <salil.mehta@huawei.com>; Zhuangyuzeng (Yisen) <yisen.zhuang@huawei.com>;
> Linuxarm <linuxarm@huawei.com>; linyunsheng <linyunsheng@huawei.com>; lipeng
> (Y) <lipeng321@huawei.com>; tanhuazhong <tanhuazhong@huawei.com>
> Subject: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for
> misc interrupt
> 
> From: Yunsheng Lin <linyunsheng@huawei.com>
> 
> The misc interrupt is used to schedule the reset and mailbox
> subtask, and a 1 sec timer is used to schedule the service
> subtask, which does periodic work.
> 
> This patch sets the above three subtask's affinity using the
> misc interrupt' affinity.
> 
> Also this patch setups a affinity notify for misc interrupt to
> allow user to change the above three subtask's affinity.
> 
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> Signed-off-by: Peng Li <lipeng321@huawei.com>
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> ---
>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 59
> ++++++++++++++++++++--
>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  4 ++
>  2 files changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index f345095..fe45986 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -1270,6 +1270,12 @@ static int hclge_configure(struct hclge_dev *hdev)
> 
>  	hclge_init_kdump_kernel_config(hdev);
> 
> +	/* Set the init affinity based on pci func number */
> +	i = cpumask_weight(cpumask_of_node(dev_to_node(&hdev->pdev->dev)));
> +	i = i ? PCI_FUNC(hdev->pdev->devfn) % i : 0;
> +	cpumask_set_cpu(cpumask_local_spread(i, dev_to_node(&hdev->pdev->dev)),
> +			&hdev->affinity_mask);
> +
>  	return ret;
>  }
> 
> @@ -2502,14 +2508,16 @@ static void hclge_mbx_task_schedule(struct hclge_dev
> *hdev)
>  {
>  	if (!test_bit(HCLGE_STATE_CMD_DISABLE, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_MBX_SERVICE_SCHED, &hdev->state))
> -		schedule_work(&hdev->mbx_service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,


Why we have to use system Work Queue here? This could adversely affect
other work scheduled not related to the HNS3 driver. Mailbox is internal
to the driver and depending upon utilization of the mbx channel(which could
be heavy if many VMs are running), this might stress other system tasks
as well. Have we thought of this?



> +			      &hdev->mbx_service_task);
>  }
> 
>  static void hclge_reset_task_schedule(struct hclge_dev *hdev)
>  {
>  	if (!test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state))
> -		schedule_work(&hdev->rst_service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
> +			      &hdev->rst_service_task);
>  }
> 
>  static void hclge_task_schedule(struct hclge_dev *hdev)
> @@ -2517,7 +2525,8 @@ static void hclge_task_schedule(struct hclge_dev *hdev)
>  	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
>  	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>  	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
> -		(void)schedule_work(&hdev->service_task);
> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,

Same here.


Salil.

Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Yunsheng Lin]

On 2019/7/27 7:18, Saeed Mahameed wrote:
> On Thu, 2019-07-25 at 10:05 +0800, Yunsheng Lin wrote:
>> On 2019/7/25 3:24, Saeed Mahameed wrote:
>>> On Wed, 2019-07-24 at 11:18 +0800, Huazhong Tan wrote:
>>>> From: Yunsheng Lin <linyunsheng@huawei.com>
>>>>
> 
> [...]
>>>>
>>>> +static void hclge_irq_affinity_notify(struct irq_affinity_notify
>>>> *notify,
>>>> +				      const cpumask_t *mask)
>>>> +{
>>>> +	struct hclge_dev *hdev = container_of(notify, struct hclge_dev,
>>>> +					      affinity_notify);
>>>> +
>>>> +	cpumask_copy(&hdev->affinity_mask, mask);
>>>> +	del_timer_sync(&hdev->service_timer);
>>>> +	hdev->service_timer.expires = jiffies + HZ;
>>>> +	add_timer_on(&hdev->service_timer, cpumask_first(&hdev-
>>>>> affinity_mask));
>>>> +}
>>>
>>> I don't see any relation between your misc irq vector and &hdev-
>>>> service_timer, to me this looks like an abuse of the irq affinity
>>>> API
>>> to allow the user to move the service timer affinity.
>>
>> Hi, thanks for reviewing.
>>
>> hdev->service_timer is used to schedule the periodic work
>> queue hdev->service_task， we want all the management work
>> queue including hdev->service_task to bind to the same cpu
>> to improve cache and power efficiency, it is better to move
>> service timer affinity according to that.
>>
>> The hdev->service_task is changed to delay work queue in
>> next patch " net: hns3: make hclge_service use delayed workqueue",
>> So the affinity in the timer of the delay work queue is automatically
>> set to the affinity of the delay work queue, we will move the
>> "make hclge_service use delayed workqueue" patch before the
>> "add interrupt affinity support for misc interrupt" patch, so
>> we do not have to set service timer affinity explicitly.
>>
>> Also, There is there work queues(mbx_service_task, service_task,
>> rst_service_task) in the hns3 driver, we plan to combine them
>> to one or two workqueue to improve efficiency and readability.
>>
> 
> So just to make it clear, you have 3 deferred works, 2 are triggered by
> the misc irq and 1 is periodic, you want them all on the same core and
> you want to control their affinity via the irq affinity ? for works #1
> and  #2 you get that for free since the irq is triggering them, but for
> work #3 (the periodic one) you need to manually move it when the irq
> affinity changes.

Yes, You are right.

> 
> I guess i am ok with this, since moving the irq affinity isn't only
> required by the periodic work but also for the other works that the irq
> is actually triggering (so it is not an actual abuse, sort of .. )
> 
> 
> 

Re: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for misc interrupt

[Yunsheng Lin]

On 2019/7/29 17:18, Salil Mehta wrote:
>> From: tanhuazhong
>> Sent: Wednesday, July 24, 2019 4:19 AM
>> To: davem@davemloft.net
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Salil Mehta
>> <salil.mehta@huawei.com>; Zhuangyuzeng (Yisen) <yisen.zhuang@huawei.com>;
>> Linuxarm <linuxarm@huawei.com>; linyunsheng <linyunsheng@huawei.com>; lipeng
>> (Y) <lipeng321@huawei.com>; tanhuazhong <tanhuazhong@huawei.com>
>> Subject: [PATCH net-next 08/11] net: hns3: add interrupt affinity support for
>> misc interrupt
>>
>> From: Yunsheng Lin <linyunsheng@huawei.com>
>>
>> The misc interrupt is used to schedule the reset and mailbox
>> subtask, and a 1 sec timer is used to schedule the service
>> subtask, which does periodic work.
>>
>> This patch sets the above three subtask's affinity using the
>> misc interrupt' affinity.
>>
>> Also this patch setups a affinity notify for misc interrupt to
>> allow user to change the above three subtask's affinity.
>>
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> Signed-off-by: Peng Li <lipeng321@huawei.com>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
>> ---
>>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 59
>> ++++++++++++++++++++--
>>  .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h    |  4 ++
>>  2 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> index f345095..fe45986 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> @@ -1270,6 +1270,12 @@ static int hclge_configure(struct hclge_dev *hdev)
>>
>>  	hclge_init_kdump_kernel_config(hdev);
>>
>> +	/* Set the init affinity based on pci func number */
>> +	i = cpumask_weight(cpumask_of_node(dev_to_node(&hdev->pdev->dev)));
>> +	i = i ? PCI_FUNC(hdev->pdev->devfn) % i : 0;
>> +	cpumask_set_cpu(cpumask_local_spread(i, dev_to_node(&hdev->pdev->dev)),
>> +			&hdev->affinity_mask);
>> +
>>  	return ret;
>>  }
>>
>> @@ -2502,14 +2508,16 @@ static void hclge_mbx_task_schedule(struct hclge_dev
>> *hdev)
>>  {
>>  	if (!test_bit(HCLGE_STATE_CMD_DISABLE, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_MBX_SERVICE_SCHED, &hdev->state))
>> -		schedule_work(&hdev->mbx_service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
> 
> 
> Why we have to use system Work Queue here? This could adversely affect
> other work scheduled not related to the HNS3 driver. Mailbox is internal
> to the driver and depending upon utilization of the mbx channel(which could
> be heavy if many VMs are running), this might stress other system tasks
> as well. Have we thought of this?

If I understand the CMWQ correctly, it is better to reuse the system Work
Queue when the work queue needed by HNS3 driver shares the same property of
system Work Queue, because Concurrency Managed Workqueue mechnism has ensured
they have the same worker pool if they share the same property.

Some driver(i40e) allocates a work queue with WQ_MEM_RECLAIM, I am not sure
what is the usecase which requires the WQ_MEM_RECLAIM.

Anyway, If the HNS3 need to allocate a new work queue, we need to
figure out the usecase first.

> 
> 
> 
>> +			      &hdev->mbx_service_task);
>>  }
>>
>>  static void hclge_reset_task_schedule(struct hclge_dev *hdev)
>>  {
>>  	if (!test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_RST_SERVICE_SCHED, &hdev->state))
>> -		schedule_work(&hdev->rst_service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
>> +			      &hdev->rst_service_task);
>>  }
>>
>>  static void hclge_task_schedule(struct hclge_dev *hdev)
>> @@ -2517,7 +2525,8 @@ static void hclge_task_schedule(struct hclge_dev *hdev)
>>  	if (!test_bit(HCLGE_STATE_DOWN, &hdev->state) &&
>>  	    !test_bit(HCLGE_STATE_REMOVING, &hdev->state) &&
>>  	    !test_and_set_bit(HCLGE_STATE_SERVICE_SCHED, &hdev->state))
>> -		(void)schedule_work(&hdev->service_task);
>> +		queue_work_on(cpumask_first(&hdev->affinity_mask), system_wq,
> 
> Same here.
> 
> 
> Salil.
> 
> .
>