Messages in this thread | | | From | "Jamaluddin, Aminuddin" <> | Subject | RE: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback | Date | Thu, 27 Oct 2022 04:35:49 +0000 |
| |
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, 20 September, 2022 7:46 AM > To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@intel.com> > Cc: Heiner Kallweit <hkallweit1@gmail.com>; Russell King > <linux@armlinux.org.uk>; David S . Miller <davem@davemloft.net>; Eric > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > Paolo Abeni <pabeni@redhat.com>; Ismail, Mohammad Athari > <mohammad.athari.ismail@intel.com>; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; stable@vger.kernel.org; Tan, Tee Min > <tee.min.tan@intel.com>; Zulkifli, Muhammad Husaini > <muhammad.husaini.zulkifli@intel.com> > Subject: Re: [PATCH net 1/1] net: phy: marvell: add link status check before > enabling phy loopback > > > > > 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. > > > > > > > > > > > > > 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/s > > tmicro/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. > > You have not said anything about my comment: > > > Maybe you need to fake a link up? > > My guess is, some PHYs are going to report link up when put into loopback. > Others might not. For the Marvell PHY, it looks like you need to make > marvell_read_status() return that the link is up if loopback is enabled. >
We able to do the PHY loopback test, after fake link up without cable plugged on as suggested above. We will provide version 2 patch with minimum code changes without having the status link check. Only need to increase the msleep(200) to msleep(1000) inside m88e1510_loopback() function.
Amin
| |