Re: [PATCH net] ice: fix netdev bring-up and bring-down in self-test

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Anthony Nguyen <anthony.l.nguyen@intel.com>
Date: Wed, 8 Apr 2026 14:12:28 -0700

> 
> 
> On 3/27/2026 12:23 AM, Aleksandr Loktionov wrote:
>> From: Konstantin Ilichev <konstantin.ilichev@intel.com>
>>
>> When an offline self-test is initiated with ethtool -t, any ongoing
>> traffic could get stuck because ice_stop() and ice_open() are called
>> without letting the OS know about state transitions.  In most cases
>> a write() system call would block.
>>
>> Fix this by calling dev_change_flags() to bring the netdev up and
>> down, which ensures ndo_open()/ndo_stop() are called and all watchers
>> are notified correctly.
> 
> + Olek
> 
> AI review reports:
> 
> The ethtool core acquires the per-netdev mutex via netdev_lock_ops(dev)
> before invoking the driver's .self_test callback. dev_change_flags() is
> an exported API that explicitly re-acquires this exact same lock:
> net/core/dev_api.c:dev_change_flags() {
>     ...
>     netdev_lock_ops(dev);
>     ret = netif_change_flags(dev, flags, extack);
>     netdev_unlock_ops(dev);
>     ...
> }
> Because dev->lock is a standard, non-recursive mutex, this will result
> in a hard deadlock for any driver that opts into request_ops_lock. While
> ice might not currently set this flag, introducing nested lock
> acquisitions of the same mutex guarantees a deadlock as the subsystem
> migrates toward per-netdev locking.
> 
> 
> With ice netdev lock changes in progress [1], this would soon become an
> issue.

Hmmm, seems like we'll need to either export netif_change_flags() or
introduce something like dev_change_flags_locked()...

> 
> Thanks,
> Tony
> 
> [1] https://lore.kernel.org/netdev/20260325200644.2528726-4-
> anthony.l.nguyen@intel.com/
> 
>> Fixes: 0e674aeb0b77 ("ice: Add handler for ethtool selftest")
>> Cc: stable@vger.kernel.org
>> Co-developed-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
>> Signed-off-by: Grzegorz Nitka <grzegorz.nitka@intel.com>
>> Signed-off-by: Konstantin Ilichev <konstantin.ilichev@intel.com>
>> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>> ---
>>
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/
>> net/ethernet/intel/ice/ice_ethtool.c
>> index 96d95af..2a4f06f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -1416,7 +1416,7 @@ ice_self_test(struct net_device *netdev, struct
>> ethtool_test *eth_test,
>>           /* If the device is online then take it offline */
>>           if (if_running)
>>               /* indicate we're in test mode */
>> -            ice_stop(netdev);
>> +            dev_change_flags(netdev, netdev->flags & ~IFF_UP, NULL);
>>             data[ICE_ETH_TEST_LINK] = ice_link_test(netdev);
>>           data[ICE_ETH_TEST_EEPROM] = ice_eeprom_test(netdev);
>> @@ -1434,10 +1434,12 @@ ice_self_test(struct net_device *netdev,
>> struct ethtool_test *eth_test,
>>           clear_bit(ICE_TESTING, pf->state);
>>             if (if_running) {
>> -            int status = ice_open(netdev);
>> +            int status = dev_change_flags(netdev,
>> +                              netdev->flags | IFF_UP,
>> +                              NULL);

BTW are you sure there's no other way to do an ifdown/ifup cycle other
than sneaking into the flags? Have you checked whether there's an
exported generic function which calls .ndo_open()?

>>                 if (status) {
>> -                dev_err(dev, "Could not open device %s, err %d\n",
>> +                dev_err(dev, "Could not bring up device %s, err %d\n",
>>                       pf->int_name, status);
>>               }
>>           }

Thanks,
Olek