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

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

[Jijie Shao]

This patchset includes 3 fixes:
1. Patch1 fixes the issue of loopback failure under half-duplex mode.
  The driver will automatically switch to full-duplex mode
  before loopback tests.
2. Patch2 fixes an incorrect function return value.
3. Patch4 fixes the potential loss of user rate and duplex configuration
  after reset.

Jijie Shao (3):
  net: hns3: fix loopback test of serdes and phy is failed if duplex is
    half
  net: hns3: return error code when function fails
  net: hns3: use user configure after hardware reset when using kernel
    PHY

 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  26 ++--
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 139 ++++++++++++++----
 .../hisilicon/hns3/hns3pf/hclge_main.h        |   3 +-
 .../hisilicon/hns3/hns3pf/hclge_mdio.c        |   9 +-
 .../hisilicon/hns3/hns3pf/hclge_mdio.h        |   2 +-
 5 files changed, 130 insertions(+), 49 deletions(-)

-- 
2.33.0

[PATCH net 1/3] net: hns3: fix loopback test of serdes and phy is failed if duplex is half

[Jijie Shao]

If duplex setting is half, mac and phy can not transmit and receive data
at the same time, loopback test of serdes and phy will be failed, print
message as follow:
hns3 0000:35:00.2 eth2: self test start
hns3 0000:35:00.2 eth2: link down
hns3 0000:35:00.2 eth2: mode 2 recv fail, cnt=0x0, budget=0x1
hns3 0000:35:00.2 eth2: mode 3 recv fail, cnt=0x0, budget=0x1
hns3 0000:35:00.2 eth2: mode 4 recv fail, cnt=0x0, budget=0x1
hns3 0000:35:00.2 eth2: self test end
The test result is FAIL
The test extra info:
External Loopback test     4
App      Loopback test     0
Serdes   serial Loopback test     3
Serdes   parallel Loopback test     3
Phy      Loopback test     3

To fix this problem, duplex setting of mac or phy will be set to
full before serdes and phy starting loopback test, and restore duplex
setting after test is end.

Fixes: c39c4d98dc65 ("net: hns3: Add mac loopback selftest support in hns3 driver")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 26 +++++++++++++++++++
 .../hisilicon/hns3/hns3pf/hclge_main.h        |  1 +
 2 files changed, 27 insertions(+)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index f209a05e2033..78b10f2668e5 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -7844,8 +7844,15 @@ static int hclge_cfg_common_loopback(struct hclge_dev *hdev, bool en,
 static int hclge_set_common_loopback(struct hclge_dev *hdev, bool en,
 				     enum hnae3_loop loop_mode)
 {
+	u8 duplex;
 	int ret;
 
+	duplex = en ? DUPLEX_FULL : hdev->hw.mac.duplex;
+	ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.speed, duplex,
+					 hdev->hw.mac.lane_num);
+	if (ret)
+		return ret;
+
 	ret = hclge_cfg_common_loopback(hdev, en, loop_mode);
 	if (ret)
 		return ret;
@@ -7871,6 +7878,12 @@ static int hclge_enable_phy_loopback(struct hclge_dev *hdev,
 			return ret;
 	}
 
+	hdev->hw.mac.duplex_last = phydev->duplex;
+
+	ret = phy_set_bits(phydev, MII_BMCR, BMCR_FULLDPLX);
+	if (ret)
+		return ret;
+
 	ret = phy_resume(phydev);
 	if (ret)
 		return ret;
@@ -7887,12 +7900,19 @@ static int hclge_disable_phy_loopback(struct hclge_dev *hdev,
 	if (ret)
 		return ret;
 
+	if (hdev->hw.mac.duplex_last == DUPLEX_HALF) {
+		ret = phy_clear_bits(phydev, MII_BMCR, BMCR_FULLDPLX);
+		if (ret)
+			return ret;
+	}
+
 	return phy_suspend(phydev);
 }
 
 static int hclge_set_phy_loopback(struct hclge_dev *hdev, bool en)
 {
 	struct phy_device *phydev = hdev->hw.mac.phydev;
+	u8 duplex;
 	int ret;
 
 	if (!phydev) {
@@ -7902,6 +7922,12 @@ static int hclge_set_phy_loopback(struct hclge_dev *hdev, bool en)
 		return -ENOTSUPP;
 	}
 
+	duplex = en ? DUPLEX_FULL : hdev->hw.mac.duplex;
+	ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.speed, duplex,
+					 hdev->hw.mac.lane_num);
+	if (ret)
+		return ret;
+
 	if (en)
 		ret = hclge_enable_phy_loopback(hdev, phydev);
 	else
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 032b472d2368..36f2b06fa17d 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -283,6 +283,7 @@ struct hclge_mac {
 	u8 autoneg;
 	u8 req_autoneg;
 	u8 duplex;
+	u8 duplex_last;
 	u8 req_duplex;
 	u8 support_autoneg;
 	u8 speed_type;	/* 0: sfp speed, 1: active speed */
-- 
2.33.0

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

[Jijie Shao]

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

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

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

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

[PATCH net 3/3] net: hns3: use user configure after hardware reset when using kernel PHY

[Jijie Shao]

When a reset occurring, it's supposed to recover user's configuration.
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
previous configuration which is not the user's. Therefore, in this case,
the network port may not link up.

In this patch, saving user configure to req_xxx variable after successful
configuration when using kernel PHY or firmware PHY. And then, use the
req_xxx variable after hardware reset. Compared to the previous commit
"net: hns3: using user configure after hardware reset", it adds handling
for kernel PHY and FIBER.

Fixes: 05eb60e9648c ("net: hns3: using user configure after hardware reset")
Signed-off-by: Jijie Shao <shaojijie@huawei.com>
---
 .../ethernet/hisilicon/hns3/hns3_ethtool.c    |  26 ++---
 .../hisilicon/hns3/hns3pf/hclge_main.c        | 110 +++++++++++++-----
 .../hisilicon/hns3/hns3pf/hclge_main.h        |   2 +-
 3 files changed, 95 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index d5454e126c85..f7ac84840a2f 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -811,12 +811,11 @@ static int hns3_get_link_ksettings(struct net_device *netdev,
 }
 
 static int hns3_check_ksettings_param(const struct net_device *netdev,
-				      const struct ethtool_link_ksettings *cmd)
+				      const struct ethtool_link_ksettings *cmd,
+				      u8 media_type)
 {
 	struct hnae3_handle *handle = hns3_get_handle(netdev);
 	const struct hnae3_ae_ops *ops = hns3_get_ops(handle);
-	u8 module_type = HNAE3_MODULE_TYPE_UNKNOWN;
-	u8 media_type = HNAE3_MEDIA_TYPE_UNKNOWN;
 	u32 lane_num;
 	u8 autoneg;
 	u32 speed;
@@ -836,9 +835,6 @@ static int hns3_check_ksettings_param(const struct net_device *netdev,
 			return 0;
 	}
 
-	if (ops->get_media_type)
-		ops->get_media_type(handle, &media_type, &module_type);
-
 	if (cmd->base.duplex == DUPLEX_HALF &&
 	    media_type != HNAE3_MEDIA_TYPE_COPPER) {
 		netdev_err(netdev,
@@ -863,6 +859,8 @@ static int hns3_set_link_ksettings(struct net_device *netdev,
 	struct hnae3_handle *handle = hns3_get_handle(netdev);
 	struct hnae3_ae_dev *ae_dev = hns3_get_ae_dev(handle);
 	const struct hnae3_ae_ops *ops = hns3_get_ops(handle);
+	u8 module_type = HNAE3_MODULE_TYPE_UNKNOWN;
+	u8 media_type = HNAE3_MEDIA_TYPE_UNKNOWN;
 	int ret;
 
 	/* Chip don't support this mode. */
@@ -878,22 +876,20 @@ static int hns3_set_link_ksettings(struct net_device *netdev,
 		  cmd->base.autoneg, cmd->base.speed, cmd->base.duplex,
 		  cmd->lanes);
 
-	/* Only support ksettings_set for netdev with phy attached for now */
-	if (netdev->phydev) {
-		if (cmd->base.speed == SPEED_1000 &&
-		    cmd->base.autoneg == AUTONEG_DISABLE)
-			return -EINVAL;
+	if (!ops->get_media_type)
+		return -EOPNOTSUPP;
+	ops->get_media_type(handle, &media_type, &module_type);
 
-		return phy_ethtool_ksettings_set(netdev->phydev, cmd);
-	} else if (test_bit(HNAE3_DEV_SUPPORT_PHY_IMP_B, ae_dev->caps) &&
-		   ops->set_phy_link_ksettings) {
+	if (media_type == HNAE3_MEDIA_TYPE_COPPER) {
+		if (!ops->set_phy_link_ksettings)
+			return -EOPNOTSUPP;
 		return ops->set_phy_link_ksettings(handle, cmd);
 	}
 
 	if (ae_dev->dev_version < HNAE3_DEVICE_VERSION_V2)
 		return -EOPNOTSUPP;
 
-	ret = hns3_check_ksettings_param(netdev, cmd);
+	ret = hns3_check_ksettings_param(netdev, cmd, media_type);
 	if (ret)
 		return ret;
 
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
index 85f07219b8be..50150f1cd79c 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.c
@@ -1574,7 +1574,6 @@ static int hclge_configure(struct hclge_dev *hdev)
 		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);
@@ -2684,6 +2683,7 @@ static int hclge_set_autoneg(struct hnae3_handle *handle, bool enable)
 {
 	struct hclge_vport *vport = hclge_get_vport(handle);
 	struct hclge_dev *hdev = vport->back;
+	int ret;
 
 	if (!hdev->hw.mac.support_autoneg) {
 		if (enable) {
@@ -2695,7 +2695,10 @@ static int hclge_set_autoneg(struct hnae3_handle *handle, bool enable)
 		}
 	}
 
-	return hclge_set_autoneg_en(hdev, enable);
+	ret = hclge_set_autoneg_en(hdev, enable);
+	if (!ret)
+		hdev->hw.mac.req_autoneg = enable;
+	return ret;
 }
 
 static int hclge_get_autoneg(struct hnae3_handle *handle)
@@ -2957,20 +2960,6 @@ static int hclge_mac_init(struct hclge_dev *hdev)
 	if (!test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state))
 		hdev->hw.mac.duplex = HCLGE_MAC_FULL;
 
-	if (hdev->hw.mac.support_autoneg) {
-		ret = hclge_set_autoneg_en(hdev, hdev->hw.mac.autoneg);
-		if (ret)
-			return ret;
-	}
-
-	if (!hdev->hw.mac.autoneg) {
-		ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed,
-						 hdev->hw.mac.req_duplex,
-						 hdev->hw.mac.lane_num);
-		if (ret)
-			return ret;
-	}
-
 	mac->link = 0;
 
 	if (mac->user_fec_mode & BIT(HNAE3_FEC_USER_DEF)) {
@@ -3358,8 +3347,8 @@ static int hclge_get_phy_link_ksettings(struct hnae3_handle *handle,
 }
 
 static int
-hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
-			     const struct ethtool_link_ksettings *cmd)
+hclge_ethtool_ksettings_set(struct hnae3_handle *handle,
+			    const struct ethtool_link_ksettings *cmd)
 {
 	struct hclge_desc desc[HCLGE_PHY_LINK_SETTING_BD_NUM];
 	struct hclge_vport *vport = hclge_get_vport(handle);
@@ -3369,12 +3358,6 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
 	u32 advertising;
 	int ret;
 
-	if (cmd->base.autoneg == AUTONEG_DISABLE &&
-	    ((cmd->base.speed != SPEED_100 && cmd->base.speed != SPEED_10) ||
-	     (cmd->base.duplex != DUPLEX_HALF &&
-	      cmd->base.duplex != DUPLEX_FULL)))
-		return -EINVAL;
-
 	hclge_cmd_setup_basic_desc(&desc[0], HCLGE_OPC_PHY_LINK_KSETTING,
 				   false);
 	desc[0].flag |= cpu_to_le16(HCLGE_COMM_CMD_FLAG_NEXT);
@@ -3394,6 +3377,33 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
 	req1->master_slave_cfg = cmd->base.master_slave_cfg;
 
 	ret = hclge_cmd_send(&hdev->hw, desc, HCLGE_PHY_LINK_SETTING_BD_NUM);
+
+	if (!ret)
+		linkmode_copy(hdev->hw.mac.advertising,
+			      cmd->link_modes.advertising);
+
+	return ret;
+}
+
+static int
+hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
+			     const struct ethtool_link_ksettings *cmd)
+{
+	struct hclge_vport *vport = hclge_get_vport(handle);
+	struct hclge_dev *hdev = vport->back;
+	int ret;
+
+	if (cmd->base.autoneg == AUTONEG_DISABLE &&
+	    ((cmd->base.speed != SPEED_100 && cmd->base.speed != SPEED_10) ||
+	     (cmd->base.duplex != DUPLEX_HALF &&
+	      cmd->base.duplex != DUPLEX_FULL)))
+		return -EINVAL;
+
+	if (hnae3_dev_phy_imp_supported(hdev))
+		ret = hclge_ethtool_ksettings_set(handle, cmd);
+	else
+		ret = phy_ethtool_ksettings_set(handle->netdev->phydev, cmd);
+
 	if (ret) {
 		dev_err(&hdev->pdev->dev,
 			"failed to set phy link ksettings, ret = %d.\n", ret);
@@ -3401,9 +3411,10 @@ hclge_set_phy_link_ksettings(struct hnae3_handle *handle,
 	}
 
 	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);
+	if (cmd->base.speed != SPEED_UNKNOWN)
+		hdev->hw.mac.req_speed = cmd->base.speed;
+	if (cmd->base.duplex != DUPLEX_UNKNOWN)
+		hdev->hw.mac.req_duplex = cmd->base.duplex;
 
 	return 0;
 }
@@ -11747,6 +11758,27 @@ static int hclge_set_wol(struct hnae3_handle *handle,
 	return ret;
 }
 
+static int hclge_set_autoneg_speed_dup(struct hclge_dev *hdev)
+{
+	int ret;
+
+	if (hdev->hw.mac.support_autoneg) {
+		ret = hclge_set_autoneg_en(hdev, hdev->hw.mac.req_autoneg);
+		if (ret)
+			return ret;
+	}
+
+	if (!hdev->hw.mac.req_autoneg) {
+		ret = hclge_cfg_mac_speed_dup_hw(hdev, hdev->hw.mac.req_speed,
+						 hdev->hw.mac.req_duplex,
+						 hdev->hw.mac.lane_num);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 {
 	struct pci_dev *pdev = ae_dev->pdev;
@@ -11908,6 +11940,23 @@ static int hclge_init_ae_dev(struct hnae3_ae_dev *ae_dev)
 	if (ret)
 		goto err_ptp_uninit;
 
+	if (hdev->hw.mac.media_type == HNAE3_MEDIA_TYPE_COPPER)
+		hdev->hw.mac.req_autoneg = AUTONEG_ENABLE;
+	else
+		hdev->hw.mac.req_autoneg = hdev->hw.mac.autoneg;
+
+	/* When lane_num is 0, the firmware will automatically
+	 * select the appropriate lane_num based on the speed.
+	 */
+	hdev->hw.mac.lane_num = 0;
+
+	ret = hclge_set_autoneg_speed_dup(hdev);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to set autoneg speed duplex, ret = %d\n", ret);
+		goto err_ptp_uninit;
+	}
+
 	INIT_KFIFO(hdev->mac_tnl_log);
 
 	hclge_dcb_ops_set(hdev);
@@ -12238,6 +12287,13 @@ static int hclge_reset_ae_dev(struct hnae3_ae_dev *ae_dev)
 		return ret;
 	}
 
+	ret = hclge_set_autoneg_speed_dup(hdev);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to set autoneg speed duplex, ret = %d\n", ret);
+		return ret;
+	}
+
 	ret = hclge_tp_port_init(hdev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to init tp port, ret = %d\n",
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
index 36f2b06fa17d..f650b07bc739 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_main.h
@@ -285,7 +285,7 @@ struct hclge_mac {
 	u8 duplex;
 	u8 duplex_last;
 	u8 req_duplex;
-	u8 support_autoneg;
+	u8 support_autoneg;	/* for non-copper port */
 	u8 speed_type;	/* 0: sfp speed, 1: active speed */
 	u8 lane_num;
 	u32 speed;
-- 
2.33.0

Re: [PATCH net 1/3] net: hns3: fix loopback test of serdes and phy is failed if duplex is half

[Andrew Lunn]

On Wed, Sep 17, 2025 at 08:29:52PM +0800, Jijie Shao wrote:
> If duplex setting is half, mac and phy can not transmit and receive data
> at the same time.

Lets think about the fundamentals of Ethernet, MII and half duplex.

Is this specific to your MAC/PHY combination, or just generally true?

Should this is solved in phylib, because it is true for every MAC/PHY
combination? phylib returning -EINAL to phy_loopback() would seem like
the correct thing to do.

Is it specific to your PHY, but independent of the MAC?  Then the PHY
should return -EINVAL in its set_loopback() method.

> +	hdev->hw.mac.duplex_last = phydev->duplex;
> +
> +	ret = phy_set_bits(phydev, MII_BMCR, BMCR_FULLDPLX);

A MAC driver should not be doing this. What if the PHY is C45 only?
And Marvell PHYs need a soft reset before such an operation take
effect.

    Andrew

---
pw-bot: cr

Re: [PATCH net 3/3] net: hns3: use user configure after hardware reset when using kernel PHY

[Andrew Lunn]

On Wed, Sep 17, 2025 at 08:29:54PM +0800, Jijie Shao wrote:
> When a reset occurring

Why would a reset occur? Is it the firmware crashing?

> Consider the case that reset was happened consecutively.

Does that mean the firmware crashed twice in quick succession?

     Andrew

Re: [PATCH net 3/3] net: hns3: use user configure after hardware reset when using kernel PHY

[Jijie Shao]

on 2025/9/18 1:11, Andrew Lunn wrote:
> On Wed, Sep 17, 2025 at 08:29:54PM +0800, Jijie Shao wrote:
>> When a reset occurring
> Why would a reset occur? Is it the firmware crashing?
>
>> Consider the case that reset was happened consecutively.
> Does that mean the firmware crashed twice in quick succession?
>
>       Andrew

Actually, We can trigger a reset by ethtool:
ethtool --reset ethx...

Re: [PATCH net 1/3] net: hns3: fix loopback test of serdes and phy is failed if duplex is half

[Jijie Shao]

on 2025/9/18 0:58, Andrew Lunn wrote:
> On Wed, Sep 17, 2025 at 08:29:52PM +0800, Jijie Shao wrote:
>> If duplex setting is half, mac and phy can not transmit and receive data
>> at the same time.
> Lets think about the fundamentals of Ethernet, MII and half duplex.
>
> Is this specific to your MAC/PHY combination, or just generally true?
>
> Should this is solved in phylib, because it is true for every MAC/PHY
> combination? phylib returning -EINAL to phy_loopback() would seem like
> the correct thing to do.
>
> Is it specific to your PHY, but independent of the MAC?  Then the PHY
> should return -EINVAL in its set_loopback() method.

My idea is to prevent customers from seeing command failures,
so I switched from half-duplex to full-duplex for testing.
But it doesn't seem quite appropriate.


>
>> +	hdev->hw.mac.duplex_last = phydev->duplex;
>> +
>> +	ret = phy_set_bits(phydev, MII_BMCR, BMCR_FULLDPLX);
> A MAC driver should not be doing this. What if the PHY is C45 only?
> And Marvell PHYs need a soft reset before such an operation take
> effect.
>
>      Andrew

Thanks for the reminder.

Thanks，
Jijie Shao

Re: [PATCH net 3/3] net: hns3: use user configure after hardware reset when using kernel PHY

[Andrew Lunn]

On Fri, Sep 19, 2025 at 02:07:34PM +0800, Jijie Shao wrote:
> 
> on 2025/9/18 1:11, Andrew Lunn wrote:
> > On Wed, Sep 17, 2025 at 08:29:54PM +0800, Jijie Shao wrote:
> > > When a reset occurring
> > Why would a reset occur? Is it the firmware crashing?
> > 
> > > Consider the case that reset was happened consecutively.
> > Does that mean the firmware crashed twice in quick succession?
> > 
> >       Andrew
> 
> Actually, We can trigger a reset by ethtool:
> ethtool --reset ethx...
 
For that, RTNL should be held. So the configuration cannot change
while the reset is happening. Two resets in quick success should not
be an issue, because they will be complete sequential, one finished
before the next one started.

	Andrew