Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 9/29/26 8:11 PM, Tony Nguyen wrote:
> 
> 
> On 9/28/2026 12:14 AM, Linkui Xiao wrote:
>> Hi,
>>
>> Thanks for the review. Please find the missing information below.
>>
>> - How the issue was discovered:
>>    Found during manual code inspection of the ice VF setup/teardown 
>> error paths. It was not reported by syzbot, a static analysis tool, or 
>> an LLM scan, and was not hit in production.
>>
>> - Whether the issue was actually triggered:
>>    Not actually triggered. It is a theoretical error-path cleanup 
>> issue found by inspection; no stack trace or error message was observed.
>>
>> - Hardware tested:
>>    Not tested on real hardware. The change was only compile-tested; no 
>> affected Intel NIC/firmware test was performed.
>>
>> The same applies to patch 2/2: it was also found by the same manual 
>> code inspection, was not triggered at runtime, and was only compile- 
>> tested, not tested on real hardware.
>>
>> If a v3 is needed for other review reasons, I will include this 
>> information in the commit messages.
>>
>> Thanks,
>> Linkui Xiao
>>
>> On 2026/9/28 14:59, netdev-bot+sinfo@kernel.org wrote:
>>> Hi!
>>>
>>> This is an automated message. This series looks like a fix, but its
>>> commit messages seem to be missing some information:
>>>
>>>   - How the issue was discovered, e.g. hit in production, hit during
>>>     development, syzbot report, manual code inspection, LLM or static
>>>     analysis tool scan.
>>>
>>>   - Whether the issue was actually triggered, or is only theoretical
>>>     (e.g. found by code inspection). If it was triggered please include
>>>     the symptoms, like the stack trace or error messages.
>>>
>>>   - What hardware the change was tested on. For driver fixes please
>>>     mention the device (and if relevant firmware version) used for
>>>     testing, or say that the change was not tested on real hardware.
>>>
>>> Please do not repost the series just to address the above. Instead,
>>> reply to this email with the missing information, so that reviewers
>>> can take it into account. If the series needs another revision for
>>> other reasons, please include the information in the commit messages
>>> then.
> 
> Hi Jakub,
> 
> For patches that go through iwl-net, how did you want this handled? It 
> says to not repost just to address the above, but since this will get 
> submitted later, did you want the commit updated to make the latter 
> check happy, did you want me to copy/paste the response to the commit, 
> or something else?

In general, I would say it's up to you Tony.
Perhaps we could decide based on the amount of additional changes.

For this particular patch sashiko already posted complains:
https://sashiko.dev/#/patchset/20260928065306.1514795-1-xiaolinkui%40126.com?part=1

I've encountered the same problem (although during my OOT encounters),
and the solution is to detach/attach VF representors outside of
vf->cfg_lock. More rationale by AI follows:

ice_reset_all_vfs() and ice_free_vfs() call ice_eswitch_detach() and
ice_eswitch_attach() with vf->cfg_lock held. Both take the devlink
instance lock and then register or unregister the representor netdev,
which takes RTNL. ndo_set_vf_mac(), ndo_set_vf_vlan() and the
representor's ethtool reset take vf->cfg_lock under RTNL, so the order
is inverted. The ice_check_vf_ready_for_cfg() check in those callers
runs before vf->cfg_lock is taken, so it doesn't keep them away from a
reset that is already in progress.

Detach before taking vf->cfg_lock and attach after releasing it.
Representors are added and removed only under pf->vfs.table_lock, and
ice_start_vfs() already attaches without vf->cfg_lock. Both functions
set ICE_VF_DIS first, so a concurrent ice_reset_vf() bails out before it
reaches ice_eswitch_update_repr().