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

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

[Mengyuan Lou]

The shared PM callbacks used by txgbevf and ngbevf free queue vectors and
disable PCI without first stopping a running VF or draining service
activity. Resume does not re-enable PCI or restore the queues and IRQs of
a running VF. Service work can therefore access torn-down resources, and a
running interface is not restored correctly after resume.

Stop a running VF under RTNL before freeing its interrupt resources,
preserving its administrative running state. Drain the service timer and
work before disabling PCI.

Re-enable PCI on resume and defer PF readiness checks, VF reset and
interrupt setup to the service task. Retry readiness and reset failures
while keeping the device detached. Reopen the interface only if it is
still administratively running. If interrupt setup or reopening fails,
stop recovery, close the interface if necessary, and leave the device
detached with PCI disabled.

Track freed resources and PCI enable state to avoid duplicate teardown.
Reject ring-parameter changes while queue vectors are unavailable.

When backporting to kernels that do not have e73e4d187a1f52 ("net:
wangxun: add pcie error handler"), add WX_STATE_DISABLED and
WX_STATE_RES_FREED to enum wx_state before WX_STATE_NBITS. The PCIe
error-handler implementation from that commit is not needed.

Fixes: 377d180bd71c ("net: wangxun: add txgbevf build")
Cc: stable@vger.kernel.org
Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
Changelogs:
v6:
- Clarify the enum dependency for stable backports to kernels without the
  PCIe error-handler commit.
- Keep API negotiation and MAC refresh after reset errors for existing
  callers, while returning the reset error to the resume retry path.
v5:
- Schedule the reopen subtask immediately on resume; keep the service timer
  to retry if the PF is not ready.
- Serialize the PF mailbox probe with mbx_lock, send a firmware-version
  request, and propagate VF reset failures.
- Serialize link updates with RTNL and skip them while reopening is pending.
- Track PCI and resource state to avoid duplicate teardown. Use dev_close()
  on reopen failure and reject ring-parameter changes without queue vectors.
- Retarget the patch to net and add Fixes and Cc: stable tags.
v4: https://lore.kernel.org/netdev/20260908095801.61346-1-mengyuanlou@net-swift.com/
- Refactor patch description and subject to focus on PM flow rework for
  net-next instead of bug fixes, removing the Fixes tag.
- Unconditionally restore the interrupt scheme in wxvf_reopen_subtask()
  to fix NULL msix_entry dereference on previously admin-DOWN interfaces.
- Add WX_FLAG_NEED_REOPEN guard in wxvf_link_config_subtask() to block
  mailbox and register operations before reopening completes.
- Validate netif_running() under rtnl_lock() in wxvf_reopen_subtask()
  before invoking wxvf_open() to respect administrative DOWN events
  during deferral.
- Synchronize core netdev flags (__LINK_STATE_START and IFF_UP) and
  trigger DOWN notifications on wxvf_reopen_subtask() failure paths.
- Flush the service timer and work synchronously during suspend to avoid
  tasks running after PCI device teardown.
v3: https://lore.kernel.org/netdev/20260904085216.43469-1-mengyuanlou@net-swift.com/
- Fix DMA-after-free and UAF races in wxvf_suspend() by invoking
  wxvf_close() when running, ensuring DMA engines are stopped and
  NAPI/IRQs/rings are cleanly freed before clearing PCI state.
- Replace manual resource allocation in wxvf_resume() with deferred
  reopening via WX_FLAG_NEED_REOPEN, scheduling wxvf_reopen_subtask() to
  run full wxvf_open() sequence under rtnl_lock().
- Move timer/work synchronization after rtnl_lock() release in
  wxvf_suspend() to prevent service tasks from re-arming the timer.
- Add error unwinding in wxvf_reopen_subtask() to set WX_STATE_DOWN,
  clear bus mastering, and log errors if reopen fails.
- Skip wxvf_link_config_subtask() when WX_STATE_DOWN is set to prevent
  link updates on downed interfaces.
v2: https://lore.kernel.org/netdev/20260829091423.83097-1-mengyuanlou@net-swift.com/
- Refactored the suspend and resume logic to eliminate full netdevice
  close/open cycles in favor of lightweight interrupt and queue manipulation:
  * In wxvf_suspend(), replaced wxvf_close() with explicit queue stopping
    (netif_tx_disable), carrier drop, NAPI disabling (wx_napi_disable_all), IRQ
    releasing (wx_free_irq), and resource freeing (wx_free_resources).
  * In wxvf_resume(), replaced wxvf_open() with granular resource allocation
    (wx_setup_resources), MSI-X IRQ requesting (wx_request_msix_irqs_vf),
    and deferred hardware reconfiguration via WX_FLAG_NEED_DO_RESET flag.
- Dropped the addition of device_link_add() to parent PF in ngbevf and txgbevf
  probe() paths to keep the patch focused strictly on libwx PM suspend/resume flow.
v1: https://lore.kernel.org/netdev/20260826095243.16939-1-mengyuanlou@net-swift.com/
---
 .../net/ethernet/wangxun/libwx/wx_ethtool.c   |   3 +
 drivers/net/ethernet/wangxun/libwx/wx_lib.c   |   3 +-
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |   1 +
 drivers/net/ethernet/wangxun/libwx/wx_vf.c    |   4 +-
 drivers/net/ethernet/wangxun/libwx/wx_vf.h    |   2 +
 .../net/ethernet/wangxun/libwx/wx_vf_common.c | 125 ++++++++++++++++--
 .../net/ethernet/wangxun/libwx/wx_vf_common.h |   2 +-
 7 files changed, 128 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 940d2e59876c..729b1ec65bd7 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -790,6 +790,9 @@ static int wx_set_ringparam_vf(struct net_device *netdev,
 	struct wx_ring *temp_ring;
 	int i, err = 0;
 
+	if (!wx->num_q_vectors)
+		return -EBUSY;
+
 	new_tx_count = clamp_t(u32, ring->tx_pending, WX_MIN_TXD, WX_MAX_TXD);
 	new_tx_count = ALIGN(new_tx_count, WX_REQ_TX_DESCRIPTOR_MULTIPLE);
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
index dcbf5811046e..701d239ab13c 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
@@ -3402,7 +3402,8 @@ EXPORT_SYMBOL(wx_set_ring);
 
 void wx_service_event_schedule(struct wx *wx)
 {
-	if (!test_bit(WX_STATE_DOWN, wx->state) &&
+	if ((!test_bit(WX_STATE_DOWN, wx->state) ||
+	     test_bit(WX_FLAG_NEED_REOPEN, wx->flags)) &&
 	    !test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state))
 		queue_work(system_power_efficient_wq, &wx->service_task);
 }
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index afd980dbb793..d8551923cdea 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1288,6 +1288,7 @@ enum wx_pf_flags {
 	WX_FLAG_NEED_MODULE_RESET,
 	WX_FLAG_NEED_UPDATE_LINK,
 	WX_FLAG_NEED_DO_RESET,
+	WX_FLAG_NEED_REOPEN,
 	WX_FLAG_RX_MERGE_ENABLED,
 	WX_FLAG_TXHEAD_WB_ENABLED,
 	WX_FLAG_NEED_PCIE_RECOVERY,
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf.c b/drivers/net/ethernet/wangxun/libwx/wx_vf.c
index 7567216a005f..213d13d26070 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf.c
@@ -38,8 +38,8 @@ void wx_init_hw_vf(struct wx *wx)
 }
 EXPORT_SYMBOL(wx_init_hw_vf);
 
-static int wx_mbx_write_and_read_reply(struct wx *wx, u32 *req_buf,
-				       u32 *resp_buf, u16 size)
+int wx_mbx_write_and_read_reply(struct wx *wx, u32 *req_buf,
+				u32 *resp_buf, u16 size)
 {
 	int ret;
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf.h b/drivers/net/ethernet/wangxun/libwx/wx_vf.h
index b64a4de089f2..e8001d5856a9 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf.h
@@ -109,6 +109,8 @@ struct wx_link_reg_fields {
 };
 
 void wx_init_hw_vf(struct wx *wx);
+int wx_mbx_write_and_read_reply(struct wx *wx, u32 *req_buf,
+				u32 *resp_buf, u16 size);
 int wx_reset_hw_vf(struct wx *wx);
 void wx_get_mac_addr_vf(struct wx *wx, u8 *mac_addr);
 void wx_stop_adapter_vf(struct wx *wx);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 26de78e9a69e..e2271e8bffcd 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -16,9 +16,23 @@ int wxvf_suspend(struct device *dev_d)
 	struct pci_dev *pdev = to_pci_dev(dev_d);
 	struct wx *wx = pci_get_drvdata(pdev);
 
+	rtnl_lock();
 	netif_device_detach(wx->netdev);
+	if (netif_running(wx->netdev))
+		wxvf_close(wx->netdev);
+
+	clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
 	wx_clear_interrupt_scheme(wx);
-	pci_disable_device(pdev);
+	set_bit(WX_STATE_RES_FREED, wx->state);
+	rtnl_unlock();
+
+	timer_delete_sync(&wx->service_timer);
+	cancel_work_sync(&wx->service_task);
+	clear_bit(WX_STATE_SERVICE_SCHED, wx->state);
+
+	pci_clear_master(pdev);
+	if (!test_and_set_bit(WX_STATE_DISABLED, wx->state))
+		pci_disable_device(pdev);
 
 	return 0;
 }
@@ -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;
 }
 EXPORT_SYMBOL(wxvf_resume);
@@ -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);
 }
 EXPORT_SYMBOL(wxvf_remove);
 
@@ -137,7 +162,7 @@ void wx_negotiate_api_vf(struct wx *wx)
 }
 EXPORT_SYMBOL(wx_negotiate_api_vf);
 
-void wx_reset_vf(struct wx *wx)
+int wx_reset_vf(struct wx *wx)
 {
 	struct net_device *netdev = wx->netdev;
 	int ret = 0;
@@ -150,6 +175,8 @@ void wx_reset_vf(struct wx *wx)
 		eth_hw_addr_set(netdev, wx->mac.addr);
 		ether_addr_copy(netdev->perm_addr, wx->mac.addr);
 	}
+
+	return ret;
 }
 EXPORT_SYMBOL(wx_reset_vf);
 
@@ -279,6 +306,7 @@ void wxvf_up_complete(struct wx *wx)
 	wx_configure_msix_vf(wx);
 	smp_mb__before_atomic();
 	clear_bit(WX_STATE_DOWN, wx->state);
+	clear_bit(WX_STATE_RES_FREED, wx->state);
 	wx_napi_enable_all(wx);
 
 	/* clear any pending interrupts, may auto mask */
@@ -376,6 +404,9 @@ int wxvf_close(struct net_device *netdev)
 {
 	struct wx *wx = netdev_priv(netdev);
 
+	if (test_and_set_bit(WX_STATE_RES_FREED, wx->state))
+		return 0;
+
 	wxvf_down(wx);
 	wx_free_irq(wx);
 	wx_free_resources(wx);
@@ -388,25 +419,103 @@ static void wxvf_link_config_subtask(struct wx *wx)
 {
 	struct net_device *netdev = wx->netdev;
 
+	rtnl_lock();
+	if (test_bit(WX_STATE_DOWN, wx->state) ||
+	    test_bit(WX_FLAG_NEED_REOPEN, wx->flags))
+		goto out;
+
 	wxvf_watchdog_update_link(wx);
 	if (wx->link) {
 		if (netif_carrier_ok(netdev))
-			return;
+			goto out;
 		netif_carrier_on(netdev);
 		netdev_info(netdev, "Link is Up - %s\n",
 			    phy_speed_to_str(wx->speed));
 	} else {
 		if (!netif_carrier_ok(netdev))
-			return;
+			goto out;
 		netif_carrier_off(netdev);
 		netdev_info(netdev, "Link is Down\n");
 	}
+
+out:
+	rtnl_unlock();
+}
+
+static void wxvf_reopen_subtask(struct wx *wx)
+{
+	u32 ping[2] = { WX_VF_GET_FW_VERSION };
+	int ret;
+
+	if (!test_bit(WX_FLAG_NEED_REOPEN, wx->flags))
+		return;
+
+	rtnl_lock();
+	if (!test_bit(WX_FLAG_NEED_REOPEN, wx->flags) ||
+	    test_bit(WX_STATE_DISABLED, wx->state)) {
+		rtnl_unlock();
+		return;
+	}
+
+	/* Serialize the ping with other VF mailbox accesses.
+	 * The helper sends the request and waits for the PF reply.
+	 */
+	spin_lock_bh(&wx->mbx.mbx_lock);
+	ret = wx_mbx_write_and_read_reply(wx, ping, ping, ARRAY_SIZE(ping));
+	spin_unlock_bh(&wx->mbx.mbx_lock);
+	if (ret) {
+		rtnl_unlock();
+		return;
+	}
+
+	ret = wx_reset_vf(wx);
+	if (ret) {
+		rtnl_unlock();
+		return;
+	}
+
+	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;
+	}
+
+	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:
+	/* Do not retry interrupt setup or reopen from the service task. Keep the
+	 * netdev detached and down, and disable PCI.
+	 * Retry by rebinding the driver or through a later PM resume; bring the
+	 * interface up after recovery.
+	 */
+	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");
 }
 
 static void wxvf_service_task(struct work_struct *work)
 {
 	struct wx *wx = container_of(work, struct wx, service_task);
 
+	wxvf_reopen_subtask(wx);
 	wxvf_link_config_subtask(wx);
 	wxvf_reset_subtask(wx);
 	wx_service_event_complete(wx);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
index d45d5d8ac3ab..5bfeab6fdde6 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.h
@@ -10,7 +10,7 @@ int wxvf_resume(struct device *dev_d);
 void wxvf_remove(struct pci_dev *pdev);
 int wx_request_msix_irqs_vf(struct wx *wx);
 void wx_negotiate_api_vf(struct wx *wx);
-void wx_reset_vf(struct wx *wx);
+int wx_reset_vf(struct wx *wx);
 void wx_set_rx_mode_vf(struct net_device *netdev);
 void wx_configure_vf(struct wx *wx);
 int wx_set_mac_vf(struct net_device *netdev, void *p);
-- 
2.30.1

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

[netdev-bot+sinfo]

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.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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.

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

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

[mengyuanlou]

> 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
> 
> 

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

[Simon Horman]

On Mon, Oct 26, 2026 at 02:59:59PM +0800, mengyuanlou@net-swift.com wrote:
> 
> 
> > 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.

...

> > [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;
> >> + }

Hi Mengyuan,

I apologise if I am missing something obvious.
But could you respond to the High concern above?

...

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

[mengyuanlou]

> 2026年10月26日 23:16，Simon Horman <horms@kernel.org> 写道：
> 
> On Mon, Oct 26, 2026 at 02:59:59PM +0800, mengyuanlou@net-swift.com wrote:
>> 
>> 
>>> 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.
> 
> ...
> 
>>> [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?


The VF may resume before its PF, and the mailbox is not usable until the PF has resumed.
So resume only sets WX_FLAG_NEED_REOPEN and defers the reopen to wxvf_reopen_subtask(),
which retries over the mailbox until the PF is ready.

It's right about that WX_STATE_DISABLED is only set at the end of wxvf_remove(),
after unregister_netdev() has returned, so a pending (or newly queued) service
work could pass that guard and touch the netdev while it is being unregistered.
That window was real. 

V7 will add check: the netdev to still be registered

 	rtnl_lock();
 	if (!test_bit(WX_FLAG_NEED_REOPEN, wx->flags) ||
 	    test_bit(WX_STATE_DISABLED, wx->state) ||
 +++	    wx->netdev->reg_state != NETREG_REGISTERED) {
 		rtnl_unlock();
 		return;
 	}
wxvf_reopen_subtask() and unregister_netdev() serialize on the same RTNL, and reg_state
leaves NETREG_REGISTERED before that lock is dropped.

Thanks,
Mengyuan



>>> 
>>> [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;
>>>> + }
> 
> Hi Mengyuan,
> 
> I apologise if I am missing something obvious.
> But could you respond to the High concern above?
> 
> ...