diff mbox series

[V3,net,1/7] net: hns3: using user configure after hardware reset

Message ID 20240507134224.2646246-2-shaojijie@huawei.com (mailing list archive)
State Accepted
Commit 05eb60e9648cca0beeebdbcd263b599fb58aee48
Delegated to: Netdev Maintainers
Headers show
Series There are some bugfix for the HNS3 ethernet driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 2 blamed authors not CCed: huangguangbin2@huawei.com tanhuazhong@huawei.com; 2 maintainers not CCed: huangguangbin2@huawei.com tanhuazhong@huawei.com
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 47 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Jijie Shao May 7, 2024, 1:42 p.m. UTC 
From: Peiyang Wang <wangpeiyang1@huawei.com>

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 <wangpeiyang1@huawei.com>
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.c   | 15 +++++++++------
 .../ethernet/hisilicon/hns3/hns3pf/hclge_main.h   |  3 +++
 2 files changed, 12 insertions(+), 6 deletions(-)

Comments

Przemek Kitszel May 7, 2024, 3:20 p.m. UTC | #1 
On 5/7/24 15:42, Jijie Shao wrote:
> From: Peiyang Wang <wangpeiyang1@huawei.com>
> 
> 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.

nit: for future submissions please run your commit messages through
spellchecker

> 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 <wangpeiyang1@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> ---
>   .../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 ff6a2ed23ddb..8043f1795dc7 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);
>   
> @@ -3342,9 +3345,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;
> @@ -3377,9 +3380,9 @@ static int hclge_tp_port_init(struct hclge_dev *hdev)
>   	if (!hnae3_dev_phy_imp_supported(hdev))
>   		return 0;
>   
> -	cmd.base.autoneg = hdev->hw.mac.autoneg;
> -	cmd.base.speed = hdev->hw.mac.speed;
> -	cmd.base.duplex = hdev->hw.mac.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(cmd.link_modes.advertising, hdev->hw.mac.advertising);
>   
>   	return hclge_set_phy_link_ksettings(&hdev->vport->nic, &cmd);
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
> index e821dd2f1528..e3c69be8256f 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 */

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Markus Elfring May 7, 2024, 4:51 p.m. UTC | #2 
Can any wording adjustments be a bit nicer?


> When a reset occurring, it's supposed to recover user's configuration.

An user configuration should be recovered after a reset occurred.


…
> and will be scheduled updated. Consider the case that reset was happened

and the schedule will be updated. Consider also the case that reset happened


…
> To avoid aboved situation, this patch introduced …

* Would you like to avoid another typo here?

* How do you think about to use imperative wordings for improved change descriptions?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc7#n94

Regards,
Markus
Simon Horman May 8, 2024, 4:21 p.m. UTC | #3 
On Tue, May 07, 2024 at 09:42:18PM +0800, Jijie Shao wrote:
> From: Peiyang Wang <wangpeiyang1@huawei.com>
> 
> 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 <wangpeiyang1@huawei.com>
> Signed-off-by: Jijie Shao <shaojijie@huawei.com>

Thanks for the update since v1.

Reviewed-by: Simon Horman <horms@kernel.org>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index ff6a2ed23ddb..8043f1795dc7 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);
 
@@ -3342,9 +3345,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;
@@ -3377,9 +3380,9 @@  static int hclge_tp_port_init(struct hclge_dev *hdev)
 	if (!hnae3_dev_phy_imp_supported(hdev))
 		return 0;
 
-	cmd.base.autoneg = hdev->hw.mac.autoneg;
-	cmd.base.speed = hdev->hw.mac.speed;
-	cmd.base.duplex = hdev->hw.mac.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(cmd.link_modes.advertising, hdev->hw.mac.advertising);
 
 	return hclge_set_phy_link_ksettings(&hdev->vport->nic, &cmd);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index e821dd2f1528..e3c69be8256f 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 */