Thread (4 messages) 4 messages, 3 authors, 19d ago

Re: [PATCH net v3] net: libwx: fix PM suspend/resume flow for VF drivers

From: Harshitha Ramamurthy <hramamurthy@google.com>
Date: 2026-09-05 01:57:11

On Fri, Sep 4, 2026 at 1:52 AM Mengyuan Lou [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Rework wxvf_suspend() and wxvf_resume() to handle interface lifecycle
transitions safely under rtnl_lock() and eliminate resource races.

In wxvf_suspend(), if the netdevice is running, invoke wxvf_close() to
quiesce DMA engines, disable NAPI, and release IRQs/rings cleanly prior
to clearing PCI bus mastering and disabling the device.

In wxvf_resume(), avoid partial hardware re-initialization on active
queues. If the device was running before suspend, defer complete queue
and interrupt setup to a new reset subtask flag WX_FLAG_NEED_REOPEN,
which safely invokes wxvf_open() under rtnl_lock(). Provide error
unwinding inside wxvf_reopen_subtask() if re-opening fails.

Fixes: 377d180bd71c ("net: wangxun: add txgbevf build")
Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>
---
Changelogs:
v3:
- 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/ (local)
- 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/ (local)
---
 drivers/net/ethernet/wangxun/libwx/wx_type.h  |  1 +
 .../net/ethernet/wangxun/libwx/wx_vf_common.c | 57 ++++++++++++++++++-
 2 files changed, 55 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 2eba5ab59925..1b18b069decc 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_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
index 26de78e9a69e..3e55738e2096 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
@@ -16,9 +16,16 @@ 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);
        wx_clear_interrupt_scheme(wx);
+       pci_clear_master(pdev);
        pci_disable_device(pdev);
+       rtnl_unlock();
Is the rtnl_lock() protection needed around pci_clear_master() and
pci_disable_device()?
+       timer_delete_sync(&wx->service_timer);
+       cancel_work_sync(&wx->service_task);
These should probably be moved up before
pci_clear_master/pci_disable_device and after the rtnl_unlock().
quoted hunk ↗ jump to hunk
        return 0;
 }
@@ -34,11 +41,25 @@ 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;
+       }

+       rtnl_lock();
+       if (netif_running(wx->netdev)) {
+               clear_bit(WX_STATE_DOWN, wx->state);
+               clear_bit(WX_STATE_SERVICE_SCHED, wx->state);
+               mod_timer(&wx->service_timer, jiffies + HZ);
+               set_bit(WX_FLAG_NEED_REOPEN, wx->flags);
+       } else {
+               pci_set_master(pdev);
+               netif_device_attach(wx->netdev);
+       }
+       rtnl_unlock();
        return 0;
 }
 EXPORT_SYMBOL(wxvf_resume);
@@ -388,6 +409,9 @@ static void wxvf_link_config_subtask(struct wx *wx)
 {
        struct net_device *netdev = wx->netdev;

+       if (test_bit(WX_STATE_DOWN, wx->state))
+               return;
+
        wxvf_watchdog_update_link(wx);
        if (wx->link) {
                if (netif_carrier_ok(netdev))
@@ -403,10 +427,37 @@ static void wxvf_link_config_subtask(struct wx *wx)
        }
 }

+static void wxvf_reopen_subtask(struct wx *wx)
+{
+       if (!test_bit(WX_FLAG_NEED_REOPEN, wx->flags))
+               return;
+
+       rtnl_lock();
+       pci_set_master(wx->pdev);
+       if (wx_init_interrupt_scheme(wx))
+               goto out;
+       if (wxvf_open(wx->netdev))
+               goto out_clear_scheme;
+       clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
+       netif_device_attach(wx->netdev);
+       rtnl_unlock();
+       return;
+
+out_clear_scheme:
+       wx_clear_interrupt_scheme(wx);
+out:
+       pci_clear_master(wx->pdev);
+       clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
By clearing this on the error path, the VF tries once to establish
communication with the PF. What if the PF is not back up by that time?
+       set_bit(WX_STATE_DOWN, wx->state);
And by setting this, the service_task which calls
wxvf_reopen_subtask() won't run again. So essentially on a 1 time
failure of the VF trying to talk to the PF, the VF gives up. I think
it is better for the VF to retry continuously(or at least a maximum
amount of times) to talk to the PF before giving up.

Also, it probably would be better to split up the patch into:
- rtnl_lock() protections
- implement a proper retry mechanism for establishing VF-PF communication.
+       rtnl_unlock();
+       dev_err(&wx->pdev->dev, "Failed to reopen 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);
--
2.30.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help