-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Tuesday, 30 August, 2022 8:17 PM
To: Jamaluddin, Aminuddin <redacted>
Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King
[off-list ref]; David S . Miller [off-list ref]; Eric
Dumazet [off-list ref]; Jakub Kicinski [off-list ref];
Paolo Abeni [off-list ref]; Ismail, Mohammad Athari
[off-list ref]; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min
[off-list ref]; Zulkifli, Muhammad Husaini
[off-list ref]
Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before
enabling phy loopback

quoted

quoted

quoted

@@ -2015,14 +2016,23 @@ static int m88e1510_loopback(struct

phy_device *phydev, bool enable)

quoted

 		if (err < 0)
 			return err;

-		/* FIXME: Based on trial and error test, it seem 1G need to

have

quoted

-		 * delay between soft reset and loopback enablement.
-		 */
-		if (phydev->speed == SPEED_1000)
-			msleep(1000);
+		if (phydev->speed == SPEED_1000) {
+			err = phy_read_poll_timeout(phydev, MII_BMSR,

val, val & BMSR_LSTATUS,

quoted

+						    PHY_LOOP_BACK_SLEEP,
+

PHY_LOOP_BACK_TIMEOUT, true);

Is this link with itself?

Its required cabled plug in, back to back connection.

Loopback should not require that. The whole point of loopback in the PHY is
you can do it without needing a cable.

quoted

quoted

Have you tested this with the cable unplugged?

Yes we have and its expected to have the timeout. But the self-test
required the link to be up first before it can be run.

So you get an ETIMEDOUT, and then skip the code which actually sets the
LOOPBACK bit?

If cable unplugged, test result will be displayed as 1. See comments below.

Please look at this again, and make it work without a cable.

Related to this the flow without cable, what we see in the codes during debugging.
After the phy loopback bit was set.
The test will be run through this __stmmac_test_loopback()
https://elixir.bootlin.com/linux/v5.19.8/source/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c#L320
Here, it will have another set of checking in dev_direct_xmit(), __dev_direct_xmit().
returning value 1(NET_XMIT_DROP)
https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4288
Which means the interface is not available or the interface link status is not up.
For this case the interface link status is not up. 
Thus failing the phy loopback test.
https://elixir.bootlin.com/linux/v5.19.8/source/net/core/dev.c#L4296
Since we don't own this __stmmac_test_loopback(), we conclude the behaviour was as expected.

Maybe you are addressing the wrong issue? Is the PHY actually performing
loopback, but reporting the link is down? Maybe you need to fake a link up?
Maybe you need the self test to not care about the link state, all it really
needs is that packets get looped?

When bit 14 was set, the link will be broken. 
But before the self-test was triggered it requires link to be up as stated above comments.

Amin