There are some bugfix for the HNS3 ethernet driver
---
changeLog:
v1 -> v2:
- Adjust the code sequence to completely eliminate the race window, suggested by Jiri Pirko
v1: https://lore.kernel.org/all/[email protected]/
---
Jian Shen (1):
net: hns3: direct return when receive a unknown mailbox message
Peiyang Wang (4):
net: hns3: change type of numa_node_mask as nodemask_t
net: hns3: release PTP resources if pf initialization failed
net: hns3: use appropriate barrier function after setting a bit value
net: hns3: using user configure after hardware reset
Yonglong Liu (2):
net: hns3: fix port vlan filter not disabled issue
net: hns3: fix kernel crash when devlink reload during initialization
drivers/net/ethernet/hisilicon/hns3/hnae3.h | 2 +-
.../hisilicon/hns3/hns3pf/hclge_main.c | 52 +++++++++++--------
.../hisilicon/hns3/hns3pf/hclge_main.h | 5 +-
.../hisilicon/hns3/hns3pf/hclge_mbx.c | 7 +--
.../hisilicon/hns3/hns3vf/hclgevf_main.c | 20 ++++---
.../hisilicon/hns3/hns3vf/hclgevf_main.h | 2 +-
6 files changed, 49 insertions(+), 39 deletions(-)
--
2.30.0
From: Peiyang Wang <[email protected]>
When a reset occurring, it's supposed to recover user's configuration.
Currently, the port info(speed, duplex and autoneg) is stored in hclge_mac
and will be scheduled updated. Consider the case that reset was happened
consecutively. During the first reset, the port info is configured with
a temporary value cause the PHY is reset and looking for best link config.
Second reset start and use pervious configuration which is not the user's.
The specific process is as follows:
+------+ +----+ +----+
| USER | | PF | | HW |
+---+--+ +-+--+ +-+--+
| ethtool --reset | |
+------------------->| reset command |
| ethtool --reset +-------------------->|
+------------------->| +---+
| +---+ | |
| | |reset currently | | HW RESET
| | |and wait to do | |
| |<--+ | |
| | send pervious cfg |<--+
| | (1000M FULL AN_ON) |
| +-------------------->|
| | read cfg(time task) |
| | (10M HALF AN_OFF) +---+
| |<--------------------+ | cfg take effect
| | reset command |<--+
| +-------------------->|
| | +---+
| | send pervious cfg | | HW RESET
| | (10M HALF AN_OFF) |<--+
| +-------------------->|
| | read cfg(time task) |
| | (10M HALF AN_OFF) +---+
| |<--------------------+ | cfg take effect
| | | |
| | read cfg(time task) |<--+
| | (10M HALF AN_OFF) |
| |<--------------------+
| | |
v v v
To avoid aboved situation, this patch introduced req_speed, req_duplex,
req_autoneg to store user's configuration and it only be used after
hardware reset and to recover user's configuration
Fixes: f5f2b3e4dcc0 ("net: hns3: add support for imp-controlled PHYs")
Signed-off-by: Peiyang Wang <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
---
.../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 15 +++++++++------
.../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 3 +++
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 6eda73f1e6ad..5dc8593c97be 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev)
cfg.default_speed, ret);
return ret;
}
+ hdev->hw.mac.req_speed = hdev->hw.mac.speed;
+ hdev->hw.mac.req_autoneg = AUTONEG_ENABLE;
+ hdev->hw.mac.req_duplex = DUPLEX_FULL;
hclge_parse_link_mode(hdev, cfg.speed_ability);
@@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
return ret;
}
- hdev->hw.mac.autoneg = cmd->base.autoneg;
- hdev->hw.mac.speed = cmd->base.speed;
- hdev->hw.mac.duplex = cmd->base.duplex;
+ hdev->hw.mac.req_autoneg = cmd->base.autoneg;
+ hdev->hw.mac.req_speed = cmd->base.speed;
+ hdev->hw.mac.req_duplex = cmd->base.duplex;
linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising);
return 0;
@@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev)
if (ret)
return ret;
- hdev->hw.mac.autoneg = cmd.base.autoneg;
- hdev->hw.mac.speed = cmd.base.speed;
- hdev->hw.mac.duplex = cmd.base.duplex;
+ cmd.base.autoneg = hdev->hw.mac.req_autoneg;
+ cmd.base.speed = hdev->hw.mac.req_speed;
+ cmd.base.duplex = hdev->hw.mac.req_duplex;
linkmode_copy(hdev->hw.mac.advertising, cmd.link_modes.advertising);
return 0;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 37527b847f2f..3a9186457ad8 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -279,11 +279,14 @@ struct hclge_mac {
u8 media_type; /* port media type, e.g. fibre/copper/backplane */
u8 mac_addr[ETH_ALEN];
u8 autoneg;
+ u8 req_autoneg;
u8 duplex;
+ u8 req_duplex;
u8 support_autoneg;
u8 speed_type; /* 0: sfp speed, 1: active speed */
u8 lane_num;
u32 speed;
+ u32 req_speed;
u32 max_speed;
u32 speed_ability; /* speed ability supported by current media */
u32 module_type; /* sub media type, e.g. kr/cr/sr/lr */
--
2.30.0
From: Yonglong Liu <[email protected]>
According to hardware limitation, for device support modify
VLAN filter state but not support bypass port VLAN filter,
it should always disable the port VLAN filter. but the driver
enables port VLAN filter when initializing, if there is no
VLAN(except VLAN 0) id added, the driver will disable it
in service task. In most time, it works fine. But there is
a time window before the service task shceduled and net device
being registered. So if user adds VLAN at this time, the driver
will not update the VLAN filter state, and the port VLAN filter
remains enabled.
To fix the problem, if support modify VLAN filter state but not
support bypass port VLAN filter, set the port vlan filter to "off".
Fixes: 184cd221a863 ("net: hns3: disable port VLAN filter when support function level VLAN filter control")
Fixes: 2ba306627f59 ("net: hns3: add support for modify VLAN filter state")
Signed-off-by: Yonglong Liu <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 7 ++++++-
1 file changed, 6 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 5dc8593c97be..018069b12de6 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -9910,6 +9910,7 @@ static int hclge_set_vlan_protocol_type(struct hclge_dev *hdev)
static int hclge_init_vlan_filter(struct hclge_dev *hdev)
{
struct hclge_vport *vport;
+ bool enable = true;
int ret;
int i;
@@ -9929,8 +9930,12 @@ static int hclge_init_vlan_filter(struct hclge_dev *hdev)
vport->cur_vlan_fltr_en = true;
}
+ if (test_bit(HNAE3_DEV_SUPPORT_VLAN_FLTR_MDF_B, hdev->ae_dev->caps) &&
+ !test_bit(HNAE3_DEV_SUPPORT_PORT_VLAN_BYPASS_B, hdev->ae_dev->caps))
+ enable = false;
+
return hclge_set_vlan_filter_ctrl(hdev, HCLGE_FILTER_TYPE_PORT,
- HCLGE_FILTER_FE_INGRESS, true, 0);
+ HCLGE_FILTER_FE_INGRESS, enable, 0);
}
static int hclge_init_vlan_type(struct hclge_dev *hdev)
--
2.30.0
From: Peiyang Wang <[email protected]>
It provides nodemask_t to describe the numa node mask in kernel. To
improve transportability, change the type of numa_node_mask as nodemask_t.
Fixes: 38caee9d3ee8 ("net: hns3: Add support of the HNAE3 framework")
Signed-off-by: Peiyang Wang <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
---
drivers/net/ethernet/hisilicon/hns3/hnae3.h | 2 +-
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 6 ++++--
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 2 +-
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c | 7 ++++---
drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h | 2 +-
5 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index f19f1e1d1f9f..133c94646c21 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -897,7 +897,7 @@ struct hnae3_handle {
struct hnae3_roce_private_info rinfo;
};
- u32 numa_node_mask; /* for multi-chip support */
+ nodemask_t numa_node_mask; /* for multi-chip support */
enum hnae3_port_base_vlan_state port_base_vlan_state;
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index ff6a2ed23ddb..62ddce05fa2b 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1766,7 +1766,8 @@ static int hclge_vport_setup(struct hclge_vport *vport, u16 num_tqps)
nic->pdev = hdev->pdev;
nic->ae_algo = &ae_algo;
- nic->numa_node_mask = hdev->numa_node_mask;
+ bitmap_copy(nic->numa_node_mask.bits, hdev->numa_node_mask.bits,
+ MAX_NUMNODES);
nic->kinfo.io_base = hdev->hw.hw.io_base;
ret = hclge_knic_setup(vport, num_tqps,
@@ -2458,7 +2459,8 @@ static int hclge_init_roce_base_info(struct hclge_vport *vport)
roce->pdev = nic->pdev;
roce->ae_algo = nic->ae_algo;
- roce->numa_node_mask = nic->numa_node_mask;
+ bitmap_copy(roce->numa_node_mask.bits, nic->numa_node_mask.bits,
+ MAX_NUMNODES);
return 0;
}
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index e821dd2f1528..37527b847f2f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -891,7 +891,7 @@ struct hclge_dev {
u16 fdir_pf_filter_count; /* Num of guaranteed filters for this PF */
u16 num_alloc_vport; /* Num vports this driver supports */
- u32 numa_node_mask;
+ nodemask_t numa_node_mask;
u16 rx_buf_len;
u16 num_tx_desc; /* desc num of per tx queue */
u16 num_rx_desc; /* desc num of per rx queue */
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
index 0aa9beefd1c7..b57111252d07 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.c
@@ -412,7 +412,8 @@ static int hclgevf_set_handle_info(struct hclgevf_dev *hdev)
nic->ae_algo = &ae_algovf;
nic->pdev = hdev->pdev;
- nic->numa_node_mask = hdev->numa_node_mask;
+ bitmap_copy(nic->numa_node_mask.bits, hdev->numa_node_mask.bits,
+ MAX_NUMNODES);
nic->flags |= HNAE3_SUPPORT_VF;
nic->kinfo.io_base = hdev->hw.hw.io_base;
@@ -2082,8 +2083,8 @@ static int hclgevf_init_roce_base_info(struct hclgevf_dev *hdev)
roce->pdev = nic->pdev;
roce->ae_algo = nic->ae_algo;
- roce->numa_node_mask = nic->numa_node_mask;
-
+ bitmap_copy(roce->numa_node_mask.bits, nic->numa_node_mask.bits,
+ MAX_NUMNODES);
return 0;
}
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
index a73f2bf3a56a..cccef3228461 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_main.h
@@ -236,7 +236,7 @@ struct hclgevf_dev {
u16 rss_size_max; /* HW defined max RSS task queue */
u16 num_alloc_vport; /* num vports this driver supports */
- u32 numa_node_mask;
+ nodemask_t numa_node_mask;
u16 rx_buf_len;
u16 num_tx_desc; /* desc num of per tx queue */
u16 num_rx_desc; /* desc num of per rx queue */
--
2.30.0
From: Jian Shen <[email protected]>
Currently, the driver didn't return when receive a unknown
mailbox message, and continue checking whether need to
generate a response. It's unnecessary and may be incorrect.
Fixes: bb5790b71bad ("net: hns3: refactor mailbox response scheme between PF and VF")
Signed-off-by: Jian Shen <[email protected]>
Signed-off-by: Jijie Shao <[email protected]>
---
drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c
index d4a0e0be7a72..59c863306657 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_mbx.c
@@ -1077,12 +1077,13 @@ static void hclge_mbx_request_handling(struct hclge_mbx_ops_param *param)
hdev = param->vport->back;
cmd_func = hclge_mbx_ops_list[param->req->msg.code];
- if (cmd_func)
- ret = cmd_func(param);
- else
+ if (!cmd_func) {
dev_err(&hdev->pdev->dev,
"un-supported mailbox message, code = %u\n",
param->req->msg.code);
+ return;
+ }
+ ret = cmd_func(param);
/* PF driver should not reply IMP */
if (hnae3_get_bit(param->req->mbx_need_resp, HCLGE_MBX_NEED_RESP_B) &&
--
2.30.0
On Fri, Apr 26, 2024 at 06:00:43PM +0800, Jijie Shao wrote:
> From: Peiyang Wang <[email protected]>
>
> When a reset occurring, it's supposed to recover user's configuration.
> Currently, the port info(speed, duplex and autoneg) is stored in hclge_mac
> and will be scheduled updated. Consider the case that reset was happened
> consecutively. During the first reset, the port info is configured with
> a temporary value cause the PHY is reset and looking for best link config.
> Second reset start and use pervious configuration which is not the user's.
> The specific process is as follows:
>
> +------+ +----+ +----+
> | USER | | PF | | HW |
> +---+--+ +-+--+ +-+--+
> | ethtool --reset | |
> +------------------->| reset command |
> | ethtool --reset +-------------------->|
> +------------------->| +---+
> | +---+ | |
> | | |reset currently | | HW RESET
> | | |and wait to do | |
> | |<--+ | |
> | | send pervious cfg |<--+
> | | (1000M FULL AN_ON) |
> | +-------------------->|
> | | read cfg(time task) |
> | | (10M HALF AN_OFF) +---+
> | |<--------------------+ | cfg take effect
> | | reset command |<--+
> | +-------------------->|
> | | +---+
> | | send pervious cfg | | HW RESET
> | | (10M HALF AN_OFF) |<--+
> | +-------------------->|
> | | read cfg(time task) |
> | | (10M HALF AN_OFF) +---+
> | |<--------------------+ | cfg take effect
> | | | |
> | | read cfg(time task) |<--+
> | | (10M HALF AN_OFF) |
> | |<--------------------+
> | | |
> v v v
>
> To avoid aboved situation, this patch introduced req_speed, req_duplex,
> req_autoneg to store user's configuration and it only be used after
> hardware reset and to recover user's configuration
Hi Peiyang Wang and Jijie Shao,
In reviewing this patch it would be helpful if the diagram above could be
related to driver code. I'm sure it is obvious enough, but I'm having a
bit of trouble. F.e., is it hclge_tp_port_init() where "port info is
configured with a temporary value cause the PHY is reset and looking for
best link config." ?
>
> Fixes: f5f2b3e4dcc0 ("net: hns3: add support for imp-controlled PHYs")
> Signed-off-by: Peiyang Wang <[email protected]>
> Signed-off-by: Jijie Shao <[email protected]>
> ---
> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 15 +++++++++------
> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> index 6eda73f1e6ad..5dc8593c97be 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
> @@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev)
> cfg.default_speed, ret);
> return ret;
> }
> + hdev->hw.mac.req_speed = hdev->hw.mac.speed;
> + hdev->hw.mac.req_autoneg = AUTONEG_ENABLE;
> + hdev->hw.mac.req_duplex = DUPLEX_FULL;
I am curious to know why the initialisation of req_autoneg and req_duplex
is not:
hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
hdev->hw.mac.req_duplex = hdev->hw.mac.duplex
As it seems to me the value .autoneg is 0 (AUTONEG_DISABLE)
and the value of .duplex is 0 (DUPLEX_HALF).
> hclge_parse_link_mode(hdev, cfg.speed_ability);
>
> @@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
> return ret;
> }
>
> - hdev->hw.mac.autoneg = cmd->base.autoneg;
> - hdev->hw.mac.speed = cmd->base.speed;
> - hdev->hw.mac.duplex = cmd->base.duplex;
> + hdev->hw.mac.req_autoneg = cmd->base.autoneg;
> + hdev->hw.mac.req_speed = cmd->base.speed;
> + hdev->hw.mac.req_duplex = cmd->base.duplex;
> linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising);
>
> return 0;
> @@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev)
> if (ret)
> return ret;
>
> - hdev->hw.mac.autoneg = cmd.base.autoneg;
> - hdev->hw.mac.speed = cmd.base.speed;
> - hdev->hw.mac.duplex = cmd.base.duplex;
> + cmd.base.autoneg = hdev->hw.mac.req_autoneg;
> + cmd.base.speed = hdev->hw.mac.req_speed;
> + cmd.base.duplex = hdev->hw.mac.req_duplex;
It is unclear to me why fields of cmd are set here, cmd is a local variable
and they don't seem to be used for the rest of the function.
> linkmode_copy(hdev->hw.mac.advertising, cmd.link_modes.advertising);
>
> return 0;
..
On Fri, Apr 26, 2024 at 06:00:40PM +0800, Jijie Shao wrote:
> From: Peiyang Wang <[email protected]>
>
> It provides nodemask_t to describe the numa node mask in kernel. To
> improve transportability, change the type of numa_node_mask as nodemask_t.
>
> Fixes: 38caee9d3ee8 ("net: hns3: Add support of the HNAE3 framework")
> Signed-off-by: Peiyang Wang <[email protected]>
> Signed-off-by: Jijie Shao <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Fri, Apr 26, 2024 at 06:00:39PM +0800, Jijie Shao wrote:
> From: Jian Shen <[email protected]>
>
> Currently, the driver didn't return when receive a unknown
> mailbox message, and continue checking whether need to
> generate a response. It's unnecessary and may be incorrect.
>
> Fixes: bb5790b71bad ("net: hns3: refactor mailbox response scheme between PF and VF")
> Signed-off-by: Jian Shen <[email protected]>
> Signed-off-by: Jijie Shao <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
On Fri, Apr 26, 2024 at 06:00:44PM +0800, Jijie Shao wrote:
> From: Yonglong Liu <[email protected]>
>
> According to hardware limitation, for device support modify
> VLAN filter state but not support bypass port VLAN filter,
> it should always disable the port VLAN filter. but the driver
> enables port VLAN filter when initializing, if there is no
> VLAN(except VLAN 0) id added, the driver will disable it
> in service task. In most time, it works fine. But there is
> a time window before the service task shceduled and net device
> being registered. So if user adds VLAN at this time, the driver
> will not update the VLAN filter state, and the port VLAN filter
> remains enabled.
>
> To fix the problem, if support modify VLAN filter state but not
> support bypass port VLAN filter, set the port vlan filter to "off".
>
> Fixes: 184cd221a863 ("net: hns3: disable port VLAN filter when support function level VLAN filter control")
> Fixes: 2ba306627f59 ("net: hns3: add support for modify VLAN filter state")
For the record, my reading of this is:
184cd221a863 is a fix for 2ba306627f59. Both were included in v5.14.
This patch fixes 184cd221a863 and in turn 2ba306627f59.
> Signed-off-by: Yonglong Liu <[email protected]>
> Signed-off-by: Jijie Shao <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
..
on 2024/4/26 22:25, Simon Horman wrote:
> On Fri, Apr 26, 2024 at 06:00:43PM +0800, Jijie Shao wrote:
>> From: Peiyang Wang <[email protected]>
>>
>> When a reset occurring, it's supposed to recover user's configuration.
>> Currently, the port info(speed, duplex and autoneg) is stored in hclge_mac
>> and will be scheduled updated. Consider the case that reset was happened
>> consecutively. During the first reset, the port info is configured with
>> a temporary value cause the PHY is reset and looking for best link config.
>> Second reset start and use pervious configuration which is not the user's.
>> The specific process is as follows:
>>
>> +------+ +----+ +----+
>> | USER | | PF | | HW |
>> +---+--+ +-+--+ +-+--+
>> | ethtool --reset | |
>> +------------------->| reset command |
>> | ethtool --reset +-------------------->|
>> +------------------->| +---+
>> | +---+ | |
>> | | |reset currently | | HW RESET
>> | | |and wait to do | |
>> | |<--+ | |
>> | | send pervious cfg |<--+
>> | | (1000M FULL AN_ON) |
>> | +-------------------->|
>> | | read cfg(time task) |
>> | | (10M HALF AN_OFF) +---+
>> | |<--------------------+ | cfg take effect
>> | | reset command |<--+
>> | +-------------------->|
>> | | +---+
>> | | send pervious cfg | | HW RESET
>> | | (10M HALF AN_OFF) |<--+
>> | +-------------------->|
>> | | read cfg(time task) |
>> | | (10M HALF AN_OFF) +---+
>> | |<--------------------+ | cfg take effect
>> | | | |
>> | | read cfg(time task) |<--+
>> | | (10M HALF AN_OFF) |
>> | |<--------------------+
>> | | |
>> v v v
>>
>> To avoid aboved situation, this patch introduced req_speed, req_duplex,
>> req_autoneg to store user's configuration and it only be used after
>> hardware reset and to recover user's configuration
> Hi Peiyang Wang and Jijie Shao,
>
> In reviewing this patch it would be helpful if the diagram above could be
> related to driver code. I'm sure it is obvious enough, but I'm having a
> bit of trouble. F.e., is it hclge_tp_port_init() where "port info is
> configured with a temporary value cause the PHY is reset and looking for
> best link config." ?
Sorry, the description here is a bit confusing.
driver periodically updates port info. If only one reset occurs, driver
updates the port info based on the actual port info of the HW after
the reset is complete. However, When two resets occur consecutively,
If the port info of the HW is not updated in time before the second reset,
The driver may query a temporary info instead of the actual port info.
As a result, the actual port info is overwritten, Therefore, the port info
restored by the driver after the second reset is incorrect.
>> Fixes: f5f2b3e4dcc0 ("net: hns3: add support for imp-controlled PHYs")
>> Signed-off-by: Peiyang Wang <[email protected]>
>> Signed-off-by: Jijie Shao <[email protected]>
>> ---
>> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c | 15 +++++++++------
>> .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h | 3 +++
>> 2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> index 6eda73f1e6ad..5dc8593c97be 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
>> @@ -1537,6 +1537,9 @@ static int hclge_configure(struct hclge_dev *hdev)
>> cfg.default_speed, ret);
>> return ret;
>> }
>> + hdev->hw.mac.req_speed = hdev->hw.mac.speed;
>> + hdev->hw.mac.req_autoneg = AUTONEG_ENABLE;
>> + hdev->hw.mac.req_duplex = DUPLEX_FULL;
> I am curious to know why the initialisation of req_autoneg and req_duplex
> is not:
>
> hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
> hdev->hw.mac.req_duplex = hdev->hw.mac.duplex
>
> As it seems to me the value .autoneg is 0 (AUTONEG_DISABLE)
> and the value of .duplex is 0 (DUPLEX_HALF).
Yes, the value .autoneg is 0 and the value of .duplex is 0 here.
We hope that the network port is initialized in AUTONEG_ENABLE
and DUPLEX_FULL, so req_autoneg and req_duplex are fixed.
>
>> hclge_parse_link_mode(hdev, cfg.speed_ability);
>>
>> @@ -3344,9 +3347,9 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
>> return ret;
>> }
>>
>> - hdev->hw.mac.autoneg = cmd->base.autoneg;
>> - hdev->hw.mac.speed = cmd->base.speed;
>> - hdev->hw.mac.duplex = cmd->base.duplex;
>> + hdev->hw.mac.req_autoneg = cmd->base.autoneg;
>> + hdev->hw.mac.req_speed = cmd->base.speed;
>> + hdev->hw.mac.req_duplex = cmd->base.duplex;
>> linkmode_copy(hdev->hw.mac.advertising, cmd->link_modes.advertising);
>>
>> return 0;
>> @@ -3364,9 +3367,9 @@ static int hclge_update_tp_port_info(struct hclge_dev *hdev)
>> if (ret)
>> return ret;
>>
>> - hdev->hw.mac.autoneg = cmd.base.autoneg;
>> - hdev->hw.mac.speed = cmd.base.speed;
>> - hdev->hw.mac.duplex = cmd.base.duplex;
>> + cmd.base.autoneg = hdev->hw.mac.req_autoneg;
>> + cmd.base.speed = hdev->hw.mac.req_speed;
>> + cmd.base.duplex = hdev->hw.mac.req_duplex;
> It is unclear to me why fields of cmd are set here, cmd is a local variable
> and they don't seem to be used for the rest of the function.
I'm so sorry, the code here is wrong.
I will fix it in next version of this patch
Thank you very much for your review.