Re: [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control

[Andrew Lunn <andrew@lunn.ch>]

> > [Severity: Medium]
> > The defer queue is LIFO (ksft_flush_defer() pops from the end), so at
> > teardown the link restore runs first. The pause restore follows right
> > after it, with no wait for autoneg to complete.
> > 
> > Can the pause restore be refused in that window? i40e ties pause autoneg
> > to the link autoneg state:
> That's a i40e driver issue :) The tests aren't build around the HW behaviour.

Seems like the AI has not understood this is a test to find such
issues :-(

> > [Severity: Low]
> > This isn't a bug, but expect_pauseparams_set() also accepts EINVAL for
> > unsupported combinations:
> > 
> >     ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)
> > 
> > phylink_ethtool_set_pauseparam() also returns -EINVAL for rx != tx when
> > Asym_Pause is not supported. Should the docstring mention EINVAL too?
> 
> Docstrings are gonna be shrunk down as pylint says the file's too big... I'll
> update.

Please do consider the alternatives if the documentations is useful
for those trying to understand why a test has failed.

# pylint: disable=too-many-lines

> > Several drivers fail at least one of the four link autoneg / pause autoneg
> > combinations:
> > 
> >   bnxt_set_pauseparam() returns -EINVAL for pause autoneg without
> >   BNXT_AUTONEG_SPEED, which is what the uAPI describes.
> > 
> >   mlx5e_ethtool_set_pauseparam() returns -EINVAL for any pause autoneg
> >   request.
> > 
> >   ixgbe_set_pauseparam() returns -EINVAL when
> >   !ixgbe_device_supports_autoneg_fc().
> > 
> >   i40e_set_pauseparam() returns -EOPNOTSUPP whenever pause->autoneg !=
> >   is_an. That already fails the link autoneg on / pause autoneg off step.
> 
> That's why we write those tests

:-)

I wounder if saying in the commit message that this test is to find
drivers who pause implementation is wrong will help get the AI into
the 'right mind' about what it is reviewing'.

> > [Severity: Medium]
> > What happens when set_local_pauseparams() fails with something other than
> > EOPNOTSUPP?
> > 
> > The docstrings of set_local_pauseparams() and get_local_pauseparams() say
> > "Raise an error if the return is not 0 or EOPNOTSUPP". Neither function
> > raises. Both just return the result of ethtool_ret(), which uses
> > cmd(..., fail=False).
> > 
> > Here, an EINVAL, an EBUSY, or ethtool's raw exit code 1 is silently dropped,
> > and ret is overwritten by wait_for_aneg(). The advertising checks then run
> > against whatever state was there before, because pause_setup() does not
> > put the pause params into a known state.
> 
> Ok will address

EBUSY is an odd error code for these use cases. I would probably say
that is a test fail. It at least needs investigating why it happens.

	Andrew