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

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

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