[U-Boot] [PATCH] test/py: Detect timeout in phy negotiation

[Michal Simek]

On 17.5.2016 21:09, Stephen Warren wrote:
> On 05/17/2016 12:00 PM, Michal Simek wrote:
>> On 17.5.2016 19:36, Stephen Warren wrote:
>>> On 05/17/2016 11:03 AM, Michal Simek wrote:
>>>> On 17.5.2016 19:00, Stephen Warren wrote:
>>>>> On 05/17/2016 10:56 AM, Michal Simek wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 17.5.2016 18:50, Stephen Warren wrote:
>>>>>>> On 05/17/2016 07:57 AM, Michal Simek wrote:
>>>>>>>> If timeout happen it should be reported as fault.
>>>>>>>
>>>>>>> Presumably if a timeout occurs, the expected text does not appear,
>>>>>>> i.e.
>>>>>>> the existing assert fails anyway?
>>>>>>>
>>>>>>> Anyway, it's useful to point out problems explicitly, so,
>>>>>>> Acked-by: Stephen Warren <swarren at nvidia.com>
>>>>>>
>>>>>> Unfortunately I found this issue when I was checking logs where I am
>>>>>> getting this.
>>>>>>
>>>>>> ethernet at e000b000 Waiting for PHY auto negotiation to
>>>>>> complete.........
>>>>>> TIMEOUT !
>>>>>> BOOTP broadcast 1
>>>>>> BOOTP broadcast 2
>>>>>> BOOTP broadcast 3
>>>>>> DHCP client bound to address 192.168.0.107 (882 ms)
>>>>>> Zynq> .Zynq> setenv serverip 192.168.0.105
>>>>>>
>>>>>> I haven't looked at the exact reason why it is failing but IMHO it is
>>>>>> worth to check.
>>>>>
>>>>> Oh, in that case I think I should withdraw my ack; in the log
>>>>> above, the
>>>>> operation completed successfully, so I'm not convinced the test should
>>>>> be marked a failure. I thought this change simply provided more detail
>>>>> re: the cause of a test failure.
>>>>
>>>> Is there any other way how to run just phy negotiation and mark this
>>>> test as fail?
>>>
>>> I don't see anything obvious that will do that; I think that only
>>> happens when net_loop starts. It might be possible to add some new
>>> command to test just PHY startup, or a new mode for net_loop() that just
>>> waited for link up and did no protocol work.
>>
>> ok. It means you are saying that this is bug and we are not able to test
>> it by single test. That's why I think this should be report as a bug
>> when this is happening just to hands up there is a problem.
>>
>> Not sure if there is any way to report is as a warning instead of bug
>> that test case will pass with warning.
> 
> At least my only-marginally-old version of pytest (2.5.1) doesn't appear
> to have any concept of warnings. Some quite new versions of pytest do,
> although I'm not sure how they're reported. Integrating some form of
> warning system into the tests could be useful though.

ok.

> 
> I wonder if the correct fix for this issue isn't to make the PHY code
> actively return an error whenever there is a TIMEOUT raised, rather than
> presumably ignoring it as it does now (since the network code goes on to
> actually work correctly)?

I will send some patches to fix this problem that transfers not start
when TIMEOUT happens.

All these boards should be fixed to return error codes properly.
drivers/net/cpsw.c:612:	phy_startup(phy);
drivers/net/keystone_net.c:581:		phy_startup(phy_dev);
drivers/net/keystone_net.c:768:		phy_startup(priv->phydev);
drivers/net/lpc32xx_eth.c:605:	phy_startup(phydev);
drivers/net/mvgbe.c:697:	phy_startup(phydev);
drivers/net/mvneta.c:1478:		phy_startup(phydev);
drivers/net/mvpp2.c:3384:		phy_startup(phy_dev);
drivers/net/pic32_eth.c:343:	phy_startup(priv->phydev);

But again if we have visible errors and code is not handling it properly
I do agree that code should be fixed first but still testing should be
able to capture this problem for others to avoid that this problem
happens again.

Thanks,
Michal