Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] net: phy: genphy_loopback: fix loopback failed when speed
 is unknown
To:     Andrew Lunn <andrew@lunn.ch>
CC:     <davem@davemloft.net>, <kuba@kernel.org>,
        <o.rempel@pengutronix.de>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <lipeng321@huawei.com>,
        <chenhao288@hisilicon.com>
References: <20220331114819.14929-1-huangguangbin2@huawei.com>
 <YkWdTpCsO8JhiSaT@lunn.ch>
From:   "huangguangbin (A)" <huangguangbin2@huawei.com>
Message-ID: <130bb780-0dc1-3819-8f6d-f2daf4d9ece9@huawei.com>
Date:   Thu, 31 Mar 2022 21:57:06 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <YkWdTpCsO8JhiSaT@lunn.ch>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2022/3/31 20:23, Andrew Lunn wrote:
> On Thu, Mar 31, 2022 at 07:48:19PM +0800, Guangbin Huang wrote:
>> If phy link status is down because link partner goes down, the phy speed
>> will be updated to SPEED_UNKNOWN when autoneg on with general phy driver.
>> If test loopback in this case, the phy speed will be set to 10M. However,
>> the speed of mac may not be 10M, it causes loopback test failed.
>>
>> To fix this problem, if speed is SPEED_UNKNOWN, don't configure link speed.
> 
> I don't think this explanation is correct.
> 
> If speed is UNKNOWN, ctl is just going to have BMCR_LOOPBACK set. That
> is very similar to what you are doing. The code then waits for the
> link to establish. This is where i guess your problem is. Are you
> seeing ETIMEDOUT? Does the link not establish?
> 
> Thanks
> 	Andrew	
> .
> 
Hi Andrew
This problem is not timeout, I have print return value of phy_read_poll_timeout()
and it is 0.

In this case, as speed and duplex both are unknown, ctl is just set to 0x4000.
However, the follow code sets mask to ~0 for function phy_modify():
int genphy_loopback(struct phy_device *phydev, bool enable)
{
	if (enable) {
		...
		phy_modify(phydev, MII_BMCR, ~0, ctl);
		...
}
so all other bits of BMCR will be cleared and just set bit 14, I use phy trace to
prove that:

$ cat /sys/kernel/debug/tracing/trace
# tracer: nop
#
# entries-in-buffer/entries-written: 923/923   #P:128
#
#                                _-----=> irqs-off/BH-disabled
#                               / _----=> need-resched
#                              | / _---=> hardirq/softirq
#                              || / _--=> preempt-depth
#                              ||| / _-=> migrate-disable
#                              |||| /     delay
#           TASK-PID     CPU#  |||||  TIMESTAMP  FUNCTION
#              | |         |   |||||     |         |
   kworker/u257:2-694     [015] .....   209.263912: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x1040
   kworker/u257:2-694     [015] .....   209.263951: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
   kworker/u257:2-694     [015] .....   209.263990: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
   kworker/u257:2-694     [015] .....   209.264028: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x09 val:0x0200
   kworker/u257:2-694     [015] .....   209.264067: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x0a val:0x0000
          ethtool-1148    [007] .....   209.665693: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x1040
          ethtool-1148    [007] .....   209.665706: mdio_access: mii-0000:bd:00.1 write phy:0x03 reg:0x00 val:0x1840
          ethtool-1148    [007] .....   210.588139: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x1840
          ethtool-1148    [007] .....   210.588152: mdio_access: mii-0000:bd:00.1 write phy:0x03 reg:0x00 val:0x1040
          ethtool-1148    [007] .....   210.615900: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x1040
          ethtool-1148    [007] .....   210.615912: mdio_access: mii-0000:bd:00.1 write phy:0x03 reg:0x00 val:0x4000 //here just set bit 14
          ethtool-1148    [007] .....   210.620952: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.625992: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.631034: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.636075: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.641116: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.646159: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.651215: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.656256: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.661296: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.666338: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x7989
          ethtool-1148    [007] .....   210.671378: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x798d
          ethtool-1148    [007] .....   210.679016: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x4000
          ethtool-1148    [007] .....   210.679053: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x798d
          ethtool-1148    [007] .....   210.679091: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x09 val:0x0200
          ethtool-1148    [007] .....   210.679129: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x0a val:0x0000
          ethtool-1148    [007] .....   210.695902: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x00 val:0x4000
          ethtool-1148    [007] .....   210.695939: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x01 val:0x798d
          ethtool-1148    [007] .....   210.695977: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x09 val:0x0200
          ethtool-1148    [007] .....   210.696014: mdio_access: mii-0000:bd:00.1 read  phy:0x03 reg:0x0a val:0x0000

So phy speed will be set to 10M in this case, if previous speed of device before going
down is 10M, loopback test is pass. Only previous speed is 100M or 1000M, loopback test is failed.

Thanks
	Guangbin
.