RE: [PATCH net 1/1] net: phy: marvell: add link status check before enabling phy loopback

[Jamaluddin, Aminuddin]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Tuesday, 20 September, 2022 7:46 AM
> To: Jamaluddin, Aminuddin <aminuddin.jamaluddin@xxxxxxxxx>
> Cc: Heiner Kallweit <hkallweit1@xxxxxxxxx>; Russell King
> <linux@xxxxxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
> Paolo Abeni <pabeni@xxxxxxxxxx>; Ismail, Mohammad Athari
> <mohammad.athari.ismail@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; Tan, Tee Min
> <tee.min.tan@xxxxxxxxx>; Zulkifli, Muhammad Husaini
> <muhammad.husaini.zulkifli@xxxxxxxxx>
> 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