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

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

[Mengyuan Lou]

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/
- 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/
---
 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();
+	timer_delete_sync(&wx->service_timer);
+	cancel_work_sync(&wx->service_task);
 
 	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);
+	set_bit(WX_STATE_DOWN, wx->state);
+	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

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

[Harshitha Ramamurthy]

On Fri, Sep 4, 2026 at 1:52 AM Mengyuan Lou <mengyuanlou@net-swift.com> wrote:
>
> 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/
> - 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/
> ---
>  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().

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

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

[mengyuanlou]

> 2026年9月5日 09:56，Harshitha Ramamurthy <hramamurthy@google.com> 写道：
> 
> On Fri, Sep 4, 2026 at 1:52 AM Mengyuan Lou <mengyuanlou@net-swift.com> wrote:
>> 
>> 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/
>> - 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/
>> ---
>> 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().
ok

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

Error handling is indeed very important and requires careful consideration.
Thanks.

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider:

Critical: 0 · High: 3 · Medium: 5 · Low: 0

- [High] wxvf_resume() clears WX_STATE_DOWN (wx_vf_common.c:54) and
  returns success while the device has zero q_vectors, zero rings, no…
- [High] wxvf_resume()'s else branch (interface was administratively down
  at suspend time) only does pci_set_master() + netif_device_attach()…
- [High] wxvf_reopen_subtask() tests WX_FLAG_NEED_REOPEN *outside*
  rtnl_lock and then, after acquiring rtnl_lock, unconditionally…
- [Medium] The commit message advertises 'Provide error unwinding inside
  wxvf_reopen_subtask() if re-opening fails', but the unwinding does not…
- [Medium] The VF PM ops report a successful resume before the
  previously-running netdevice has been restored to an operational…
- [Medium] wxvf_suspend() frees the q_vectors
  (wx_clear_interrupt_scheme()) and calls…
- [Medium] wxvf_reopen_subtask() re-programs the VF Tx/Rx rings and
  mailbox-based configuration via wxvf_open() without any wx_reset_vf()…
- [Medium] wxvf_suspend() newly calls
  cancel_work_sync(&wx->service_task), which can cancel a work item that…

--

net: libwx: fix PM suspend/resume flow for VF drivers

This reworks the libwx VF PM callbacks so that wxvf_suspend() closes a
running interface with wxvf_close() before clearing bus mastering and
disabling the PCI device, and wxvf_resume() defers the rebuild to a new
service-task subtask gated by WX_FLAG_NEED_REOPEN.

A few questions about the new lifecycle handling below.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index 26de78e9a69ef..3e55738e2096f 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();
> +	timer_delete_sync(&wx->service_timer);

[Severity: Medium]
Is it safe to free the q_vectors and disable the PCI function before the
service timer and work are quiesced?

wx_service_timer() re-arms and queues the work while the interface is up:

	mod_timer(&wx->service_timer, next_event_offset + jiffies);

	wx_service_event_schedule(wx);

wxvf_link_config_subtask() runs without rtnl, so the rtnl_lock() taken
here does not exclude it, and the new gate added in this patch is a plain
read:

	if (test_bit(WX_STATE_DOWN, wx->state))
		return;

	wxvf_watchdog_update_link(wx);

A worker that passes that test just before wxvf_down() sets the bit
continues into wx_check_mac_link_vf() while suspend is running
wxvf_close() -> wxvf_down() -> wx_reset_vf().  wx_reset_hw_vf() issues its
wx_mbx_write_and_read_reply() without taking wx->mbx.mbx_lock, while
wxvf_watchdog_update_link() does take it, so the two mailbox users are not
mutually excluded against the WX_VXCTRL_RST self-reset and
wx_virt_clr_reg().

Would moving timer_delete_sync()/cancel_work_sync() ahead of
wx_clear_interrupt_scheme() and pci_disable_device() close this window?

> +	cancel_work_sync(&wx->service_task);

[Severity: Medium]
Can WX_STATE_SERVICE_SCHED get stuck here?

cancel_work_sync() on a work item that was queued but has not started
skips the callback, so wx_service_event_complete() - the only place that
clears WX_STATE_SERVICE_SCHED - never runs.  wxvf_resume() clears that bit
only in the netif_running() branch, so if the interface was administratively
down at suspend time the bit survives and wx_service_event_schedule() never
queues work again:

	if (!test_bit(WX_STATE_DOWN, wx->state) &&
	    !test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state))
		queue_work(system_power_efficient_wq, &wx->service_task);

Since wxvf_up_complete() leaves the carrier off and relies on the service
task to report link up, a later "ip link set dev X up" would then never
report carrier.  Should the clear_bit(WX_STATE_SERVICE_SCHED, wx->state)
be unconditional next to the new cancel_work_sync()?

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

[Severity: High]
Should WX_STATE_DOWN be cleared here, a second or more before anything is
re-allocated?

At this point wxvf_suspend() has already run wxvf_close() and
wx_clear_interrupt_scheme(), so there are no q_vectors, no rings, no
requested IRQs and no bus mastering.  The rebuild only happens once
wxvf_reopen_subtask() runs, which cannot be before the service timer
expires.

netif_running() is still true because wxvf_close() was called directly
rather than through dev_close(), so __LINK_STATE_START was never cleared.
An "ip link set dev X down" inside that window reaches
ndo_stop == wxvf_close(), and the idempotency guard in wxvf_down() no
longer short-circuits because the bit was just cleared:

	if (test_and_set_bit(WX_STATE_DOWN, wx->state))
		return;

	timer_delete_sync(&wx->service_timer);

so the whole teardown runs a second time.  Doesn't that mean:

- wx_reset_vf() drives MMIO and mailbox traffic on a function whose bus
  mastering was cleared,

- the timer_delete_sync() above removes the only trigger for the pending
  WX_FLAG_NEED_REOPEN, so the netdev stays netif_device_detach()ed and
  __dev_open() keeps returning -ENODEV for good,

- wx_free_irq() takes its non-MSI-X branch, with wx->misc_irq_domain false
  for VFs and pdev->irq forced to 0 for VFs by pci_read_irq():

	if (!(pdev->msix_enabled)) {
		if (!wx->misc_irq_domain)
			free_irq(pdev->irq, wx);
		return;
	}

  which lands in WARN(1, "Trying to free already-free IRQ %d")?

Would keeping WX_STATE_DOWN set until wxvf_reopen_subtask() has actually
completed avoid all three?

> +	} else {
> +		pci_set_master(pdev);
> +		netif_device_attach(wx->netdev);
> +	}

[Severity: High]
The else branch drops the wx_init_interrupt_scheme() call that the old
unconditional code provided, but wxvf_suspend() still calls
wx_clear_interrupt_scheme() unconditionally.

For a VF, MSI-X is the only mode, so wx_reset_interrupt_capability()
always runs:

	kfree(wx->msix_q_entries);
	wx->msix_q_entries = NULL;
	kfree(wx->msix_entry);
	wx->msix_entry = NULL;

and wx_free_q_vectors() zeroes num_q_vectors, num_tx_queues and
num_rx_queues.  Nothing on this path rebuilds any of that, while
netif_device_attach() makes the next "ip link set dev X up" succeed into
ndo_open == wxvf_open().

With num_q_vectors == 0, does wx_request_msix_irqs_vf() skip its per-queue
loop entirely and then dereference the NULL msix_entry?

	for (vector = 0; vector < wx->num_q_vectors; vector++) {
		...
	}

	err = request_irq(wx->msix_entry->vector, wx_msix_misc_vf,
			  0, netdev->name, wx);

The sibling ngbe_resume() restores the interrupt scheme unconditionally
and only conditionalizes the open:

	err = wx_init_interrupt_scheme(wx);
	if (!err && netif_running(netdev))
		err = ngbe_open(netdev);
	if (!err)
		netif_device_attach(netdev);

> +	rtnl_unlock();
>  	return 0;
>  }
>  EXPORT_SYMBOL(wxvf_resume);

[Severity: Medium]
Returning 0 here reports a completed resume while a previously running
interface still has no rings, no IRQs and no bus mastering for at least
the one second until the service timer fires, so traffic is silently
dropped in that window.

DEFINE_SIMPLE_DEV_PM_OPS in ngbevf_main.c and txgbevf_main.c installs
wxvf_resume() as .resume, .thaw and .restore, and neither pm_ops has a
.complete callback, so nothing forces wxvf_reopen_subtask() to finish
before the PM transition ends and any failure it hits can only be logged
after the PM core has already seen success.

Is deferring the restore preferable to performing it synchronously in the
callback, the way ngbe_resume() does?

[ ... ]

> @@ -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();

[Severity: High]
The flag is tested before rtnl_lock() and nothing is re-validated once the
lock is held.  No path (wxvf_close(), wxvf_suspend(), wxvf_remove()) ever
clears WX_FLAG_NEED_REOPEN, and cancel_work_sync() can only wait for an
already executing work item.

Since dev_close(), wxvf_suspend() and unregister_netdev() all hold rtnl,
can this worker sit blocked in rtnl_lock() across the transition and then
reopen afterwards?

wxvf_suspend()
	rtnl_lock();
	... wxvf_close(), wx_clear_interrupt_scheme(), pci_disable_device()
	rtnl_unlock();
	cancel_work_sync(&wx->service_task);	/* only waits */

wxvf_reopen_subtask()	/* was blocked in rtnl_lock() */
	pci_set_master(wx->pdev);
	wx_init_interrupt_scheme(wx);
	wxvf_open(wx->netdev);

That re-enables bus mastering, MSI-X and DMA on the function that was just
disabled.  The wxvf_remove() case looks worse:

	unregister_netdev(netdev);
	timer_shutdown_sync(&wx->service_timer);
	cancel_work_sync(&wx->service_task);
	...
	wx_clear_interrupt_scheme(wx);

unregister_netdev() holds rtnl while calling ndo_stop, so the blocked
worker reopens after it, and wx_clear_interrupt_scheme() then frees the
q_vectors that the late reopen registered as IRQ dev_id:

	wx->q_vector[v_idx] = NULL;
	netif_napi_del(&q_vector->napi);
	kfree_rcu(q_vector, rcu);

Remove never calls wx_free_irq() or wx_free_resources(), so do the
handlers stay registered against freed q_vectors, with the DMA rings
leaked and the device still bus-mastering?

wx_io_resume() in the same library and ngbe_resume() both re-check
netif_running() under rtnl before reopening.  Would re-testing the flag
and netif_running()/netif_device_present() under rtnl here, plus clearing
WX_FLAG_NEED_REOPEN in close/suspend/remove before dropping rtnl, be
enough?

> +	pci_set_master(wx->pdev);
> +	if (wx_init_interrupt_scheme(wx))
> +		goto out;
> +	if (wxvf_open(wx->netdev))
> +		goto out_clear_scheme;

[Severity: Medium]
Should this path reset the VF before reprogramming it?

The last reset was wxvf_close() -> wxvf_down() -> wx_reset_vf(), which ran
before pci_disable_device() and the low-power transition.  Coming back,
wxvf_open() reprograms the rings and issues mailbox configuration without
redoing the sequence in wx_reset_hw_vf():

	wx->vfinfo->vf_api = wx_mbox_api_null;
	...
	wr32m(wx, WX_VXCTRL, WX_VXCTRL_RST, WX_VXCTRL_RST);
	...
	if (wx->mac.type == wx_mac_aml || wx->mac.type == wx_mac_aml40)
		wr32(wx, WX_VX_PF_BME, WX_VF_BME_ENABLE);
	...
	wx_virt_clr_reg(wx);
	mbx->timeout = 2000;
	ret = wx_mbx_write_and_read_reply(wx, msgbuf, msgbuf, ARRAY_SIZE(msgbuf));

So the WX_VF_RESET handshake is not redone, the API version is not
renegotiated, and on aml/aml40 parts WX_VX_PF_BME is not re-armed.  Since
wx_set_rx_mode_vf() ignores its return values and wx_configure_rx_vf()
only wx_dbg()s an RLPML failure, would silently NACKed configuration go
unnoticed here?

ngbe_resume() calls ngbe_reset_hw() before wx_init_interrupt_scheme() and
ngbe_open(), and ixgbevf_resume() calls ixgbevf_reset() before
ixgbevf_init_interrupt_scheme()/ixgbevf_open().

> +	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);
> +	set_bit(WX_STATE_DOWN, wx->state);
> +	rtnl_unlock();
> +	dev_err(&wx->pdev->dev, "Failed to reopen device\n");
> +}

[Severity: Medium]
The commit message says "Provide error unwinding inside
wxvf_reopen_subtask() if re-opening fails".  Does this unwinding leave a
recoverable device?

netif_device_attach() is only reached on the success path, and
wxvf_suspend() already did netif_device_detach(), so __LINK_STATE_PRESENT
stays clear and __dev_open() returns -ENODEV:

	if (!netif_device_present(dev)) {
		if (dev->dev.parent)
			pm_runtime_resume(dev->dev.parent);
		if (!netif_device_present(dev))
			return -ENODEV;
	}

WX_FLAG_NEED_REOPEN is cleared here, and setting WX_STATE_DOWN also stops
wx_service_event_schedule() from ever queueing the service task again:

	if (!test_bit(WX_STATE_DOWN, wx->state) &&
	    !test_and_set_bit(WX_STATE_SERVICE_SCHED, wx->state))
		queue_work(system_power_efficient_wq, &wx->service_task);

so nothing retries, and wxvf_resume() already returned 0 so the PM core
never learns about the failure.  After one transient allocation or
request_irq() failure at resume, is the interface unusable until driver
unbind/rebind?  netif_running() also stays true with all rings, IRQs and
q_vectors freed, which feeds back into the wx_free_irq() concern noted
above.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904085216.43469-1-mengyuanlou%40net-swift.com