Re: [PATCH net v2] selftests: drv-net: pause: Avoid cleanup failures for unsupported link settings

[Simon Horman]

On Thu, Nov 05, 2026 at 10:08:26AM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The fix is incomplete: the same cleanup failure still happens on
>   the link-autoneg-on branch of pause_setup() in…
> - [Low] In pause_setup()
>   (tools/testing/selftests/drivers/net/hw/pause.py, local lines 410-414,…
> 
> --
> 
> selftests: drv-net: pause: Avoid cleanup failures for unsupported link settings
> 
> This patch changes pause_setup() so that the forced-link restore is queued
> only after setup has successfully enabled link autonegotiation. The goal is
> to avoid EOPNOTSUPP cleanup failures on devices such as fbnic. It also makes
> pause_test_support() fall back to checking forced MAC pause on the current
> link when no link configuration change is accepted.
> 
> > diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> > index ca4a55e59c30f..99ce6a59dfcd6 100755
> > --- a/tools/testing/selftests/drivers/net/hw/pause.py
> > +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> > @@ -404,14 +404,14 @@ def pause_setup(cfg):
> >  
> >      if link["auto-negotiation"]:
> >          defer(cmd, f"ethtool -s {cfg.ifname} autoneg on")
> 
> [Severity: High]
> Can the same cleanup failure still happen on this branch?
> 
> This restore is still queued whenever the device reports link autoneg on,
> even when setup never changes the link. The remote interface has the same
> unconditional defer further down in pause_setup().
> 
> When the driver has no set_link_ksettings, ethnl_set_linkmodes_validate()
> rejects every LINKMODES_SET before it checks for a no-op:
> 
> net/ethtool/linkmodes.c:ethnl_set_linkmodes_validate() {
>     ...
> 	if (!ops->get_link_ksettings || !ops->set_link_ksettings)
> 		return -EOPNOTSUPP;
>     ...
> }
> 
> That means a restore to the same autoneg on value also fails with
> EOPNOTSUPP.
> 
> Some in-tree drivers report autoneg on and provide only get_link_ksettings.
> nicvf_get_link_ksettings() sets cmd->base.autoneg = AUTONEG_ENABLE.
> be_get_link_ksettings() does the same when auto speeds are supported.
> fbnic also has get_link_ksettings and no set_link_ksettings.
> 
> On these devices pause_test_support() takes the new "current link"
> fallback. The deferred "ethtool -s <if> autoneg on" then fails in
> ksft_flush_defer() and the test ends in a cleanup failure. The subject
> line says the patch avoids exactly this.
> 
> Every other test that calls pause_setup() would hit it too, because the
> defer is flushed even when the test skips.
> 
> Dropping the autoneg on restore isn't an option, since pause_test_support()
> and pause_autoneg_link_autoneg() force the link and rely on it. Would it
> work to queue the restore only where a link change is accepted? One place
> is after a successful "autoneg off {forced}" in pause_test_support().
> Another option is to let the restore tolerate EOPNOTSUPP.

This looks like a legitimate concern that warrants a v3.

> 
> >  
> >      # Local interface admin up, link aneg on
> >      ip(f"link set {cfg.ifname} up")
> >      if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
> >          ethtool(f"-s {cfg.ifname} autoneg on")
> > +        if "speed" in link and "duplex" in link:
> > +            defer(cmd, f"ethtool -s {cfg.ifname} autoneg off speed {link['speed']} "
> > +                       f"duplex {link['duplex'].lower()}")
> 
> [Severity: Low]
> Could this leave a forced link in autoneg state if the ethtool call fails
> partway through?
> 
> ethtool() raises on a non-zero exit. With the defer now registered after
> the call, nothing restores the original forced speed/duplex if the driver
> commits part of the change and then returns an error.
> 
> bnxt_set_link_ksettings() is one example. It sets
> link_info->autoneg |= BNXT_AUTONEG_SPEED before calling
> bnxt_hwrm_set_link_setting(), and returns that call's error without
> rolling back.
> 
> This branch only runs when the device supports autoneg. Would queuing the
> restore just before the ethtool call still avoid the fbnic cleanup
> failure? The same ordering applies to the remote interface later in
> pause_setup().
> 
> [ ... ]

This one I'm less sure about, but I think it should at least be considered.

-- 
pw-bot: cr