Re: [PATCH net v3 1/2] enic: retain admin-channel DMA until queues stop

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 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).

- [Medium] enic_remove() now holds rtnl_lock() across the V2 VF PF-
  unregister handshake and enic_admin_channel_close(), not just across…
- [Low] A failed VF re-registration in enic_admin_chan_reopen() can leave
  enic->vf_registered true after the channel has been closed.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  touches the exact line: when enic_open() fails inside enic_reset() or…

--

enic: retain admin-channel DMA until queues stop

This patch tracks the admin channel as DOWN, LIVE or QUARANTINED. When the
admin WQ/RQ cannot be shown to have stopped, it keeps the admin DMA graph
instead of freeing it. The reset workers and the probe/remove paths are
changed so they refuse to reopen or free a quarantined channel, and removal
holds RTNL while it detaches the netdev and closes the channel.

[ ... ]

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but this
line is being touched: what state is the netdev left in when enic_open()
fails here?

By this point enic_stop() has already called napi_disable() on every NAPI
and has called enic_free_intr(). enic_open() can then fail in
enic_request_intr(), page_pool_create(), the RQ fill or enic_dev_enable().
Its unwind disables the NAPIs it enabled and calls enic_free_intr() again.
__LINK_STATE_START and IFF_UP stay set the whole time.

enic_free_intr() also never clears msix[i].requested:

	case VNIC_DEV_INTR_MODE_MSIX:
		for (i = 0; i < enic->intr_count; i++)
			if (enic->msix[i].requested)
				free_irq(enic->msix_entry[i].vector,
					enic->msix[i].devid);

Any of these would then run enic_stop() again over the released
resources: a later ip link set down, the new dev_close() in the quarantine
branch of a later reset, or unregister_netdev() from enic_remove():

enic_stop()
    napi_disable(&enic->napi[i])    <- already disabled
    ...
    enic_free_intr(enic)            <- vectors already freed

Can the second napi_disable() loop forever in napi_disable_locked() while
RTNL is held? NAPIF_STATE_SCHED and NAPIF_STATE_NPSVC are still set from
the first disable. The repeated free_irq() calls would also WARN.

enic_tx_hang_reset() has the same sequence, with the same new
goto reset_out after a failed enic_open().

[Severity: Low]
Can vf_registered stay true after enic_admin_chan_reopen() closes the
channel because re-registration failed?

The commit message says:

  "Clear local VF registration after draining channel replies, including
  when recovery exits early."

The clears added to enic_reset() and enic_tx_hang_reset() run before the
reopen. The failure path inside enic_admin_chan_reopen() does not clear the
flag afterwards:

		err = enic_mbox_vf_register(enic);
		if (err) {
			netdev_err(...);
			enic_admin_channel_close(enic);
		}

enic_mbox_send_msg_id() publishes mbox_expected_reply and
mbox_expected_msg_num before it rings the doorbell. It then polls the WQ CQ
for up to 5 s while holding only mbox_lock. During that window,
admin_msg_work can accept a REGISTER_REPLY in enic_mbox_vf_handle_reply():

		case ENIC_MBOX_VF_REGISTER_REPLY:
			WRITE_ONCE(enic->vf_registered, true);

If the sender still sees no WQ completion, it returns -ETIMEDOUT. That
error comes back through enic_mbox_vf_register(), and the channel is closed
with vf_registered still set.

enic_remove() later sees vf_registered and calls enic_mbox_vf_unregister().
That call fails with -ENODEV because mbox_send_disabled is set, and remove
logs "Failed to unregister from PF".

Should vf_registered also be cleared after the enic_admin_channel_close()
calls in enic_admin_chan_reopen()?

[ ... ]

[Severity: Medium]
Is RTNL held here for longer than it needs to be?

The new RTNL section covers the whole V2 VF unregister handshake and the
channel close:

enic_remove()
  rtnl_lock()
  enic_mbox_vf_unregister()
    enic_mbox_send_msg_id()           <- WQ CQ poll, up to 5 s
    enic_mbox_wait_reply(enic, 3000)  <- up to 3 s
  enic_admin_channel_close()
    enic_admin_qp_type_set(enic, QP_DISABLE)
    enic_admin_wq_disable()           <- 500 ms, retried on -ETIMEDOUT
    enic_admin_rq_disable()           <- 2 x 500 ms, retried on -ETIMEDOUT
  rtnl_unlock()

The new admin helpers poll with:

	return read_poll_timeout(ioread32, running, !running, 10, 500000,
				 false, &wq->ctrl->running);

These replace the earlier vnic_wq_disable()/vnic_rq_disable() polls of
about 10 ms.

This patch targets the case where the admin queues don't stop or the PF
doesn't respond. In that case, could removing the VF stall every RTNL user
for roughly 8 to 11 seconds? The same longer queue polls now also run under
RTNL and mbox_lock in enic_reset() and enic_tx_hang_reset().

The reset works are already disabled at this point. After rtnl_unlock(),
both enic_open() and the core check netif_device_present(). Would holding
RTNL only around netif_device_detach() be enough to keep a concurrent open
out?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-b4-enic-dma-vf-index-net-generic-first-v1-20261010-v3-0-ba237ed71241%40cisco.com