Re: [PATCH net v6] net: libwx: fix VF PM suspend/resume flow

["mengyuanlou@net-swift.com" <mengyuanlou@net-swift.com>]

> 2026年10月22日 17:02，netdev-bot+sashiko@kernel.org 写道：
> 
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1 (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] net: libwx: race between wxvf_remove() and wxvf_reopen_subtask()
> - [Low] The v5 changelog says "Schedule the reopen subtask immediately on
>  resume; keep the service timer to retry if the PF is not ready."…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch adds
>  a new caller that depends on it.
> - [Low] This problem wasn't introduced by this patch, but VF teardown
>  never frees the coherent interrupt-status block (wx->isb_mem) that…
> 
> --
> 
> net: libwx: fix VF PM suspend/resume flow
> 
> This patch reworks the shared txgbevf/ngbevf PM callbacks. Suspend now
> closes a running VF under RTNL and drains the service timer and work
> before disabling PCI. Resume re-enables PCI and hands the PF readiness
> check, VF reset, interrupt setup and reopen to the service task through
> a new WX_FLAG_NEED_REOPEN flag.
> 
>> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
>> index 26de78e9a69ef..e2271e8bffcda 100644
>> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
>> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> 
> [ ... ]
> 
>> @@ -34,11 +48,20 @@ int wxvf_resume(struct device *dev_d)
>> {
>> struct pci_dev *pdev = to_pci_dev(dev_d);
>> struct wx *wx = pci_get_drvdata(pdev);
>> + int err;
>> 
>> - pci_set_master(pdev);
>> - wx_init_interrupt_scheme(wx);
>> - netif_device_attach(wx->netdev);
>> + err = pci_enable_device_mem(pdev);
>> + if (err) {
>> + dev_err(&pdev->dev, "Cannot enable PCI device from suspend\n");
>> + return err;
>> + }
>> 
>> + clear_bit(WX_STATE_DISABLED, wx->state);
>> + pci_set_master(pdev);
>> + rtnl_lock();
>> + mod_timer(&wx->service_timer, jiffies + HZ);
>> + set_bit(WX_FLAG_NEED_REOPEN, wx->flags);
>> + rtnl_unlock();
>> return 0;
>> }
> 
> [Severity: Low]
> The v5 changelog says:
> 
>  Schedule the reopen subtask immediately on resume; keep the service
>  timer to retry if the PF is not ready.
> 

This is removed in v6 after test.
And it was not mentioned in the changelog, which is a mistake.

> Does wxvf_resume() actually do that? It sets service_timer to fire at
> jiffies + HZ and sets WX_FLAG_NEED_REOPEN. It does not call
> wx_service_event_schedule() or queue_work().
> 
> The only place that queues the service work is the timer callback:
> 
> wx_lib.c:wx_service_timer() {
>    ...
> mod_timer(&wx->service_timer, next_event_offset + jiffies);
> 
> wx_service_event_schedule(wx);
> }
> 
> So the first reopen attempt looks like it runs about one second after
> resume.
> 
> Should wxvf_resume() call wx_service_event_schedule() after setting
> WX_FLAG_NEED_REOPEN, or should the changelog be corrected?



> 
>> @@ -52,13 +75,15 @@ void wxvf_remove(struct pci_dev *pdev)
>> unregister_netdev(netdev);
>> timer_shutdown_sync(&wx->service_timer);
>> cancel_work_sync(&wx->service_task);
>> + clear_bit(WX_STATE_SERVICE_SCHED, wx->state);
>> kfree(wx->vfinfo);
>> kfree(wx->rss_key);
>> kfree(wx->mac_table);
>> wx_clear_interrupt_scheme(wx);
>> pci_release_selected_regions(pdev,
>>     pci_select_bars(pdev, IORESOURCE_MEM));
>> - pci_disable_device(pdev);
>> + if (!test_and_set_bit(WX_STATE_DISABLED, wx->state))
>> + pci_disable_device(pdev);
>> }
> 
> [Severity: High]
> Can wxvf_remove() race with a reopen that is still pending after
> resume?
> 
> After wxvf_resume(), WX_FLAG_NEED_REOPEN is set, service_timer is
> armed, and WX_STATE_RES_FREED is still set from wxvf_suspend(). The VF
> can be unbound before wxvf_reopen_subtask() has completed. That window
> includes the first second after resume, and any period where the PF
> ping or wx_reset_vf() keeps failing and the subtask keeps retrying. In
> that case wxvf_remove() calls unregister_netdev() before it shuts down
> the timer or cancels the work.
> 
> If the interface was up, unregister_netdev() reaches wxvf_close(),
> which returns early because WX_STATE_RES_FREED is already set. If the
> interface was down, wxvf_close() is not called at all. Either way,
> nothing clears WX_FLAG_NEED_REOPEN and nothing sets
> WX_STATE_DISABLED. WX_STATE_DISABLED is only set at the very end of
> wxvf_remove().
> 
> Once unregister_netdev() drops RTNL, the timer can still queue the
> service task. wx_service_event_schedule() now lets it through because
> WX_FLAG_NEED_REOPEN is set. An already queued work can also run while
> wxvf_remove() waits in cancel_work_sync(). The guard in
> wxvf_reopen_subtask() passes, so it proceeds against a netdev that has
> already been unregistered:
> 
>  wxvf_reopen_subtask()
>    wx_mbx_write_and_read_reply()    /* PF mailbox traffic */
>    wx_reset_vf()                    /* VF reset */
>    wx_init_interrupt_scheme()
>      wx_set_interrupt_capability()  /* MSI-X vector allocation */
>      wx_alloc_q_vectors()           /* netif_napi_add() */
>    netif_device_attach()
> 
> wx_clear_interrupt_scheme() later in wxvf_remove() frees those
> q_vectors again. Even so, adding NAPI instances to a netdev after
> unregister_netdevice() has torn it down does not look safe. Neither
> does resetting the VF over the mailbox and allocating IRQ vectors in
> the middle of remove.
> 
> Should wxvf_remove() quiesce the service machinery before
> unregister_netdev()? For example, it could clear WX_FLAG_NEED_REOPEN or
> mark the device as being removed, then call timer_shutdown_sync() and
> cancel_work_sync() first. That is similar to how ixgbevf_remove() sets
> __IXGBEVF_REMOVING and cancels service_task before
> unregister_netdev(). Alternatively, should wxvf_reopen_subtask() refuse
> to run once the netdev is no longer registered?
> 
> [Severity: Low]
> This isn't a regression from this patch, but does the VF teardown path
> leak wx->isb_mem?
> 
> wxvf_open()->wx_setup_resources()->wx_setup_isb_resources() allocates it
> with dma_alloc_coherent(), which is not devm-managed:
> 
> wx->isb_mem = dma_alloc_coherent(&pdev->dev,
> sizeof(u32) * 4,
> &wx->isb_dma,
> GFP_KERNEL);
> 
> wxvf_close() frees only the IRQs and the RX/TX rings. wxvf_remove() never
> calls wx_free_isb_resources(). That function is called only from
> ngbe_main.c and txgbe_main.c.
> 
> wx_setup_isb_resources() returns early when isb_mem is already set, so
> repeated open/resume cycles do not leak more. Even so, the 16-byte
> coherent buffer seems to be left behind on every VF unbind if the
> interface was opened at least once.
> 
> [ ... ]
> 
>> +static void wxvf_reopen_subtask(struct wx *wx)
>> +{
> 
> [ ... ]
> 
>> + if (wx_init_interrupt_scheme(wx)) {
>> + dev_err(&wx->pdev->dev, "Failed to initialize interrupt scheme\n");
>> + goto hard_failure;
>> + }
>> +
>> + if (netif_running(wx->netdev)) {
>> + if (wxvf_open(wx->netdev))
>> + goto open_failed;
>> + }
> 
> [Severity: High]
> This is a pre-existing issue, but this patch adds a new caller that
> relies on it. Can the wxvf_open() error unwind free RX buffers and
> descriptor rings while the hardware queues are still enabled?
> 
> wxvf_open() configures the rings before requesting IRQs:
> 
> wx_vf_common.c:wxvf_open() {
>    ...
> wx_configure_vf(wx);
> 
> err = wx_request_msix_irqs_vf(wx);
> if (err)
> goto err_free_resources;
>    ...
> err_free_irq:
> wx_free_irq(wx);
> err_free_resources:
> wx_free_resources(wx);
> err_reset:
> wx_reset_vf(wx);
> return err;
> }
> 
> wx_configure_vf()->wx_configure_rx_vf()->wx_configure_rx_ring_vf()
> enables the queue and posts buffers:
> 
> rxdctl |= WX_VXRXDCTL_ENABLE | WX_VXRXDCTL_VLAN;
> ...
> wx_enable_rx_queue(wx, ring);
> wx_alloc_rx_buffers(ring, wx_desc_unused(ring));
> 
> Suppose wx_request_msix_irqs_vf() or netif_set_real_num_{tx,rx}_queues()
> fails. Then wx_free_resources()->wx_free_rx_resources() returns the
> pages to the page_pool, calls dma_free_coherent() on the descriptor ring
> and calls page_pool_destroy().
> 
> WX_VXRXDCTL_ENABLE is cleared only after that, through
> wx_reset_vf()->wx_reset_hw_vf()->wx_stop_adapter_vf().
> 
> On this new resume path, wxvf_resume() has already called
> pci_set_master(). A frame that arrives in that window could be DMA'd
> into a recycled page or into freed descriptor memory.
> 
> The open_failed path here does not quiesce the device either.
> wx_clear_interrupt_scheme() only frees the queue vectors. dev_close()
> does nothing because WX_STATE_RES_FREED is still set from suspend, so
> wxvf_close() returns early. pci_disable_device() then runs after the
> frees.
> 
> Should the wxvf_open() unwind stop the hardware queues (wx_reset_vf() or
> wx_stop_adapter_vf()) before wx_free_resources()? That would match the
> order wxvf_close() uses, where wxvf_down() runs before
> wx_free_resources().
> 
>> +
>> + clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
>> + if (!netif_running(wx->netdev))
>> + timer_delete_sync(&wx->service_timer);
>> + netif_device_attach(wx->netdev);
>> + rtnl_unlock();
>> + return;
>> +open_failed:
>> +hard_failure:
> 
> [ ... ]
> 
>> + timer_delete_sync(&wx->service_timer);
>> + wx_clear_interrupt_scheme(wx);
>> + clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
>> + set_bit(WX_STATE_DOWN, wx->state);
>> +
>> + if (netif_running(wx->netdev))
>> + dev_close(wx->netdev);
>> +
>> + rtnl_unlock();
>> + if (!test_and_set_bit(WX_STATE_DISABLED, wx->state))
>> + pci_disable_device(wx->pdev);
>> + dev_err(&wx->pdev->dev, "Failed to fully resume device\n");
>> }
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019060216.38458-1-mengyuanlou%40net-swift.com
> 
>