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

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

[Jijie Shao]

There are some bugfix for the HNS3 ethernet driver

Jian Shen (1):
  net: hns3: restore user pause configure when disable autoneg

Jie Wang (2):
  net: hns3: refactor hclge_mac_link_status_wait for interface reuse
  net: hns3: add wait until mac link down

Peiyang Wang (1):
  net: hns3: fix wrong print link down up

Yonglong Liu (2):
  net: hns3: fix side effects passed to min_t()
  net: hns3: fix deadlock issue when externel_lb and reset are executed
    together

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   | 17 ++++++++--
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 32 ++++++++++++++-----
 .../ethernet/hisilicon/hns3/hns3pf/hclge_tm.c |  2 +-
 .../ethernet/hisilicon/hns3/hns3pf/hclge_tm.h |  1 +
 4 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.30.0

[PATCH net 1/6] net: hns3: fix side effects passed to min_t()

[Jijie Shao]

From: Yonglong Liu <liuyonglong@huawei.com>

num_online_cpus() may call more than once when passing to min_t(),
between calls, it may return different values, so move num_online_cpus()
out of min_t().

Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 9f6890059666..823e6d2e85f5 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 {
 	struct hnae3_handle *h = priv->ae_handle;
 	struct hns3_enet_tqp_vector *tqp_vector;
+	u32 online_cpus = num_online_cpus();
 	struct hnae3_vector_info *vector;
 	struct pci_dev *pdev = h->pdev;
 	u16 tqp_num = h->kinfo.num_tqps;
@@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
 
 	/* RSS size, cpu online and vector_num should be the same */
 	/* Should consider 2p/4p later */
-	vector_num = min_t(u16, num_online_cpus(), tqp_num);
+	vector_num = min_t(u16, online_cpus, tqp_num);
 
 	vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector),
 			      GFP_KERNEL);
-- 
2.30.0

[PATCH net 2/6] net: hns3: restore user pause configure when disable autoneg

[Jijie Shao]

From: Jian Shen <shenjian15@huawei.com>

Restore the mac pause state to user configuration when autoneg is disabled

Signed-off-by: Jian Shen <shenjian15@huawei.com>
Signed-off-by: Peiyang Wang <wangpeiyang1@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 5 ++++-
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c   | 2 +-
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h   | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index bf675c15fbb9..5594b8dd1e1d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -10915,9 +10915,12 @@ int hclge_cfg_flowctrl(struct hclge_dev *hdev)
 	u32 rx_pause, tx_pause;
 	u8 flowctl;
 
-	if (!phydev->link || !phydev->autoneg)
+	if (!phydev->link)
 		return 0;
 
+	if (!phydev->autoneg)
+		return hclge_mac_pause_setup_hw(hdev);
+
 	local_advertising = linkmode_adv_to_lcl_adv_t(phydev->advertising);
 
 	if (phydev->pause)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
index de509e5751a7..c58c31221762 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.c
@@ -1553,7 +1553,7 @@ static int hclge_bp_setup_hw(struct hclge_dev *hdev, u8 tc)
 	return 0;
 }
 
-static int hclge_mac_pause_setup_hw(struct hclge_dev *hdev)
+int hclge_mac_pause_setup_hw(struct hclge_dev *hdev)
 {
 	bool tx_en, rx_en;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
index 45dcfef3f90c..53eec6df5194 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_tm.h
@@ -245,6 +245,7 @@ int hclge_pfc_pause_en_cfg(struct hclge_dev *hdev, u8 tx_rx_bitmap,
 			   u8 pfc_bitmap);
 int hclge_mac_pause_en_cfg(struct hclge_dev *hdev, bool tx, bool rx);
 int hclge_pause_addr_cfg(struct hclge_dev *hdev, const u8 *mac_addr);
+int hclge_mac_pause_setup_hw(struct hclge_dev *hdev);
 void hclge_pfc_rx_stats_get(struct hclge_dev *hdev, u64 *stats);
 void hclge_pfc_tx_stats_get(struct hclge_dev *hdev, u64 *stats);
 int hclge_tm_qs_shaper_cfg(struct hclge_vport *vport, int max_tx_rate);
-- 
2.30.0

[PATCH net 3/6] net: hns3: refactor hclge_mac_link_status_wait for interface reuse

[Jijie Shao]

From: Jie Wang <wangjie125@huawei.com>

Some nic configurations could only be performed after link is down. So this
patch refactor this API for reuse.

Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 5594b8dd1e1d..b440e42e1d9c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -72,6 +72,8 @@ static void hclge_restore_hw_table(struct hclge_dev *hdev);
 static void hclge_sync_promisc_mode(struct hclge_dev *hdev);
 static void hclge_sync_fd_table(struct hclge_dev *hdev);
 static void hclge_update_fec_stats(struct hclge_dev *hdev);
+static int hclge_mac_link_status_wait(struct hclge_dev *hdev, int link_ret,
+				      int wait_cnt);
 
 static struct hnae3_ae_algo ae_algo;
 
@@ -7647,10 +7649,9 @@ static void hclge_phy_link_status_wait(struct hclge_dev *hdev,
 	} while (++i < HCLGE_PHY_LINK_STATUS_NUM);
 }
 
-static int hclge_mac_link_status_wait(struct hclge_dev *hdev, int link_ret)
+static int hclge_mac_link_status_wait(struct hclge_dev *hdev, int link_ret,
+				      int wait_cnt)
 {
-#define HCLGE_MAC_LINK_STATUS_NUM  100
-
 	int link_status;
 	int i = 0;
 	int ret;
@@ -7663,13 +7664,15 @@ static int hclge_mac_link_status_wait(struct hclge_dev *hdev, int link_ret)
 			return 0;
 
 		msleep(HCLGE_LINK_STATUS_MS);
-	} while (++i < HCLGE_MAC_LINK_STATUS_NUM);
+	} while (++i < wait_cnt);
 	return -EBUSY;
 }
 
 static int hclge_mac_phy_link_status_wait(struct hclge_dev *hdev, bool en,
 					  bool is_phy)
 {
+#define HCLGE_MAC_LINK_STATUS_NUM  100
+
 	int link_ret;
 
 	link_ret = en ? HCLGE_LINK_STATUS_UP : HCLGE_LINK_STATUS_DOWN;
@@ -7677,7 +7680,8 @@ static int hclge_mac_phy_link_status_wait(struct hclge_dev *hdev, bool en,
 	if (is_phy)
 		hclge_phy_link_status_wait(hdev, link_ret);
 
-	return hclge_mac_link_status_wait(hdev, link_ret);
+	return hclge_mac_link_status_wait(hdev, link_ret,
+					  HCLGE_MAC_LINK_STATUS_NUM);
 }
 
 static int hclge_set_app_loopback(struct hclge_dev *hdev, bool en)
-- 
2.30.0

[PATCH net 4/6] net: hns3: add wait until mac link down

[Jijie Shao]

From: Jie Wang <wangjie125@huawei.com>

In some configure flow of hns3 driver, for example, change mtu, it will
disable MAC through firmware before configuration. But firmware disables
MAC asynchronously. The rx traffic may be not stopped in this case.

So fixes it by waiting until mac link is down.

Fixes: a9775bb64aa7 ("net: hns3: fix set and get link ksettings issue")
Signed-off-by: Jie Wang <wangjie125@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c    | 10 +++++++++-
 1 file changed, 9 insertions(+), 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 b440e42e1d9c..a940e35aef29 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -7560,6 +7560,8 @@ static void hclge_enable_fd(struct hnae3_handle *handle, bool enable)
 
 static void hclge_cfg_mac_mode(struct hclge_dev *hdev, bool enable)
 {
+#define HCLGE_LINK_STATUS_WAIT_CNT  3
+
 	struct hclge_desc desc;
 	struct hclge_config_mac_mode_cmd *req =
 		(struct hclge_config_mac_mode_cmd *)desc.data;
@@ -7584,9 +7586,15 @@ static void hclge_cfg_mac_mode(struct hclge_dev *hdev, bool enable)
 	req->txrx_pad_fcs_loop_en = cpu_to_le32(loop_en);
 
 	ret = hclge_cmd_send(&hdev->hw, &desc, 1);
-	if (ret)
+	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"mac enable fail, ret =%d.\n", ret);
+		return;
+	}
+
+	if (!enable)
+		hclge_mac_link_status_wait(hdev, HCLGE_LINK_STATUS_DOWN,
+					   HCLGE_LINK_STATUS_WAIT_CNT);
 }
 
 static int hclge_config_switch_param(struct hclge_dev *hdev, int vfid,
-- 
2.30.0

[PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

From: Peiyang Wang <wangpeiyang1@huawei.com>

This patch will fix a wrong print "device link down/up". Consider a case
that set autoneg to off with same speed and duplex configuration. The link
is always up while the phy state is set to PHY_UP and set back to
PHY_RUNNING later. It will print link down when the phy state is not
PHY_RUNNING. To avoid that, the condition should include PHY_UP.

Signed-off-by: Peiyang Wang <wangpeiyang1@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 3 ++-
 1 file changed, 2 insertions(+), 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 a940e35aef29..1723d0fa49ee 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -3089,7 +3089,8 @@ static int hclge_get_mac_phy_link(struct hclge_dev *hdev, int *link_status)
 	if (test_bit(HCLGE_STATE_DOWN, &hdev->state))
 		return 0;
 
-	if (phydev && (phydev->state != PHY_RUNNING || !phydev->link))
+	if (phydev && ((phydev->state != PHY_UP &&
+			phydev->state != PHY_RUNNING) || !phydev->link))
 		return 0;
 
 	return hclge_get_mac_link_status(hdev, link_status);
-- 
2.30.0

[PATCH net 6/6] net: hns3: fix deadlock issue when externel_lb and reset are executed together

[Jijie Shao]

From: Yonglong Liu <liuyonglong@huawei.com>

When externel_lb and reset are executed together, a deadlock may
occur:
[ 3147.217009] INFO: task kworker/u321:0:7 blocked for more than 120 seconds.
[ 3147.230483] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 3147.238999] task:kworker/u321:0  state:D stack:    0 pid:    7 ppid:     2 flags:0x00000008
[ 3147.248045] Workqueue: hclge hclge_service_task [hclge]
[ 3147.253957] Call trace:
[ 3147.257093]  __switch_to+0x7c/0xbc
[ 3147.261183]  __schedule+0x338/0x6f0
[ 3147.265357]  schedule+0x50/0xe0
[ 3147.269185]  schedule_preempt_disabled+0x18/0x24
[ 3147.274488]  __mutex_lock.constprop.0+0x1d4/0x5dc
[ 3147.279880]  __mutex_lock_slowpath+0x1c/0x30
[ 3147.284839]  mutex_lock+0x50/0x60
[ 3147.288841]  rtnl_lock+0x20/0x2c
[ 3147.292759]  hclge_reset_prepare+0x68/0x90 [hclge]
[ 3147.298239]  hclge_reset_subtask+0x88/0xe0 [hclge]
[ 3147.303718]  hclge_reset_service_task+0x84/0x120 [hclge]
[ 3147.309718]  hclge_service_task+0x2c/0x70 [hclge]
[ 3147.315109]  process_one_work+0x1d0/0x490
[ 3147.319805]  worker_thread+0x158/0x3d0
[ 3147.324240]  kthread+0x108/0x13c
[ 3147.328154]  ret_from_fork+0x10/0x18

In externel_lb process, the hns3 driver call napi_disable()
first, then the reset happen, then the restore process of the
externel_lb will fail, and will not call napi_enable(). When
doing externel_lb again, napi_disable() will be double call,
cause a deadlock of rtnl_lock().

This patch use the HNS3_NIC_STATE_DOWN state to protect the
calling of napi_disable() and napi_enable() in externel_lb
process, just as the usage in ndo_stop() and ndo_start().

Fixes: 04b6ba143521 ("net: hns3: add support for external loopback test")
Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 823e6d2e85f5..7da54a5b81d1 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -5855,6 +5855,9 @@ void hns3_external_lb_prepare(struct net_device *ndev, bool if_running)
 	if (!if_running)
 		return;
 
+	if (test_and_set_bit(HNS3_NIC_STATE_DOWN, &priv->state))
+		return;
+
 	netif_carrier_off(ndev);
 	netif_tx_disable(ndev);
 
@@ -5883,7 +5886,16 @@ void hns3_external_lb_restore(struct net_device *ndev, bool if_running)
 	if (!if_running)
 		return;
 
-	hns3_nic_reset_all_ring(priv->ae_handle);
+	if (hns3_nic_resetting(ndev))
+		return;
+
+	if (!test_bit(HNS3_NIC_STATE_DOWN, &priv->state))
+		return;
+
+	if (hns3_nic_reset_all_ring(priv->ae_handle))
+		return;
+
+	clear_bit(HNS3_NIC_STATE_DOWN, &priv->state);
 
 	for (i = 0; i < priv->vector_num; i++)
 		hns3_vector_enable(&priv->tqp_vector[i]);
-- 
2.30.0

RE: [PATCH net 1/6] net: hns3: fix side effects passed to min_t()

[David Laight]

From: Jijie Shao
> Sent: 28 July 2023 08:59
> 
> num_online_cpus() may call more than once when passing to min_t(),
> between calls, it may return different values, so move num_online_cpus()
> out of min_t().

Nope, wrong bug:
min() (and friends) are careful to only evaluate their arguments once.
The bug is using min_t() - especially with a small type.

If/when the number of cpu hits 65536 the (u16) cast will convert
it to zero.

Looking at the code a lot of the local variables should be
'unsigned int' not 'u16.
Just because the domain of a value is small doesn't mean
you should use a small type (unless you are saving space in
a structure).

	David

> 
> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ---
>  drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 9f6890059666..823e6d2e85f5 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>  {
>  	struct hnae3_handle *h = priv->ae_handle;
>  	struct hns3_enet_tqp_vector *tqp_vector;
> +	u32 online_cpus = num_online_cpus();
>  	struct hnae3_vector_info *vector;
>  	struct pci_dev *pdev = h->pdev;
>  	u16 tqp_num = h->kinfo.num_tqps;
> @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
> 
>  	/* RSS size, cpu online and vector_num should be the same */
>  	/* Should consider 2p/4p later */
> -	vector_num = min_t(u16, num_online_cpus(), tqp_num);
> +	vector_num = min_t(u16, online_cpus, tqp_num);
> 
>  	vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector),
>  			      GFP_KERNEL);
> --
> 2.30.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Andrew Lunn]

On Fri, Jul 28, 2023 at 03:58:39PM +0800, Jijie Shao wrote:
> From: Peiyang Wang <wangpeiyang1@huawei.com>
> 
> This patch will fix a wrong print "device link down/up". Consider a case
> that set autoneg to off with same speed and duplex configuration. The link
> is always up while the phy state is set to PHY_UP and set back to
> PHY_RUNNING later. It will print link down when the phy state is not
> PHY_RUNNING. To avoid that, the condition should include PHY_UP.

Does this really happen? If autoneg is on, and there is link, it means
the link peer is auto using auto-neg. If you turn auto-neg off, the
link peer is not going to know what speed to use, and so the link will
go down. The link will only come up again when you reconfigure the
link peer to also not use auto-neg.

I don't see how you can turn auto-neg off and not loose the link.

  Andrew

Re: [PATCH net 1/6] net: hns3: fix side effects passed to min_t()

[Jijie Shao]

Hi David：

Yes, you're right, min_t() evaluates the arguments only once.

In the actual scenario, the number of cpu is far less than 65535. 
Therefore, the minimum value will not convert to zero.

Thanks for your advice, this patch will be withdrawn.

   Jijie Shao

on 2023/7/28 16:29, David Laight wrote:
> From: Jijie Shao
>> Sent: 28 July 2023 08:59
>>
>> num_online_cpus() may call more than once when passing to min_t(),
>> between calls, it may return different values, so move num_online_cpus()
>> out of min_t().
> Nope, wrong bug:
> min() (and friends) are careful to only evaluate their arguments once.
> The bug is using min_t() - especially with a small type.
>
> If/when the number of cpu hits 65536 the (u16) cast will convert
> it to zero.
>
> Looking at the code a lot of the local variables should be
> 'unsigned int' not 'u16.
> Just because the domain of a value is small doesn't mean
> you should use a small type (unless you are saving space in
> a structure).
>
> 	David
>
>> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
>> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
>> ---
>>   drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 9f6890059666..823e6d2e85f5 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>>   {
>>   	struct hnae3_handle *h = priv->ae_handle;
>>   	struct hns3_enet_tqp_vector *tqp_vector;
>> +	u32 online_cpus = num_online_cpus();
>>   	struct hnae3_vector_info *vector;
>>   	struct pci_dev *pdev = h->pdev;
>>   	u16 tqp_num = h->kinfo.num_tqps;
>> @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>>
>>   	/* RSS size, cpu online and vector_num should be the same */
>>   	/* Should consider 2p/4p later */
>> -	vector_num = min_t(u16, num_online_cpus(), tqp_num);
>> +	vector_num = min_t(u16, online_cpus, tqp_num);
>>
>>   	vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector),
>>   			      GFP_KERNEL);
>> --
>> 2.30.0
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

Hi Andrew,
I understand what you mean, and sorry for my wrong description. The link
is not always up. If I turn auto-neg off, the link will go down finally.
However, there is an intervel between my operation and the link down. In
my experiment, it may be 1 min or evn 10 mins. The phy state is set to
PHY_UP immediately when I set auto-neg off. And the phy machine check the
state during a very small intervals. Thus, during my experiment, the phy
state has a followed varietion:
PHY_RUNNING -> PHY_UP -> PHY_RUNNING -> PHY_NOLINK.

We print link up/down based on phy state and link state. In aboved case,
It print looks like:
eth0 link down -- because phy state is set to PHY_UP
eth0 link up -- because phy state is set to PHY_RUNNING
eth0 link down -- because link down

This patch wants to fix the first two wrong print.
We will modify this patch description

Thanks!

   Jijie Shao


on 2023/7/28 16:57, Andrew Lunn wrote:
> On Fri, Jul 28, 2023 at 03:58:39PM +0800, Jijie Shao wrote:
>> From: Peiyang Wang <wangpeiyang1@huawei.com>
>>
>> This patch will fix a wrong print "device link down/up". Consider a case
>> that set autoneg to off with same speed and duplex configuration. The link
>> is always up while the phy state is set to PHY_UP and set back to
>> PHY_RUNNING later. It will print link down when the phy state is not
>> PHY_RUNNING. To avoid that, the condition should include PHY_UP.
> Does this really happen? If autoneg is on, and there is link, it means
> the link peer is auto using auto-neg. If you turn auto-neg off, the
> link peer is not going to know what speed to use, and so the link will
> go down. The link will only come up again when you reconfigure the
> link peer to also not use auto-neg.
>
> I don't see how you can turn auto-neg off and not loose the link.
>
>    Andrew

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Andrew Lunn]

On Sat, Jul 29, 2023 at 11:11:48AM +0800, Jijie Shao wrote:
> Hi Andrew,
> I understand what you mean, and sorry for my wrong description. The link
> is not always up. If I turn auto-neg off, the link will go down finally.
> However, there is an intervel between my operation and the link down. In
> my experiment, it may be 1 min or evn 10 mins. The phy state is set to
> PHY_UP immediately when I set auto-neg off. And the phy machine check the
> state during a very small intervals. Thus, during my experiment, the phy
> state has a followed varietion:
> PHY_RUNNING -> PHY_UP -> PHY_RUNNING -> PHY_NOLINK.
> 
> We print link up/down based on phy state and link state. In aboved case,
> It print looks like:
> eth0 link down -- because phy state is set to PHY_UP
> eth0 link up -- because phy state is set to PHY_RUNNING
> eth0 link down -- because link down
> 
> This patch wants to fix the first two wrong print.
> We will modify this patch description

Now i wounder if you are fixing the wrong thing. Maybe you should be
fixing the PHY so it does not report up and then down? You say 'very
snall intervals', which should in fact be 1 second. So is the PHY
reporting link for a number of poll intervals? 1min to 10 minutes?

	  Andrew

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Andrew Lunn]

>     Now i wounder if you are fixing the wrong thing. Maybe you should be
>     fixing the PHY so it does not report up and then down? You say 'very
>     snall intervals', which should in fact be 1 second. So is the PHY
>     reporting link for a number of poll intervals? 1min to 10 minutes?
> 
>               Andrew
> 
> Yes, according to the log records, the phy polls every second,
> but the link status changes take time.
> Generally, it takes 10 seconds for the phy to detect link down,
> but occasionally it takes several minutes to detect link down,

What PHY driver is this?

It is not so clear what should actually happen with auto-neg turned
off. With it on, and the link going down, the PHY should react after
about 1 second. It is not supposed to react faster than that, although
some PHYs allow fast link down notification to be configured.

Have you checked 802.3 to see what it says about auto-neg off and link
down detection?

I personally would not suppress this behaviour in the MAC
driver. Otherwise you are going to have funny combinations of special
cases of a feature which very few people actually use, making your
maintenance costs higher.

	    Andrew

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

on 2023/7/30 2:23, Andrew Lunn wrote:
>>      Now i wounder if you are fixing the wrong thing. Maybe you should be
>>      fixing the PHY so it does not report up and then down? You say 'very
>>      snall intervals', which should in fact be 1 second. So is the PHY
>>      reporting link for a number of poll intervals? 1min to 10 minutes?
>>
>>                Andrew
>>
>> Yes, according to the log records, the phy polls every second,
>> but the link status changes take time.
>> Generally, it takes 10 seconds for the phy to detect link down,
>> but occasionally it takes several minutes to detect link down,
> What PHY driver is this?
>
> It is not so clear what should actually happen with auto-neg turned
> off. With it on, and the link going down, the PHY should react after
> about 1 second. It is not supposed to react faster than that, although
> some PHYs allow fast link down notification to be configured.
>
> Have you checked 802.3 to see what it says about auto-neg off and link
> down detection?
>
> I personally would not suppress this behaviour in the MAC
> driver. Otherwise you are going to have funny combinations of special
> cases of a feature which very few people actually use, making your
> maintenance costs higher.
>
> 	    Andrew
Thanks for your suggestion, We are analyzing this issue in depth.

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

on 2023/7/31 17:10, Jijie Shao wrote:
> What PHY driver is this?
>>
>> It is not so clear what should actually happen with auto-neg turned
>> off. With it on, and the link going down, the PHY should react after
>> about 1 second. It is not supposed to react faster than that, although
>> some PHYs allow fast link down notification to be configured.
>>
>> Have you checked 802.3 to see what it says about auto-neg off and link
>> down detection?
>>
>> I personally would not suppress this behaviour in the MAC
>> driver. Otherwise you are going to have funny combinations of special
>> cases of a feature which very few people actually use, making your
>> maintenance costs higher.
>>
>>         Andrew

Hi Andrew,
We trace how the PHY state machine changed and show as followed:

[ 1974.220847][ T362] hns3 0000:35:00.0 eth1: set link(phy): autoneg=0, 
speed=100, duplex=1
[ 1974.233694][ T362] hns3 0000:35:00.0 eth1: link down
[ 1974.267444][ T32] RTL8211F Gigabit Ethernet mii-0000:35:00.0:02: PHY 
state change UP -> RUNNING
[ 1974.892830][ T7] hns3 0000:35:00.0 eth1: link up
[ 2004.277425][ T32] RTL8211F Gigabit Ethernet mii-0000:35:00.0:02: PHY 
state change RUNNING -> NOLINK
[ 2004.797731][ T7] hns3 0000:35:00.0 eth1: link down

Meanwhile, we also open tracing event about mdio and here are some 
useful logs:

kworker/1:0-19 [001] .... 1973.329775: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x00 val:0x1040
kworker/1:0-19 [001] .... 1973.331964: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x01 val:0x79ad
kworker/2:1-32 [002] .... 1974.247627: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x00 val:0x1040
kworker/2:1-32 [002] .... 1974.249870: mdio_access: mii-0000:35:00.0 
write phy:0x02 reg:0x00 val:0x2100
kworker/2:1-32 [002] .... 1974.252069: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x00 val:0x2100
kworker/2:1-32 [002] .... 1974.254143: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x01 val:0x798d
....
kworker/2:1-32 [002] .... 2003.240015: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x01 val:0x798d
....
kworker/2:1-32 [002] .... 2004.269525: mdio_access: mii-0000:35:00.0 
read phy:0x02 reg:0x01 val:0x7989

As you can see, the link state changed after 30 seconds when only 
setting autoneg off. When the BMSR changed, the PHY driver change state 
immediately. This patch wants to fixed the first link down up showed on 
logs cause the link do not changed.


Regards
Jijie Shao

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

on 2023/7/31 17:10, Jijie Shao wrote:
>
> on 2023/7/30 2:23, Andrew Lunn wrote:
>>>      Now i wounder if you are fixing the wrong thing. Maybe you 
>>> should be
>>>      fixing the PHY so it does not report up and then down? You say 
>>> 'very
>>>      snall intervals', which should in fact be 1 second. So is the PHY
>>>      reporting link for a number of poll intervals? 1min to 10 minutes?
>>>
>>>                Andrew
>>>
>>> Yes, according to the log records, the phy polls every second,
>>> but the link status changes take time.
>>> Generally, it takes 10 seconds for the phy to detect link down,
>>> but occasionally it takes several minutes to detect link down,
>> What PHY driver is this?
>>
>> It is not so clear what should actually happen with auto-neg turned
>> off. With it on, and the link going down, the PHY should react after
>> about 1 second. It is not supposed to react faster than that, although
>> some PHYs allow fast link down notification to be configured.
>>
>> Have you checked 802.3 to see what it says about auto-neg off and link
>> down detection?
>>
>> I personally would not suppress this behaviour in the MAC
>> driver. Otherwise you are going to have funny combinations of special
>> cases of a feature which very few people actually use, making your
>> maintenance costs higher.
>>
>>         Andrew

Hi Andrew,
We've rewritten the commit log to explain this problem,
Would you please take some time to review that?

The following is the new commit log:
This patch is to correct a wrong log info "link down/up" in hns3 driver.
When setting autoneg off without changing speed and duplex, the link
should be not changed. However in hns3 driver, it print link down/up once
incorrectly. We trace the phy machine state and find the phy change form
PHY_UP to PHY_RUNNING. No other state of PHY occurs during this process.
MDIO trace also indicate the link is on. The wrong log info and mdio
trace are showed as followed:

[  843.720783][  T367] hns3 0000:35:00.0 eth1: set link(phy): autoneg=0,
speed=10, duplex=1
[  843.736087][  T367] hns3 0000:35:00.0 eth1: link down
[  843.773506][   T17] RTL8211F Gigabit Ethernet mii-0000:35:00.0:02: PHY
state change UP -> RUNNING
[  844.674668][   T31] hns3 0000:35:00.0 eth1: link up

      kworker/1:1-32      [001] ....   841.457231: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x79ad
      kworker/1:1-32      [001] ....   842.486496: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x79ad
      kworker/1:1-32      [001] ....   843.520565: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x79ad
      kworker/0:1-17      [000] ....   843.757147: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x798d
      kworker/0:1-17      [000] ....   844.799141: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x798d
      kworker/0:1-17      [000] ....   845.831513: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x798d
      kworker/0:1-17      [000] ....   846.863053: mdio_access: mii-0000:
35:00.0 read  phy:0x02 reg:0x01 val:0x798d

Regards
Jijie

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Andrew Lunn]

On Tue, Oct 17, 2023 at 09:03:01PM +0800, Jijie Shao wrote:
> 
> on 2023/7/31 17:10, Jijie Shao wrote:
> > 
> > on 2023/7/30 2:23, Andrew Lunn wrote:
> > > >      Now i wounder if you are fixing the wrong thing. Maybe you
> > > > should be
> > > >      fixing the PHY so it does not report up and then down? You
> > > > say 'very
> > > >      snall intervals', which should in fact be 1 second. So is the PHY
> > > >      reporting link for a number of poll intervals? 1min to 10 minutes?
> > > > 
> > > >                Andrew
> > > > 
> > > > Yes, according to the log records, the phy polls every second,
> > > > but the link status changes take time.
> > > > Generally, it takes 10 seconds for the phy to detect link down,
> > > > but occasionally it takes several minutes to detect link down,
> > > What PHY driver is this?
> > > 
> > > It is not so clear what should actually happen with auto-neg turned
> > > off. With it on, and the link going down, the PHY should react after
> > > about 1 second. It is not supposed to react faster than that, although
> > > some PHYs allow fast link down notification to be configured.
> > > 
> > > Have you checked 802.3 to see what it says about auto-neg off and link
> > > down detection?
> > > 
> > > I personally would not suppress this behaviour in the MAC
> > > driver. Otherwise you are going to have funny combinations of special
> > > cases of a feature which very few people actually use, making your
> > > maintenance costs higher.
> > > 
> > >         Andrew
> 
> Hi Andrew,
> We've rewritten the commit log to explain this problem,
> Would you please take some time to review that?
> 
> The following is the new commit log:
> This patch is to correct a wrong log info "link down/up" in hns3 driver.
> When setting autoneg off without changing speed and duplex, the link
> should be not changed. However in hns3 driver, it print link down/up once
> incorrectly. We trace the phy machine state and find the phy change form
> PHY_UP to PHY_RUNNING. No other state of PHY occurs during this process.
> MDIO trace also indicate the link is on. The wrong log info and mdio
> trace are showed as followed:
> 
> [  843.720783][  T367] hns3 0000:35:00.0 eth1: set link(phy): autoneg=0,
> speed=10, duplex=1
> [  843.736087][  T367] hns3 0000:35:00.0 eth1: link down
> [  843.773506][   T17] RTL8211F Gigabit Ethernet mii-0000:35:00.0:02: PHY
> state change UP -> RUNNING
> [  844.674668][   T31] hns3 0000:35:00.0 eth1: link up

I still think this is totally valid and correct.

When you turn auto-neg off the link partner is going to react to that,
it might drop the link. After a while, the link partner will give up
trying to perform auto-neg and might fall back to 10/Half. At which
point, the link might allow traffic flow. However, in this example,
you have a duplex mis-match, so it might not work correctly.

Turning off auto-neg is something you need to do at both ends, and you
need to then force both ends to the same settings. Link down is
expected. I would actually be suppressed if no link down events were
reported.

	Andrew

Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

[Jijie Shao]

on 2023/10/17 21:59, Andrew Lunn wrote
> I still think this is totally valid and correct.
>
> When you turn auto-neg off the link partner is going to react to that,
> it might drop the link. After a while, the link partner will give up
> trying to perform auto-neg and might fall back to 10/Half. At which
> point, the link might allow traffic flow. However, in this example,
> you have a duplex mis-match, so it might not work correctly.
>
> Turning off auto-neg is something you need to do at both ends, and you
> need to then force both ends to the same settings. Link down is
> expected. I would actually be suppressed if no link down events were
> reported.
>
> 	Andrew

Hi Andrew,
Thank you for your comments, we are re-evaluating this issue and may drop this patch.

Regards
Jijie