RE: [PATCH net-next 0/3] lan743x: This series of patches are for lan743x driver testing

[<Mohan.Prasad@microchip.com>]

Hello Andrew,

Thank you very much for the feedback and the brief explanation.

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Fri, Sep 06, 2024 at 06:45:53AM +0000, Mohan.Prasad@microchip.com
> wrote:
> > Hello Andrew,
> >
> > Thank you for your review comments.
> >
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > >
> > > On Wed, Sep 04, 2024 at 03:45:46AM +0530, Mohan Prasad J wrote:
> > > > This series of patches are for testing the lan743x network driver.
> > > > Testing comprises autonegotiation, speed, duplex and throughput
> checks.
> > > > Tools such as ethtool, iperf3 are used in the testing process.
> > > > Performance test is done for TCP streams at different speeds.
> > >
> > > What is specific to lan743x? Why won't the autoneg test work for any
> > > interface which says it supports autoneg? Is duplex somehow special
> > > on the lan743x?
> > >
> > > Where possible, please try to make these tests generic, usable on
> > > any NIC. Or clearly document why they cannot be generic.
> > >
> >
> > As suggested, I will change the testcases to generic form and document
> them accordingly in the next version.
> 
> Great.
> 
> How much time do you have?
> 
> ethtool eth0
> Settings for eth0:
>         Supported ports: [ TP    MII ]
>         Supported link modes:   10baseT/Half 10baseT/Full
>                                 100baseT/Half 100baseT/Full
>                                 1000baseT/Half 1000baseT/Full
>         Supported pause frame use: Symmetric Receive-only
>         Supports auto-negotiation: Yes
>         Supported FEC modes: Not reported
>         Advertised link modes:  10baseT/Half 10baseT/Full
>                                 100baseT/Half 100baseT/Full
>                                 1000baseT/Half 1000baseT/Full
>         Advertised pause frame use: Symmetric Receive-only
>         Advertised auto-negotiation: Yes
>         Advertised FEC modes: Not reported
>         Link partner advertised link modes:  10baseT/Half 10baseT/Full
>                                              100baseT/Half 100baseT/Full
>                                              1000baseT/Full
> 
> You can see that both the local device and the peer support auto-neg. You
> can see what link modes both the local and peer supports. The local device
> has 1000BaseT/Half where the peer does not, which is reasonably common.
> So you could use this as a basis for the test, ensurer 5 link modes do pass
> auto-neg, and one fails.
> 
> If you can, please try to avoid hard coding any link modes. There will be some
> data centre NICs with a lowest speed to 10GBaseX, for example.
> There are some automotive devices with 10BaseT-1L which does not support
> autp-neg etc. It would be nice if the test could be used on any interface and
> the test will decide itself what can be tested, or if it should skip everything?
> 
> And by the way, thanks for working on tests. We need more people like you
> contributing to them.
> 

I am currently working on this and would rework as soon as possible.
The feedback that you provided is highly helpful and I will remodel the implementation with these points in mind.
Hopefully you can see that in the next version.

>         Andrew