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

[Przemek Kitszel]

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@xxxxxxxxxx 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().