* Re: [PATCH net-next v4] net: libwx: rework VF PM suspend/resume flow
2026-09-08 9:58 [PATCH net-next v4] net: libwx: rework VF PM suspend/resume flow Mengyuan Lou
@ 2026-09-11 9:58 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-11 9:58 UTC (permalink / raw)
To: mengyuanlou
Cc: netdev, jiawenwu, duanqiangwen, linglingzhang, andrew+netdev,
davem, edumazet, kuba, pabeni, hramamurthy
Thank you for your contribution! Sashiko AI review found 10 potential
issue(s) to consider:
Critical: 0 · High: 4 · Medium: 5 · Low: 1
- [High] wxvf_suspend() calls wxvf_close() (the ndo_stop body) directly
and then wx_clear_interrupt_scheme(), but never clears the core netdev…
- [High] The open_failed/hard_failure path of wxvf_reopen_subtask() hand-
rolls the netdev down transition: it fires…
- [High] wxvf_suspend() now calls cancel_work_sync(&wx->service_task)
after timer_delete_sync().
- [High] On the hard_failure path of wxvf_reopen_subtask()
(wx_init_interrupt_scheme() returned an error),…
- [Medium] The commit message presents the change as a cosmetic "rework
... to normalize VF netdevice lifecycle handling", but the patch…
- [Medium] wxvf_reopen_subtask() gates the whole reopen on 'if
(wx_read_mbx_vf(wx, &msgbuf, 1))' with the comment "try get mbx lock…
- [Medium] The open_failed/hard_failure path of wxvf_reopen_subtask()
calls pci_disable_device(wx->pdev) without recording that the device…
- [Medium] wxvf_suspend() performs the resource teardown (wxvf_close() +
wx_clear_interrupt_scheme()) under rtnl and only quiesces the service…
- [Medium] The reworked resume path (wxvf_resume() ->
wxvf_reopen_subtask()) restores only software state:…
- [Low] wxvf_reopen_subtask() calls wx_read_mbx_vf() without holding
wx->mbx.mbx_lock, unlike every other mailbox call site in the VF…
--
net: libwx: rework VF PM suspend/resume flow
This reworks wxvf_suspend()/wxvf_resume() so that the VF netdevice is
closed under rtnl_lock during suspend and the queue/interrupt bring-up is
deferred to a new wxvf_reopen_subtask() driven by WX_FLAG_NEED_REOPEN.
Some questions on the new flow below.
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_lib.c b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> index ed5aad7857bd9..de09a6704477a 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_lib.c
> @@ -3381,7 +3381,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_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index 26de78e9a69ef..7d52da8e6af14 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> @@ -16,8 +16,18 @@ 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)) {
> + set_bit(WX_STATE_WAS_RUNNING, wx->state);
> + wxvf_close(wx->netdev);
> + }
> wx_clear_interrupt_scheme(wx);
> + rtnl_unlock();
[Severity: High]
Calling wxvf_close() directly here runs the ndo_stop body but leaves the
core netdev state untouched, so __LINK_STATE_START and IFF_UP stay set
while wx_clear_interrupt_scheme() frees wx->msix_entry,
wx->msix_q_entries and wx->q_vector[] and zeroes num_tx_queues /
num_rx_queues / num_q_vectors.
Since wxvf_resume() restores nothing and only arms the timer, that state
is live for at least a second after userspace is thawed. If an admin
down arrives in that window, __dev_close_many() calls ndo_stop
unconditionally for any IFF_UP device, so wxvf_close() runs a second
time:
wxvf_close()
wxvf_down() /* early return, WX_STATE_DOWN already set */
wx_free_irq(wx)
and in wx_free_irq() pdev->msix_enabled is now 0 while
wx->misc_irq_domain is false for VFs (only txgbe_setup_misc_irq() sets
it), so it reaches:
if (!(pdev->msix_enabled)) {
if (!wx->misc_irq_domain)
free_irq(pdev->irq, wx);
return;
}
free_irq() on an IRQ that was never requested gives
WARN(1, "Trying to free already-free IRQ %d"). Would a
WX_STATE_RES_FREED style guard in the VF close path, like the one
ngbe_close()/txgbe_close() use for exactly this suspend-freed-resources
case, be appropriate here?
The same stale netif_running() also sends wx_set_ringparam_vf() down its
"running" branch, where wxvf_up_complete() clears WX_STATE_DOWN and
enables queues and interrupts on a device that owns no vectors and no
rings.
[Severity: Medium]
Can a service task already inside wxvf_reopen_subtask() and blocked on
rtnl_lock() re-allocate what was just freed? The teardown happens under
rtnl, and the work is only quiesced after rtnl_unlock(), and
cancel_work_sync() waits for a running work item rather than aborting
it. The blocked work would then proceed with WX_FLAG_NEED_REOPEN still
set and run wx_init_interrupt_scheme() (pci_alloc_irq_vectors() plus the
msix_entry/msix_q_entries/q_vector allocations) and, with
WX_STATE_WAS_RUNNING plus the still-true netif_running(), wxvf_open()
(ring DMA, request_irq(), and wxvf_up_complete() re-arming the service
timer and clearing WX_STATE_DOWN) immediately before
pci_clear_master()/pci_disable_device().
WX_FLAG_NEED_REOPEN is cleared by that run, so the next resume restores
nothing, and the following wx_init_interrupt_scheme() overwrites the
already-allocated msix/q_vector pointers. DEFINE_SIMPLE_DEV_PM_OPS maps
freeze/thaw/poweroff to the same pair, so hibernation performs
suspend -> resume -> suspend and can reach this with the flag still set.
> +
> + timer_delete_sync(&wx->service_timer);
> + cancel_work_sync(&wx->service_task);
[Severity: High]
Can this leave WX_STATE_SERVICE_SCHED set forever? wx_service_timer()
runs in softirq and wx_service_event_schedule() sets the bit before
queue_work():
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);
The only runtime clearing site is wx_service_event_complete() at the end
of wxvf_service_task(). If the timer queued the work just before
cancel_work_sync() runs, the pending item is dequeued without ever
executing, so the bit is never cleared and every later
test_and_set_bit() fails. The new WX_FLAG_NEED_REOPEN term only bypasses
the WX_STATE_DOWN test, not the SCHED gate.
Because wxvf_resume() no longer does wx_init_interrupt_scheme() and
netif_device_attach() itself, the VF then stays detached with no
interrupt scheme and no rings until the driver is unbound and rebound.
Should the suspend path clear WX_STATE_SERVICE_SCHED explicitly after
cancel_work_sync()?
> + pci_clear_master(pdev);
> pci_disable_device(pdev);
>
> return 0;
> @@ -34,11 +44,19 @@ 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;
> + }
[Severity: Medium]
The changelog describes this as a rework "to normalize VF netdevice
lifecycle handling during power management transitions", but two of the
changes look like fixes for user-visible defects that the message does
not mention and that carry no Fixes: tag.
The old wxvf_resume() did only pci_set_master() plus
wx_init_interrupt_scheme() and never re-enabled the device that suspend
had disabled, so pdev->enable_cnt went 1 -> 0 on the first suspend and
negative on the second cycle, tripping the "disabling already-disabled
device" warning in pci_disable_device(). The new
pci_enable_device_mem() here fixes that.
Similarly, the old wxvf_suspend() ran wx_clear_interrupt_scheme() and
pci_disable_device() while a running interface still had live DMA; the
new wxvf_close() and pci_clear_master() address that.
Both callbacks are live through DEFINE_SIMPLE_DEV_PM_OPS(ngbevf_pm_ops,
wxvf_suspend, wxvf_resume) and the equivalent txgbevf registration.
Could the two fixes be described and tagged so stable backports pick
them up?
>
> + 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);
[ ... ]
> @@ -403,10 +425,69 @@ static void wxvf_link_config_subtask(struct wx *wx)
> }
> }
>
> +static void wxvf_reopen_subtask(struct wx *wx)
> +{
> + u32 msgbuf = 0;
> +
> + if (!test_bit(WX_FLAG_NEED_REOPEN, wx->flags))
> + return;
> +
> + rtnl_lock();
> + /* try get mbx lock to make sure we can communicate with the PF */
> + if (wx_read_mbx_vf(wx, &msgbuf, 1)) {
> + rtnl_unlock();
> + return;
> + }
[Severity: Medium]
Does this actually test that the PF is reachable? wx_read_mbx_vf() takes
only the VF's own VFU bit through wx_obtain_mbx_lock_vf() ->
wx_mailbox_get_lock_vf(), which writes WX_VXMAILBOX_VFU and polls the
same bit back with no PF transaction, then drains WX_VXMBMEM and writes
WX_VXMAILBOX_ACK:
ret = wx_obtain_mbx_lock_vf(wx);
if (ret)
return ret;
for (i = 0; i < size; i++)
msg[i] = rd32a(wx, WX_VXMBMEM, i);
wr32(wx, WX_VXMAILBOX, WX_VXMAILBOX_ACK);
So the gate passes regardless of PF state, and it consumes and ACKs
whatever PF to VF message happens to be pending. The only other reader,
wx_check_mac_link_vf(), deliberately checks first:
wx_check_for_rst_vf(wx);
if (!wx_check_for_msg_vf(wx))
ret = wx_read_mbx_vf(wx, msgbuf, 2);
and dispatches WX_PF_CONTROL_MSG / WX_PF_NOFITY_VF_LINK_STATUS. A PF
ping or link-status notification landing in this window is discarded
along with the WX_FLAG_NEED_DO_RESET it would have set.
A non-decoding device also reads 0xFFFFFFFF, which has VFU set, so a
failed MMIO read looks like a successful lock, and wx_read_v2p_mailbox()
latches those all-ones R2C bits into wx->mbx.mailbox as pending-message
and PF-reset indications.
[Severity: Low]
This isn't a bug today, but this is the only mailbox access in the VF
driver taken without spin_lock_bh(&wx->mbx.mbx_lock).
wx_negotiate_api_vf(), wx_set_rx_mode_vf(), wx_configure_rx_vf(),
wx_set_mac_vf() and wxvf_watchdog_update_link() in this same work item
all wrap the helper, for example:
spin_lock_bh(&wx->mbx.mbx_lock);
err = wx_check_mac_link_vf(wx);
spin_unlock_bh(&wx->mbx.mbx_lock);
Today the other users are rtnl-serialized (neither ngbevf_netdev_ops nor
txgbevf_netdev_ops registers .ndo_set_rx_mode), so no concrete race
exists, but a future non-rtnl mailbox caller would silently race with
this one. Would taking mbx_lock here keep the convention intact?
> +
> + if (wx_init_interrupt_scheme(wx)) {
> + dev_err(&wx->pdev->dev, "Failed to initialize interrupt scheme\n");
> + goto hard_failure;
> + }
> +
> + if (test_bit(WX_STATE_WAS_RUNNING, wx->state)) {
> + if (!netif_running(wx->netdev))
> + goto finish_success;
> +
> + if (wxvf_open(wx->netdev))
> + goto open_failed;
> + }
[Severity: Medium]
Does the new bring-up sequence need the VF hardware reset and PF
handshake? This path restores only software state
(pci_enable_device_mem(), pci_set_master(), wx_init_interrupt_scheme(),
wxvf_open()) and never calls wx_reset_vf() -> wx_reset_hw_vf() /
wx_init_hw_vf() / wx_negotiate_api_vf().
wx_reset_hw_vf() is the only place that asserts WX_VXCTRL_RST, re-applies
the VXRXDCTL defaults, invalidates the negotiated API:
/* reset the api version */
wx->vfinfo->vf_api = wx_mbox_api_null;
sends WX_VF_RESET so the PF re-establishes clear_to_send, RAR and
mc_filter_type, and is the only writer in the tree of the device-level
bus-master enable:
/* amlite: bme */
if (wx->mac.type == wx_mac_aml || wx->mac.type == wx_mac_aml40)
wr32(wx, WX_VX_PF_BME, WX_VF_BME_ENABLE);
pci_set_master() only sets the config-space bit, not that register. After
a D3cold power cycle, would wxvf_open() then enable the descriptor
engines on default hardware while the driver still believes the VF is
configured and API-negotiated, with the PF-mediated calls in
wx_configure_vf() (wx_set_rar_vf(), wx_update_xcast_mode_vf(),
wx_rlpml_set_vf()) failing silently?
> +
> +finish_success:
> + clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
> + clear_bit(WX_STATE_WAS_RUNNING, wx->state);
> + netif_device_attach(wx->netdev);
> + rtnl_unlock();
> + return;
> +open_failed:
> + wx_clear_interrupt_scheme(wx);
> +hard_failure:
> + clear_bit(WX_FLAG_NEED_REOPEN, wx->flags);
> + clear_bit(WX_STATE_WAS_RUNNING, wx->state);
> + set_bit(WX_STATE_DOWN, wx->state);
[Severity: High]
On the hard_failure path, wx->num_tx_queues and num_rx_queues keep the
values programmed by wx->set_num_queues() while every wx->tx_ring[] and
rx_ring[] entry is still NULL from the suspend-time wx_free_q_vector().
In wx_init_interrupt_scheme() the counts are set first and the
wx_set_interrupt_capability() failure returns immediately:
ret = wx_set_interrupt_capability(wx);
if (ret) {
wx_err(wx, "Allocate irq vectors for failed.\n");
return ret;
}
only the wx_alloc_q_vectors() failure path zeroes the counts. The netdev
stays registered and !netif_running(), so wx_set_ringparam_vf() takes:
for (i = 0; i < wx->num_tx_queues; i++)
wx->tx_ring[i]->count = new_tx_count;
Can an ethtool -G on such a VF dereference NULL here? Should this path
also reset the queue counts?
> +
> + if (netif_running(wx->netdev)) {
> + struct net_device *netdev = wx->netdev;
> +
> + /* Can't use dev_close() here because wxvf_close() was
> + * already called during suspend. Manually clear state
> + * and send notifications instead.
> + */
> + call_netdevice_notifiers(NETDEV_GOING_DOWN, netdev);
> + call_netdevice_notifiers(NETDEV_DOWN, netdev);
> + netdev->flags &= ~IFF_UP;
> + clear_bit(__LINK_STATE_START, &netdev->state);
> + }
[Severity: High]
The changelog says this "properly synchronize core netdev states
(__LINK_STATE_START / IFF_UP) and fire notifications if reopening
fails". Does this open-coded sequence match the core contract?
__dev_close_many() does:
call_netdevice_notifiers(NETDEV_GOING_DOWN, dev);
clear_bit(__LINK_STATE_START, &dev->state);
...
dev_deactivate_many(head, true);
...
netif_rx_mode_cancel_retry(dev);
netif_set_up(dev, false);
and only then netif_close_many() emits:
rtmsg_ifinfo(RTM_NEWLINK, dev, IFF_UP | IFF_RUNNING, GFP_KERNEL, 0, NULL);
call_netdevice_notifiers(NETDEV_DOWN, dev);
Compared to that, NETDEV_DOWN is delivered here before the state and
flag changes, so notifier consumers see the device as down while
netif_running() and IFF_UP still read true. dev->up is also left stale
true, since only netif_set_up() keeps IFF_UP and dev->up in sync:
if (value)
dev->flags |= IFF_UP;
else
dev->flags &= ~IFF_UP;
...
dev->up = value;
dev_deactivate_many() and netif_rx_mode_cancel_retry() are skipped, so
the qdisc stays active and the watchdog timer may remain armed on a
device whose rings and IRQs are freed, and the RTM_NEWLINK notification
never reaches rtnetlink monitors.
Also, this path never calls netif_device_attach(), so
__LINK_STATE_PRESENT stays clear and a later admin up cannot succeed,
while wxvf_resume() already returned 0. Is the interface meant to be
unusable until driver rebind?
> +
> + rtnl_unlock();
> + pci_disable_device(wx->pdev);
[Severity: Medium]
This disable is not recorded anywhere. enable_cnt is 1 at this point
(wxvf_resume() did the single pci_enable_device_mem()), so it drops to 0,
but only WX_STATE_DOWN is set and WX_FLAG_NEED_REOPEN is cleared. A
later wxvf_suspend() (also used as .shutdown) or wxvf_remove() calls
pci_disable_device() again unconditionally, which then hits:
dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0,
"disabling already-disabled device");
if (atomic_dec_return(&dev->enable_cnt) != 0)
return;
The early return skips do_pci_disable_device(), so bus mastering is
never cleared and dev->is_busmaster stays 1 after unbind. From then on
the accounting stays skewed by one in each direction. Should this path
track that the device was disabled, or leave it enabled and let the
normal suspend/remove path do the disable?
> + 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);
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908095801.61346-1-mengyuanlou%40net-swift.com
^ permalink raw reply [flat|nested] 2+ messages in thread