Re: [Intel-wired-lan] [PATCH net] iavf: fix deadlock in reset handling

[Petr Oros <poros@redhat.com>]

On 2/3/26 11:19, Przemek Kitszel wrote:
> On 2/3/26 09:44, Petr Oros wrote:
>>
>> On 2/3/26 02:00, Jacob Keller wrote:
>>>
>>>
>>> On 2/2/2026 3:58 PM, Jakub Kicinski wrote:
>>>> On Mon,  2 Feb 2026 09:48:20 +0100 Petr Oros wrote:
>>>>> +    netdev_unlock(netdev);
>>>>> +    ret = wait_event_interruptible_timeout(adapter->reset_waitqueue,
>>>>> + !iavf_is_reset_in_progress(adapter),
>>>>> +                           msecs_to_jiffies(5000));
>>>>> +    netdev_lock(netdev);
>>>>
>>>> Dropping locks taken by the core around the driver callback
>>>> is obviously unacceptable. SMH.
>>>
>>> Right. It seems like the correct fix is to either a) have reset take 
>>> and hold the netdev lock (now that its distinct from the global RTNL 
>>> lock) or b) refactor reset so that it can defer any of the netdev 
>>> related stuff somehow.
>>>
>> I modeled this after the existing pattern in iavf_close() (ndo_stop), 
>> which also temporarily releases the netdev instance lock taken by the 
>> core to wait for an async operation to complete:
>
> First of all, thank you for working on that, I was hit by the very same
> problem (no series yet), but my local fix is the same as of now.
>
> I don't see an easy fix (w/o substantial driver refactor).
>
>>
>> static int iavf_close(struct net_device *netdev)
>> {
>>          netdev_assert_locked(netdev);
>>          ...
>>          iavf_down(adapter);
>>          iavf_change_state(adapter, __IAVF_DOWN_PENDING);
>>          iavf_free_traffic_irqs(adapter);
>>
>>          netdev_unlock(netdev);
>>
>>          status = wait_event_timeout(adapter->down_waitqueue,
>>                                      adapter->state == __IAVF_DOWN,
>>                                      msecs_to_jiffies(500));
>>          if (!status)
>>                  netdev_warn(netdev, "Device resources not yet 
>> released\n");
>>          netdev_lock(netdev);
>>          ...
>> }
>>
>> This was introduced by commit 120f28a6f314fe ("iavf: get rid of the 
>> crit lock"), and ndo_stop is called with netdev instance lock held by 
>> the core just like ndo_change_mtu is. 
>
> technically it was introduced by commmit afc664987ab3 ("eth: iavf:
> extend the netdev_lock usage")
>
>> Could you clarify why the unlock-wait- lock pattern is acceptable in 
>> ndo_stop but not here?
>>
>
> perhaps just closing netdev is a special kind of operation
>
> Other thing is that the lock was added to allow further NAPI
> development, and one silly driver should not stop that effort.
> Sadly, we have not managed to re-design the driver yet. I would like to
> do so personally, but have much work accumulated/pending to free my time
>
I agree, the unlock-wait-lock pattern is fundamentally flawed (I now 
understand
why it is unacceptable) and should be avoided.

What can we do now?

* Eliminating the wait is not an option: As noted in the description of 
commit
c2ed2403f12c, this wait was originally added to fix a race condition where
adding an interface to bonding failed because the device remained in
__RESETTING state after the callback returned.
* Passing the lock into reset is impractical: The reset path is 
triggered from
numerous contexts, many of which are not under the netdev_lock, making this
even more complex than a full refactor.

If dropping the lock is a no-go, the only viable path forward is to 
split the
reset_task so that the waiting portion is decoupled from the netdev_lock
critical section.

The fact remains that MTU configuration and ring parameter changes are
currently broken in iavf. Changing the MTU on a Virtual Function is a
fundamental configuration not an obscure edge case that can remain 
non-functional.

I would appreciate any further guidance on how you would prefer...