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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 4

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] The notify DMA target that the patch now keeps while the admin
  graph is quarantined is never unregistered.
- [Medium] After admin quarantine, enic_admin_channel_close() calls
  pci_clear_master() (enic_admin.c:708).
- [Medium] enic_reset() and enic_tx_hang_reset() reopen the admin channel
  without checking whether the enic_open() just before it quarantined…
- [Medium] The admin quarantine path calls pci_clear_master() on the
  whole PCI function (enic_admin.c:708) while the generic datapath WQ/RQ…
- [Low] In the rewritten enic_probe() unwind, `goto
  err_out_disable_sriov;` sits on the line directly before the…
- [Low] Claim: enic_open() has no admin-quarantine gate, so
  enic_change_mtu_work -> _enic_change_mtu() -> enic_open() could re-…
- [Low] The three callers of the newly parameterised
  enic_datapath_release() apply different rules.
- [Low] enic_stop() decides whether to keep the shared notify target from
  a lockless READ_ONCE of admin_chan_state (enic_datapath_release(enic,…

--

enic: retain admin-channel DMA until queues stop

This patch tracks the enic admin channel as DOWN, LIVE or QUARANTINED. The
admin WQ/RQ/CQ graph is released only after both queue disables observe
RUNNING == 0. On a disable timeout the graph is kept and bus mastering is
cleared, and the reset, SR-IOV, probe and remove paths are updated to hold
quarantined graphs until a final recheck at remove time.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
> index 61c82b48044d0..a5c08cbcb3391 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_admin.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_admin.c

[ ... ]

> @@ -600,68 +612,141 @@ int enic_admin_channel_open(struct enic *enic)

[ ... ]

> +	/* QP_DISABLE does not report every low-level stop failure.  The direct
> +	 * WQ/RQ RUNNING observations own the admin allocation graph.
> +	 */
> +	if (wq_err || rq_err) {
> +		WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_QUARANTINED);
> +		WRITE_ONCE(enic->mbox_tx_poisoned, true);
> +		mutex_unlock(&enic->mbox_lock);
> +		netif_device_detach(enic->netdev);
> +		netif_carrier_off(enic->netdev);
> +		/* Stop new PCIe requests without treating bus-master disable as
> +		 * proof that already-issued DMA has completed.
> +		 */
> +		pci_clear_master(enic->pdev);

[Severity: Medium]
pci_clear_master() here applies to the whole PCI function, and the generic
datapath WQs and RQs may still be enabled. Can a healthy datapath still
drain after this?

In enic_reset() and enic_tx_hang_reset() the admin close runs before
enic_stop(). In a V2 VF enic_remove() it runs before
unregister_netdev()->enic_stop().

enic_stop() then calls vnic_wq_disable() and vnic_rq_disable(), which poll
ctrl->running for 500 ms. With bus mastering cleared, the function can no
longer do the descriptor fetches and completion writes needed to retire
in-flight work. If those polls time out, enic_datapath_quarantine() keeps
the datapath until reboot. That seems to contradict the commit message:

  "Keep the admin and generic datapath ownership graphs independent."

The same question applies to enic_admin_channel_recheck_on_remove(). It
polls the admin WQ/RQ after bus mastering has already been removed. The
commit message says clearing bus mastering does not "prevent the final
remove-time RUNNING recheck from observing a queue which subsequently
drains". Is this behaviour of the VIC queue engine documented anywhere?

> +		return wq_err ?: rq_err;
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 5a205ea3031b9..06d76648b420c 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c

[ ... ]

> @@ -1729,11 +1730,15 @@ static void enic_notify_timer_start(struct enic *enic)
>  	}
>  }
>  
> -static void enic_datapath_release(struct enic *enic)
> +static void enic_datapath_release(struct enic *enic, bool release_notify)
>  {
>  	unsigned int i;
>  
> -	enic_dev_notify_unset(enic);
> +	/* The notification allocation is a separate vNIC DMA target.  Keep it
> +	 * with the shared lifetime envelope if another DMA graph is quarantined.
> +	 */
> +	if (release_notify)
> +		enic_dev_notify_unset(enic);

[Severity: Medium]
Once this unset is skipped, does any later path ever do it?

enic_stop() can run while the admin graph is quarantined. Two examples:

  enic_reset()->enic_admin_channel_close() times out, then enic_stop()

  a V2 VF enic_remove()->enic_admin_channel_close() times out, then
  unregister_netdev()->enic_stop()

In both cases release_notify is false. datapath_state becomes
ENIC_DMA_DOWN while vdev->notify is still registered with firmware through
CMD_NOTIFY.

Later in enic_remove(), enic_admin_channel_recheck_on_remove() can succeed
and set admin_chan_state to ENIC_DMA_DOWN. enic_datapath_recheck_on_remove()
is skipped because the datapath is not quarantined, so no caller passes
release_notify=true. enic_dma_quarantined() is then false and the full
teardown runs:

enic_remove()
  enic_dev_deinit()
  vnic_dev_close()
  vnic_dev_unregister()
    dma_free_coherent(..., vdev->notify, vdev->notify_pa)

vnic_dev_unregister() frees the buffer without the CMD_NOTIFY retraction,
which only vnic_dev_notify_unsetcmd() issues. Can this free a coherent
buffer whose bus address firmware still holds as its notify target?

Bus mastering was cleared at quarantine. For this to cause corruption, the
stale registration would have to survive until a later bind, where
enic_probe() calls pci_set_master() before notify is set up again.

The kept block also makes any later enic_open() fail in
vnic_dev_notify_set() with "notify block ... still allocated".

[ ... ]

> @@ -1899,7 +1904,7 @@ static int enic_open(struct net_device *netdev)
>  		enic_datapath_quarantine(enic);
>  		return err;
>  	}
> -	enic_datapath_release(enic);
> +	enic_datapath_release(enic, true);

[Severity: Low]
Should this follow the same rule as enic_stop() and
enic_datapath_recheck_on_remove()? Both pass
!enic_admin_channel_is_quarantined(enic).

The comment in enic_datapath_release() says notify is kept "if another DMA
graph is quarantined".

For a V2 VF, enic_remove() calls enic_admin_channel_close() before
unregister_netdev() and without rtnl. At the same time, enic_open() can be
running under rtnl from a user "ip link set up".

If the admin close quarantines and clears bus mastering mid-open,
enic_dev_enable() can time out. This unwind then releases notify with
release_notify=true and issues a CMD_NOTIFY unset that cannot reach
firmware.

[Severity: Low]
Separately, enic_open() only checks enic_datapath_is_quarantined() before
it proceeds. Can enic_change_mtu_work->_enic_change_mtu()->enic_open()
reach this function after the admin graph has been quarantined and bus
mastering cleared?

In the final state of the series this does not seem to reach ring
enablement. enic_dev_notify_set() fails first, either with -EINVAL from the
kept notify block or with a DEVCMD2 timeout once "enic: require DEVCMD2 and
set the V2 VF mailbox source" makes DEVCMD2 mandatory.

With only this patch applied, though, a devcmd1 V2 VF could get past
CMD_NOTIFY and re-enable the datapath rings on a contained function. Would
an explicit enic_admin_channel_is_quarantined() check here be clearer?

[ ... ]

> @@ -1974,7 +1979,8 @@ static int enic_stop(struct net_device *netdev)
>  		return disable_err;
>  	}
>  
> -	enic_datapath_release(enic);
> +	enic_datapath_release(enic,
> +			      !enic_admin_channel_is_quarantined(enic));

[Severity: Low]
Is this lockless read of admin_chan_state stable against a concurrent
enic_admin_channel_close()?

enic_stop() holds only rtnl here. In enic_remove() for a V2 VF,
enic_admin_channel_close() runs before unregister_netdev() without rtnl. It
publishes ENIC_DMA_QUARANTINED under mbox_lock only:

CPU0 (ip link set down)          CPU1 (enic_remove)
enic_stop()
  reads "not quarantined"
                                 enic_admin_channel_close()
                                   WRITE_ONCE(enic->admin_chan_state,
                                              ENIC_DMA_QUARANTINED)
                                   pci_clear_master()
  enic_datapath_release(enic, true)
    enic_dev_notify_unset()

This releases the notify target while another graph is quarantined, which
release_notify is meant to prevent.

[ ... ]

> @@ -2398,13 +2426,17 @@ static void enic_reset(struct work_struct *work)
>  	if (reopen)
>  		enic_open(enic->netdev);
>  

[ ... ]

>  	if (enic_has_admin_chan(enic))
>  		enic_admin_chan_reopen(enic);

[Severity: Medium]
Should enic_admin_chan_reopen() be skipped when the enic_open() just above
has quarantined the datapath?

The return value of enic_open() is ignored. enic_open() can enable RQs,
fail later, and then time out disabling an RQ. In that case
enic_datapath_quarantine() sets mbox_send_disabled, detaches the netdev and
clears bus mastering.

The new checks in enic_admin_channel_open() test only admin_chan_state and
mbox_tx_poisoned (and, later in the series, sriov_stats_dma_unresolved).
The reopen therefore still allocates admin resources, requests the admin
IRQ, enables the admin WQ/RQ, fills the admin RQ with freshly mapped
buffers and sends QP_ENABLE.

On a devcmd1 device QP_ENABLE succeeds and mbox_send_disabled is cleared
again, which undoes enic_datapath_quarantine(). With DEVCMD2, QP_ENABLE
times out and the partial-open close polls admin queues that cannot
complete DMA. That could quarantine the admin graph too.

enic_tx_hang_reset() has the same sequence.

> +	if (enic_admin_channel_is_quarantined(enic)) {
> +		stop_err = enic_stop(enic->netdev);
> +		netdev_err(enic->netdev,
> +			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
> +			   stop_err);
> +	}

[ ... ]

> @@ -2422,30 +2454,39 @@ static void enic_tx_hang_reset(struct work_struct *work)

[ ... ]

> -	if (enic_has_admin_chan(enic))
> -		enic_admin_channel_close(enic);
> +	if (enic_has_admin_chan(enic)) {
> +		err = enic_admin_channel_close(enic);
> +		if (err && enic_admin_channel_is_quarantined(enic))
> +			netdev_err(enic->netdev,
> +				   "Admin queue did not stop: %d\n", err);
> +		else if (err)
> +			netdev_warn(enic->netdev,
> +				    "Admin QP disable returned: %d\n", err);
> +	}
>  
>  	enic_dev_hang_notify(enic);
>  	if (reopen)
>  		stop_err = enic_stop(enic->netdev);

[Severity: Medium]
If enic_admin_channel_close() has just quarantined and called
pci_clear_master(), can the devcmds that follow still be delivered?

Later in the series ("enic: require DEVCMD2 and set the V2 VF mailbox
source") DEVCMD2 becomes mandatory for admin-channel functions.
_vnic_dev_cmd2() writes into the host-memory cmd_ring, bumps posted_index,
and then busy-polls result->color. Only a device DMA write can update
result->color.

With bus mastering off, each of these commands would spin for its full
timeout under devcmd_lock with BH disabled, then return -ETIMEDOUT:

enic_tx_hang_reset()
  enic_dev_hang_notify()          HANG_NOTIFY
  enic_stop()
    enic_rfs_flw_tbl_free()       filter deletes
    enic_dev_disable()            CMD_DISABLE

enic_reset()->enic_stop() runs into the same thing.

dc2c->posted, next_result and color keep advancing while the hardware
fetch_index does not. Once the ring fills, commands fail with -EBUSY.

At remove, after a successful admin recheck, the CMD_NOTIFY unset,
enic_dev_deinit() and vnic_dev_close() are lost the same way. Then
vnic_dev_deinit_devcmd2() frees the results ring before it disables a
DEVCMD2 WQ that still holds unfetched descriptors. Does this leave firmware
with the vNIC open and a stale results-ring address?

[ ... ]

> @@ -3499,12 +3564,24 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)

[ ... ]

> -err_out_dev_deinit:
>  	enic_dev_deinit(enic);
>  err_out_dev_close:
>  	vnic_dev_close(enic->vdev);
> +	goto err_out_disable_sriov;
>  err_out_disable_sriov:

[Severity: Low]
This isn't a bug, but this goto jumps to the label on the very next line.
Was it meant to be removed?

>  	kfree(enic->pp);

[ ... ]

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

[PATCH net 0/3] enic: fence DMA ownership and identify V2 VFs

[Satish Kharat]

An automated Sashiko review of the V2 SR-IOV reset paths prompted a
broader audit of ENIC queue teardown. That audit found three prerequisite
correctness problems in code which is already upstream.

First, a datapath WQ or RQ disable can time out while hardware may still
own descriptors and their DMA mappings. Patch 1 replaces iteration-counted
polling with a bounded 500 ms RUNNING poll for each queue. The RQ keeps its
required two disable-and-poll passes. The datapath graph is released only
after every applicable queue reports RUNNING == 0. Because rings, mapped
buffers, page pools, and shared state form a coupled ownership graph, it is
not partially released when only some queues stop.

The queue-disable helpers can return -ETIMEDOUT without observing
RUNNING == 0. Firmware defines RUNNING == 0 as conclusive evidence that
queue DMA has stopped. A nonzero RUNNING value does not establish that
DMA is active, but it leaves DMA ownership unresolved. The existing error
path can nevertheless release DMA-backed resources without obtaining the
required ownership proof.

This series deliberately retains the complete datapath graph if any
datapath queue does not report RUNNING == 0. This can retain resources
belonging to queues which did stop, but keeps the ownership rule simple
in an exceptional failure path. Reclaiming packet buffers individually for
queues which successfully stop is a possible follow-up optimization; it
is not required to prevent DMA into freed memory.

If a queue does not report RUNNING == 0, the whole datapath graph is
quarantined. A final direct queue recheck runs during device removal,
before terminal containment. The graph is released if every queue reports
RUNNING == 0 by then; otherwise it is retained until reboot. Firmware soft
reset, hang reset, and PCI function reset are not treated as independent
ownership proof for a queue whose stop could not be confirmed.

The terminal path detaches CPU entry points, clears bus mastering, disables
the PCI function, and intentionally retains the netdev, vnic_dev, BAR
mappings and reservations, rings, mapped buffers, page pools, and graph
metadata until reboot. Clearing bus mastering blocks new device-initiated
PCI transactions; it does not cancel DMA already in progress or prevent a
queue from draining to RUNNING == 0. The final remove-time recheck
therefore remains meaningful, but bus-master state alone does not prove
that a queue with unresolved DMA ownership relinquished its old targets.
The BAR reservation may outlive driver unbind and module unload while the
PCI device remains enumerated. The resource subsystem stores the supplied
owner-name pointer rather than copying it, so the reservation uses the PCI
device name instead of module rodata.

A PCI driver's remove callback cannot reject driver unbind. With DMA API
debugging enabled, the driver's deliberate retention therefore produces the
expected "pending DMA allocations while released from device" diagnostic
if the module is unloaded after terminal quarantine. The retained BAR
reservation prevents a normal ENIC rebind; reboot is the supported
recovery.

Ring resizing needs an explicit stop because dev_close() cannot propagate
an ndo_stop() error. Patch 1 returns a queue-disable failure to ethtool and
rejects later reconfiguration after quarantine. It also makes reset, IRQ,
NAPI, notification-buffer, and final-remove ownership transitions
idempotent.

Second, an admin-WQ send can time out while hardware may still own the
descriptor and mapping. Patch 2 applies the same bounded polling,
observed-queue-stop rule, and final remove-time recheck to the admin WQ/RQ
graph. The datapath and admin channel are independent complete graphs: a
clean graph may be released even when the other is quarantined, but an
individual graph is never partially released. Shared objects needed by an
unresolved DMA target remain retained. Putting the generic repair first
makes every committed state independently safe.

Third, the in-tree V2 VF always transmits mailbox source zero. Deployed
async PFs compare that field with sender identity from CQ metadata, so a
nonzero VF cannot register. Patch 3 obtains firmware's logical VF index
with CMD_SRIOV_STATS_GET after the admin channel is live and uses it as the
source of subsequent V2 VF mailbox messages.

Patch 3 also requires DEVCMD2 for SR-IOV V2. A V2 VF without DEVCMD2
fails probe before opening its admin channel. A PF which fell back to
DEVCMD1 keeps ordinary Ethernet operation but does not expose V2 SR-IOV;
V1 and USNIC keep their existing DEVCMD1 fallback. With DEVCMD2, -ENODEV
is detected before a descriptor is posted, so only a timeout can leave
ownership of the persistent statistics-command DMA target uncertain. A
later successful blocking command for the same vNIC proves that target
completed; otherwise it remains retained with the shared vNIC lifetime
envelope until reboot.

The companion V2 VF receive-control v3 series carries explicit prerequisite
patch IDs for all three patches.

AI assistance:

An LLM was used for design review, source review, commit-message drafting,
test automation, and review triage. Sashiko was used as an additional
review tool. All findings and generated changes were manually reviewed, and
the author takes responsibility for the series.

Validation:

- Tests used Cisco VIC PF and V2 VF functions. PF/VF probe, open/close,
  repeated clean module unload/reload, link cycling, ring resizing, jumbo
  MTU, and bidirectional ICMP, TCP, and UDP traffic passed.
- A nonzero V2 VF registered and passed traffic with the deployed async PF.
  Before patch 3, the PF rejected the VF's zero mailbox source because it
  did not match the sender identity obtained from CQ metadata.
- The full no-reboot suite exercised exactly 32 secondary unicast
  addresses, 33-address fallback, address churn, station-address
  multicast-list changes, administrative-MAC policy, 25 module reloads,
  10 ring resize cycles, 10 reopen cycles, and five VF destroy/recreate
  cycles.
- A test-only admin-channel overlay injected one false first-attempt WQ
  disable failure and then one false first-attempt RQ disable failure after
  the corresponding real hardware disable had succeeded. Each failed helper
  alone was retried, each retry succeeded, and the production module
  then passed bidirectional traffic.
- A test-only V2 VF overlay forced exactly one posted mailbox send timeout.
  Serialized reset recovery rebuilt the admin channel, refreshed the
  administrative MAC, restored bidirectional traffic, and completed a later
  exact-filter transaction.
- A disposable-VF-guest test overlay held the selected WQ or RQ's observed
  RUNNING value high without preventing the real hardware disable. A 300 ms
  hold verified that both queue types wait rather than time out early. A
  hold through the 500 ms bound quarantined each datapath and admin queue
  path. Clearing the synthetic condition before removal let the final
  recheck release each complete graph. Leaving it set exercised terminal
  retention. The uninstrumented candidate then bound and passed
  bidirectional ENIC traffic after guest reboot.

Queue-stop failure handling was exercised using instrumentation which
kept the RUNNING value observed by the driver nonzero after the underlying
hardware queue had stopped. This exercised the bounded polling,
quarantine, retained-resource, and final remove-time recheck paths. It
validates the driver's handling when RUNNING == 0 is not observed; it does
not attempt to reproduce or characterize the underlying hardware condition
which produces that result. If ownership remains unresolved at removal,
reboot is the supported recovery.

The DEVCMD2 requirement and pre-admin-channel V2 VF rejection were
verified by source review and exact per-commit builds. The physical test
adapter uses DEVCMD2, so the no-DEVCMD2 rejection path was not exercised
on hardware.

Link: https://patch.msgid.link/20260929-b4-enic-sriov-v2-vf-receive-control-v2-0-5962c10061b2@cisco.com
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
Satish Kharat (3):
      enic: retain datapath DMA resources until queues stop
      enic: retain admin-channel DMA until queues stop
      enic: require DEVCMD2 and set the V2 VF mailbox source

 drivers/net/ethernet/cisco/enic/enic.h         |  36 +-
 drivers/net/ethernet/cisco/enic/enic_admin.c   | 182 ++++++---
 drivers/net/ethernet/cisco/enic/enic_admin.h   |   4 +-
 drivers/net/ethernet/cisco/enic/enic_dev.c     |  15 +
 drivers/net/ethernet/cisco/enic/enic_dev.h     |   2 +
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |  10 +-
 drivers/net/ethernet/cisco/enic/enic_main.c    | 508 +++++++++++++++++++++----
 drivers/net/ethernet/cisco/enic/enic_mbox.c    |  13 +-
 drivers/net/ethernet/cisco/enic/vnic_dev.c     |  42 ++
 drivers/net/ethernet/cisco/enic/vnic_dev.h     |   5 +
 drivers/net/ethernet/cisco/enic/vnic_devcmd.h  |  34 ++
 drivers/net/ethernet/cisco/enic/vnic_rq.c      |  18 +-
 drivers/net/ethernet/cisco/enic/vnic_wq.c      |  18 +-
 13 files changed, 735 insertions(+), 152 deletions(-)
---
base-commit: 6d25ffca055a77787c21a36b66c253f76239411b
change-id: 20261010-b4-enic-dma-vf-index-net-generic-first-v1-20261010-d9abf4510335

Best regards,
--  
Satish Kharat <satishkh@cisco.com>

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

[Satish Kharat]

A timed-out admin WQ send leaves ownership of its descriptor and mapped
message unresolved. Independently, an admin RQ disable can time out
without observing RUNNING == 0, leaving DMA ownership of its posted
receive buffers and associated completion state unresolved. Channel close
currently logs WQ and RQ disable failures, then cleans and frees the full
WQ/RQ/CQ allocation graph.

Track the admin channel as DOWN, LIVE, or QUARANTINED. Block new
mailbox sends, stop its CPU callbacks, and take mbox_lock to wait for any
in-flight sender before disabling the queues. Release the graph only after
the WQ disable succeeds and both passes of the RQ disable succeed. The
preceding datapath fix gives every RUNNING poll a driver-chosen 500 ms
bound and preserves the two RQ passes required for its mini-cache. Run
each queue's complete disable helper once, then retry once only a helper
that timed out. Quarantine the graph if either retry still times out.

A mailbox send timeout alone is not terminal. Its mapping remains
attached to the live WQ, and a later clean WQ/RQ stop supplies
the ownership proof needed to release the graph. Only failure of a
queue-disable RUNNING check makes the admin graph quarantined.

Keep the admin and generic datapath ownership graphs independent. If
the admin graph is quarantined, stop the generic datapath and release
its queue resources only if all of its work and receive queues stop.
Likewise, a quarantined datapath does not prevent release of an admin
graph whose queues both stop. Do not partially release an individual graph
when only some of its queues stop. Retain the shared netdev, vnic_dev, BAR
mappings, PCI reservations, and any unresolved vNIC DMA target whenever
either complete graph remains quarantined.

Soft reset, hang reset, and PCI function reset do not provide an
independent ownership fence for a queue whose stop could not be
confirmed. Clear bus mastering to contain the function, detach software
entry points, and retain each quarantined graph during runtime. Clearing
bus mastering does not cancel DMA already in progress or prevent the final
remove-time
RUNNING recheck from observing a queue which subsequently drains.

Apply the ownership rules to partial open, reset, SR-IOV disable,
probe unwind, and remove. A reset worker encountering terminal admin
quarantine only converges the independent datapath to a stopped state;
it does not issue a firmware reset or reopen either graph.

At remove, after all CPU entry points have been detached, make one final
direct disable/RUNNING check of the quarantined admin WQ and RQ. Release
the complete admin graph if both queues have now stopped. Otherwise
retain it with the shared owner objects until reboot.

The issue was found while reviewing the V2 VF receive-control series
after its mailbox timeout path deliberately retained a DMA mapping. The
audit showed that admin-channel close could nevertheless free its owner
graph after an unconfirmed queue stop.

Fixes: 3258931d4052 ("enic: add admin channel open and close for SR-IOV")
Assisted-by: LLM
Signed-off-by: Satish Kharat <satishkh@cisco.com>
Reviewed-by: Aravind Dhanasekaran <aradhana@cisco.com>
Reviewed-by: Reese Faucette <rfaucett@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic.h       |  13 +-
 drivers/net/ethernet/cisco/enic/enic_admin.c | 171 +++++++++++++++++++-------
 drivers/net/ethernet/cisco/enic/enic_admin.h |   4 +-
 drivers/net/ethernet/cisco/enic/enic_main.c  | 175 ++++++++++++++++++++-------
 drivers/net/ethernet/cisco/enic/enic_mbox.c  |   1 +
 5 files changed, 274 insertions(+), 90 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 992ac43d1b0a..cf2dee8fa78f 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -306,13 +306,16 @@ struct enic {
 
 	/* Admin channel resources for SR-IOV MBOX */
 	bool has_admin_channel;
-	/* true only while the admin WQ/RQ/CQ are allocated and enabled; gates
-	 * enic_admin_channel_close() so it is a no-op after a failed (re)open
-	 * left the resources freed.
+	/* DOWN has no allocated admin DMA resources. QUARANTINED retains the
+	 * complete graph after a WQ or RQ failed to stop.
 	 */
-	bool admin_chan_up;
-	/* set on send timeout; cleared on channel re-open */
+	enum enic_dma_state admin_chan_state;
+	/* Blocks sends while the channel is closed or awaiting recovery. */
 	bool mbox_send_disabled;
+	/* A send timeout leaves a descriptor hardware-owned until both queues
+	 * report RUNNING == 0.
+	 */
+	bool mbox_tx_poisoned;
 	struct vnic_wq admin_wq;
 	struct vnic_rq admin_rq;
 	struct vnic_cq admin_cq[2];
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.c b/drivers/net/ethernet/cisco/enic/enic_admin.c
index 61c82b48044d..a5c08cbcb339 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.c
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.c
@@ -530,11 +530,22 @@ static void enic_admin_msg_drain(struct enic *enic)
 
 int enic_admin_channel_open(struct enic *enic)
 {
+	int close_err;
 	int err;
 
 	if (!enic->has_admin_channel)
 		return -ENODEV;
-
+	if (READ_ONCE(enic->admin_chan_state) != ENIC_DMA_DOWN) {
+		netdev_err(enic->netdev,
+			   "Refusing to open non-down admin channel (%u)\n",
+			   READ_ONCE(enic->admin_chan_state));
+		return -EBUSY;
+	}
+	if (READ_ONCE(enic->mbox_tx_poisoned)) {
+		netdev_err(enic->netdev,
+			   "Refusing to reopen poisoned admin channel\n");
+		return -EIO;
+	}
 	/* Keep MBOX sends disabled for the entire open sequence.  It is
 	 * cleared only after every resource is allocated and enabled below,
 	 * so any early error return here leaves sends disabled and a
@@ -567,6 +578,7 @@ int enic_admin_channel_open(struct enic *enic)
 
 	vnic_wq_enable(&enic->admin_wq);
 	vnic_rq_enable(&enic->admin_rq);
+	WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_LIVE);
 
 	err = enic_admin_rq_fill(enic, GFP_KERNEL);
 	if (err) {
@@ -600,68 +612,141 @@ int enic_admin_channel_open(struct enic *enic)
 		   enic->admin_cq[0].last_color,
 		   enic->admin_cq[1].last_color);
 
-	enic->admin_chan_up = true;
-
 	return 0;
 
 disable_queues:
-	enic_admin_teardown_intr(enic);
-	enic_admin_qp_type_set(enic, QP_DISABLE);
-	if (vnic_wq_disable(&enic->admin_wq))
-		netdev_warn(enic->netdev, "Failed to disable admin WQ\n");
-	if (vnic_rq_disable(&enic->admin_rq))
-		netdev_warn(enic->netdev, "Failed to disable admin RQ\n");
-	cancel_work_sync(&enic->admin_msg_work);
-	enic_admin_msg_drain(enic);
-	enic_admin_rq_drain(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close partial admin channel: %d\n",
+			   close_err);
+	return err;
 free_resources:
 	enic_admin_free_resources(enic);
 	return err;
 }
 
-void enic_admin_channel_close(struct enic *enic)
+static void enic_admin_channel_release(struct enic *enic)
 {
-	int err;
+	vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
+	enic_admin_rq_drain(enic);
+	vnic_cq_clean(&enic->admin_cq[0]);
+	vnic_cq_clean(&enic->admin_cq[1]);
+	vnic_intr_clean(&enic->admin_intr);
 
-	if (!enic->has_admin_channel)
-		return;
+	enic->admin_rq_handler = NULL;
+	enic_admin_free_resources(enic);
+	WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_DOWN);
+}
 
-	/* Nothing to tear down if the channel was never (re)opened, e.g. a
-	 * failed enic_admin_channel_open() in probe or in the reset path;
-	 * otherwise the disable/clean calls below dereference freed resources.
-	 */
-	if (!enic->admin_chan_up)
-		return;
+int enic_admin_channel_close(struct enic *enic)
+{
+	int qp_err;
+	int rq_err;
+	int wq_err;
+
+	if (!enic->has_admin_channel)
+		return 0;
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_DOWN)
+		return 0;
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_QUARANTINED)
+		return -EBUSY;
 
 	WRITE_ONCE(enic->mbox_send_disabled, true);
 
-	netdev_dbg(enic->netdev, "admin channel close\n");
+	if (READ_ONCE(enic->admin_chan_state) == ENIC_DMA_LIVE) {
+		netdev_dbg(enic->netdev, "admin channel quiesce\n");
 
-	vnic_intr_mask(&enic->admin_intr);
-	enic_admin_teardown_intr(enic);
-	cancel_work_sync(&enic->link_notify_work);
-	cancel_work_sync(&enic->admin_msg_work);
-	enic_admin_msg_drain(enic);
+		vnic_intr_mask(&enic->admin_intr);
+		enic_admin_teardown_intr(enic);
+		cancel_work_sync(&enic->link_notify_work);
+		cancel_work_sync(&enic->admin_msg_work);
+		enic_admin_msg_drain(enic);
+	}
 
-	enic_admin_qp_type_set(enic, QP_DISABLE);
+	/* A sender may have passed the gate before it was raised.  Wait only
+	 * after cancelling workers which can themselves wait for mbox_lock.
+	 */
+	mutex_lock(&enic->mbox_lock);
+	qp_err = enic_admin_qp_type_set(enic, QP_DISABLE);
+	if (qp_err)
+		netdev_warn(enic->netdev,
+			    "Failed to disable admin QP type: %d\n", qp_err);
 
-	err = vnic_wq_disable(&enic->admin_wq);
-	if (err)
+	wq_err = vnic_wq_disable(&enic->admin_wq);
+	rq_err = vnic_rq_disable(&enic->admin_rq);
+
+	/* Run both complete disable helpers once before retrying only a helper
+	 * which timed out.
+	 */
+	if (wq_err)
+		wq_err = vnic_wq_disable(&enic->admin_wq);
+	if (rq_err)
+		rq_err = vnic_rq_disable(&enic->admin_rq);
+
+	if (wq_err)
 		netdev_warn(enic->netdev,
-			    "Failed to disable admin WQ: %d\n", err);
-	err = vnic_rq_disable(&enic->admin_rq);
-	if (err)
+			    "Failed to disable admin WQ after retry: %d\n",
+			    wq_err);
+	if (rq_err)
 		netdev_warn(enic->netdev,
-			    "Failed to disable admin RQ: %d\n", err);
+			    "Failed to disable admin RQ after retry: %d\n",
+			    rq_err);
 
-	vnic_wq_clean(&enic->admin_wq, enic_admin_wq_buf_clean);
-	enic_admin_rq_drain(enic);
-	vnic_cq_clean(&enic->admin_cq[0]);
-	vnic_cq_clean(&enic->admin_cq[1]);
-	vnic_intr_clean(&enic->admin_intr);
+	/* QP_DISABLE does not report every low-level stop failure.  The direct
+	 * WQ/RQ RUNNING observations own the admin allocation graph.
+	 */
+	if (wq_err || rq_err) {
+		WRITE_ONCE(enic->admin_chan_state, ENIC_DMA_QUARANTINED);
+		WRITE_ONCE(enic->mbox_tx_poisoned, true);
+		mutex_unlock(&enic->mbox_lock);
+		netif_device_detach(enic->netdev);
+		netif_carrier_off(enic->netdev);
+		/* Stop new PCIe requests without treating bus-master disable as
+		 * proof that already-issued DMA has completed.
+		 */
+		pci_clear_master(enic->pdev);
+		return wq_err ?: rq_err;
+	}
 
-	enic->admin_rq_handler = NULL;
-	enic_admin_free_resources(enic);
+	enic_admin_channel_release(enic);
+	WRITE_ONCE(enic->mbox_tx_poisoned, false);
+	mutex_unlock(&enic->mbox_lock);
+
+	/* Firmware rejection does not undermine the WQ/RQ proof. */
+	return qp_err;
+}
+
+int enic_admin_channel_recheck_on_remove(struct enic *enic)
+{
+	int rq_err;
+	int wq_err;
+
+	if (READ_ONCE(enic->admin_chan_state) != ENIC_DMA_QUARANTINED)
+		return 0;
+
+	/* Channel close already detached every CPU entry point and attempted
+	 * QP_DISABLE.  Removal is the last direct check of queue ownership.
+	 */
+	mutex_lock(&enic->mbox_lock);
+	wq_err = vnic_wq_disable(&enic->admin_wq);
+	rq_err = vnic_rq_disable(&enic->admin_rq);
+	if (wq_err || rq_err) {
+		mutex_unlock(&enic->mbox_lock);
+		return wq_err ?: rq_err;
+	}
 
-	enic->admin_chan_up = false;
+	enic_admin_channel_release(enic);
+	WRITE_ONCE(enic->mbox_tx_poisoned, false);
+	mutex_unlock(&enic->mbox_lock);
+	netdev_info(enic->netdev,
+		    "Admin queues stopped at remove; releasing DMA graph\n");
+
+	return 0;
+}
+
+bool enic_admin_channel_is_quarantined(const struct enic *enic)
+{
+	return READ_ONCE(enic->admin_chan_state) ==
+		ENIC_DMA_QUARANTINED;
 }
diff --git a/drivers/net/ethernet/cisco/enic/enic_admin.h b/drivers/net/ethernet/cisco/enic/enic_admin.h
index 62c80220b0ca..5945bbb2e6d4 100644
--- a/drivers/net/ethernet/cisco/enic/enic_admin.h
+++ b/drivers/net/ethernet/cisco/enic/enic_admin.h
@@ -20,7 +20,9 @@ struct enic_admin_msg {
 };
 
 int enic_admin_channel_open(struct enic *enic);
-void enic_admin_channel_close(struct enic *enic);
+int enic_admin_channel_close(struct enic *enic);
+int enic_admin_channel_recheck_on_remove(struct enic *enic);
+bool enic_admin_channel_is_quarantined(const struct enic *enic);
 unsigned int enic_admin_wq_cq_service(struct enic *enic);
 unsigned int enic_admin_rq_cq_service(struct enic *enic);
 
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 5a205ea3031b..06d76648b420 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -71,6 +71,7 @@
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_V2   0x02b7  /* enet SRIOV V2 VF */
 #define PCI_DEVICE_ID_CISCO_VIC_ENET_VF_USNIC 0x00cf /* enet USNIC VF */
 
+static bool enic_dma_quarantined(struct enic *enic);
 static void enic_terminal_dma_quarantine(struct enic *enic);
 
 /* Supported devices */
@@ -1729,11 +1730,15 @@ static void enic_notify_timer_start(struct enic *enic)
 	}
 }
 
-static void enic_datapath_release(struct enic *enic)
+static void enic_datapath_release(struct enic *enic, bool release_notify)
 {
 	unsigned int i;
 
-	enic_dev_notify_unset(enic);
+	/* The notification allocation is a separate vNIC DMA target.  Keep it
+	 * with the shared lifetime envelope if another DMA graph is quarantined.
+	 */
+	if (release_notify)
+		enic_dev_notify_unset(enic);
 
 	enic_unset_affinity_hint(enic);
 	enic_free_intr(enic);
@@ -1899,7 +1904,7 @@ static int enic_open(struct net_device *netdev)
 		enic_datapath_quarantine(enic);
 		return err;
 	}
-	enic_datapath_release(enic);
+	enic_datapath_release(enic, true);
 
 	return err;
 
@@ -1974,7 +1979,8 @@ static int enic_stop(struct net_device *netdev)
 		return disable_err;
 	}
 
-	enic_datapath_release(enic);
+	enic_datapath_release(enic,
+			      !enic_admin_channel_is_quarantined(enic));
 
 	return 0;
 }
@@ -2328,18 +2334,30 @@ static void enic_admin_chan_reopen(struct enic *enic)
 	if (enic_is_sriov_vf_v2(enic)) {
 		err = enic_mbox_vf_capability_check(enic);
 		if (err) {
+			int close_err;
+
 			netdev_err(enic->netdev,
 				   "MBOX capability check after reset failed: %d\n",
 				   err);
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_err(enic->netdev,
+					   "Admin close after capability failure failed: %d\n",
+					   close_err);
 			return;
 		}
 		err = enic_mbox_vf_register(enic);
 		if (err) {
+			int close_err;
+
 			netdev_err(enic->netdev,
 				   "MBOX VF re-registration after reset failed: %d\n",
 				   err);
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_err(enic->netdev,
+					   "Admin close after registration failure failed: %d\n",
+					   close_err);
 		}
 	} else {
 		/* The link came back up during enic_open() above while MBOX
@@ -2355,9 +2373,14 @@ static void enic_reset(struct work_struct *work)
 	struct enic *enic = container_of(work, struct enic, reset);
 	int stop_err = 0;
 	bool reopen;
+	int err;
 
-	if (enic_datapath_is_quarantined(enic) ||
-	    !netif_running(enic->netdev))
+	if (enic_datapath_is_quarantined(enic))
+		return;
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    READ_ONCE(enic->datapath_state) != ENIC_DMA_LIVE)
+		return;
+	if (!netif_running(enic->netdev))
 		return;
 
 	rtnl_lock();
@@ -2368,22 +2391,27 @@ static void enic_reset(struct work_struct *work)
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
 
-	/* Fully tear down the V2 admin/MBOX channel before the soft reset.
-	 * The reset wipes all hardware queues including the admin WQ/RQ;
-	 * closing first tells firmware to stop the admin QP (so it no longer
-	 * DMAs from the about-to-be-reset rings) and frees the admin resources
-	 * so they are cleanly re-allocated afterwards.
+	/* Stop the V2 admin/MBOX graph before the soft reset.  A queue timeout
+	 * quarantines that graph permanently; continue only far enough to stop
+	 * and, when possible, release the independent datapath graph.
 	 */
-	if (enic_has_admin_chan(enic))
-		enic_admin_channel_close(enic);
+	if (enic_has_admin_chan(enic)) {
+		err = enic_admin_channel_close(enic);
+		if (err && enic_admin_channel_is_quarantined(enic))
+			netdev_err(enic->netdev,
+				   "Admin queue did not stop: %d\n", err);
+		else if (err)
+			netdev_warn(enic->netdev,
+				    "Admin QP disable returned: %d\n", err);
+	}
 
 	if (reopen)
 		stop_err = enic_stop(enic->netdev);
 	if (enic_is_sriov_vf_v2(enic))
 		enic_mbox_vf_link_state_reset(enic);
-	if (enic_datapath_is_quarantined(enic)) {
+	if (enic_dma_quarantined(enic)) {
 		netdev_err(enic->netdev,
-			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   "Queue DMA ownership unresolved (datapath stop %d); resources retained until reboot\n",
 			   stop_err);
 		goto reset_out;
 	}
@@ -2398,13 +2426,17 @@ static void enic_reset(struct work_struct *work)
 	if (reopen)
 		enic_open(enic->netdev);
 
-	/* Re-establish the admin/MBOX channel after the data path is back up.
-	 * It was fully torn down by enic_admin_channel_close() above;
-	 * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
-	 * state, or for a VF re-runs the probe-time PF handshake.
+	/* Re-establish the admin/MBOX channel after its clean stop.  For a PF,
+	 * re-push link state; for a VF, rerun the probe-time PF handshake.
 	 */
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
+	if (enic_admin_channel_is_quarantined(enic)) {
+		stop_err = enic_stop(enic->netdev);
+		netdev_err(enic->netdev,
+			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
+			   stop_err);
+	}
 
 reset_out:
 	/* Allow infiniband to fiddle with the device again */
@@ -2422,30 +2454,39 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	struct enic *enic = container_of(work, struct enic, tx_hang_reset);
 	int stop_err = 0;
 	bool reopen;
+	int err;
 
 	rtnl_lock();
 	reopen = netif_running(enic->netdev);
-	if (!reopen || enic_datapath_is_quarantined(enic))
+	if (enic_datapath_is_quarantined(enic) ||
+	    (enic_admin_channel_is_quarantined(enic) &&
+	     READ_ONCE(enic->datapath_state) != ENIC_DMA_LIVE) || !reopen)
 		goto unlock;
 
 	/* Stop any activity from infiniband */
 	enic_set_api_busy(enic, true);
 
-	/* Fully tear down the V2 admin/MBOX channel before the hang reset, for
-	 * the same reason as the soft reset path: stop the admin QP and free
-	 * the admin resources before the hardware queues are wiped.
+	/* Stop the V2 admin/MBOX graph before the hang reset.  As in the soft
+	 * reset path, a queue timeout is terminal for that graph.
 	 */
-	if (enic_has_admin_chan(enic))
-		enic_admin_channel_close(enic);
+	if (enic_has_admin_chan(enic)) {
+		err = enic_admin_channel_close(enic);
+		if (err && enic_admin_channel_is_quarantined(enic))
+			netdev_err(enic->netdev,
+				   "Admin queue did not stop: %d\n", err);
+		else if (err)
+			netdev_warn(enic->netdev,
+				    "Admin QP disable returned: %d\n", err);
+	}
 
 	enic_dev_hang_notify(enic);
 	if (reopen)
 		stop_err = enic_stop(enic->netdev);
 	if (enic_is_sriov_vf_v2(enic))
 		enic_mbox_vf_link_state_reset(enic);
-	if (enic_datapath_is_quarantined(enic)) {
+	if (enic_dma_quarantined(enic)) {
 		netdev_err(enic->netdev,
-			   "Datapath queue did not stop: %d; DMA resources retained until reboot\n",
+			   "Queue DMA ownership unresolved (datapath stop %d); resources retained until reboot\n",
 			   stop_err);
 		goto hang_reset_out;
 	}
@@ -2460,13 +2501,17 @@ static void enic_tx_hang_reset(struct work_struct *work)
 	if (reopen)
 		enic_open(enic->netdev);
 
-	/* Re-establish the admin/MBOX channel after the data path is back up.
-	 * It was fully torn down by enic_admin_channel_close() above;
-	 * enic_admin_chan_reopen() reopens it and, for a PF re-pushes link
-	 * state, or for a VF re-runs the probe-time PF handshake.
+	/* Re-establish the admin/MBOX channel after its clean stop.  For a PF,
+	 * re-push link state; for a VF, rerun the probe-time PF handshake.
 	 */
 	if (enic_has_admin_chan(enic))
 		enic_admin_chan_reopen(enic);
+	if (enic_admin_channel_is_quarantined(enic)) {
+		stop_err = enic_stop(enic->netdev);
+		netdev_err(enic->netdev,
+			   "Admin queue did not stop after reopen failure (datapath stop %d); resources retained until reboot\n",
+			   stop_err);
+	}
 
 hang_reset_out:
 	/* Allow infiniband to fiddle with the device again */
@@ -2998,6 +3043,7 @@ static void enic_sriov_detect_vf_type(struct enic *enic)
 static int __maybe_unused
 enic_sriov_v2_enable(struct enic *enic, int num_vfs)
 {
+	int close_err;
 	int err;
 
 	if (!enic->has_admin_channel) {
@@ -3037,15 +3083,26 @@ enic_sriov_v2_enable(struct enic *enic, int num_vfs)
 
 close_admin:
 	enic->num_vfs = 0;
-	enic_admin_channel_close(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close admin channel: %d\n", close_err);
 free_vf_state:
 	kfree(enic->vf_state);
 	enic->vf_state = NULL;
+	/* The reset worker only converges the independent datapath to a stopped
+	 * state after terminal admin-queue failure; it does not issue a reset.
+	 */
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    netif_running(enic->netdev))
+		schedule_work(&enic->reset);
 	return err;
 }
 
 static void enic_sriov_v2_disable(struct enic *enic)
 {
+	int close_err;
+
 	/* Stop new VF link-state broadcasts before tearing down vf_state.
 	 * Clearing ENIC_SRIOV_ENABLED makes enic_link_check() (called from
 	 * the notify timer/ISR) skip the VF notify path, and cancelling
@@ -3056,10 +3113,16 @@ static void enic_sriov_v2_disable(struct enic *enic)
 	cancel_work_sync(&enic->link_notify_work);
 
 	pci_disable_sriov(enic->pdev);
-	enic_admin_channel_close(enic);
+	close_err = enic_admin_channel_close(enic);
+	if (close_err)
+		netdev_err(enic->netdev,
+			   "Failed to close admin channel: %d\n", close_err);
 	kfree(enic->vf_state);
 	enic->vf_state = NULL;
 	enic->num_vfs = 0;
+	if (enic_admin_channel_is_quarantined(enic) &&
+	    netif_running(enic->netdev))
+		schedule_work(&enic->reset);
 }
 
 /*
@@ -3346,7 +3409,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		if (err) {
 			dev_err(dev,
 				"Failed to open admin channel: %d\n", err);
-			goto err_out_dev_deinit;
+			goto err_out_admin_fence;
 		}
 		err = enic_mbox_vf_capability_check(enic);
 		if (err) {
@@ -3491,6 +3554,8 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 err_out_admin_close:
 	if (enic_is_sriov_vf_v2(enic)) {
+		int close_err;
+
 		if (READ_ONCE(enic->vf_registered)) {
 			int unreg_err = enic_mbox_vf_unregister(enic);
 
@@ -3499,12 +3564,24 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 					    "Failed to unregister from PF: %d\n",
 					    unreg_err);
 		}
-		enic_admin_channel_close(enic);
+		close_err = enic_admin_channel_close(enic);
+		if (close_err)
+			netdev_err(netdev,
+				   "Failed to close admin channel: %d\n",
+				   close_err);
+	}
+err_out_admin_fence:
+	if (enic_dma_quarantined(enic)) {
+		dev_err(dev,
+			"Queue DMA ownership unresolved; retaining probe resources until reboot\n");
+		enic_terminal_dma_quarantine(enic);
+		pci_disable_device(pdev);
+		return err;
 	}
-err_out_dev_deinit:
 	enic_dev_deinit(enic);
 err_out_dev_close:
 	vnic_dev_close(enic->vdev);
+	goto err_out_disable_sriov;
 err_out_disable_sriov:
 	kfree(enic->pp);
 err_out_disable_sriov_pp:
@@ -3528,6 +3605,12 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return err;
 }
 
+static bool enic_dma_quarantined(struct enic *enic)
+{
+	return enic_datapath_is_quarantined(enic) ||
+	       enic_admin_channel_is_quarantined(enic);
+}
+
 static int enic_datapath_recheck_on_remove(struct enic *enic)
 {
 	int disable_err = 0;
@@ -3555,7 +3638,8 @@ static int enic_datapath_recheck_on_remove(struct enic *enic)
 
 	dev_info(&enic->pdev->dev,
 		 "All datapath queues stopped at remove; releasing DMA graph\n");
-	enic_datapath_release(enic);
+	enic_datapath_release(enic,
+			      !enic_admin_channel_is_quarantined(enic));
 
 	return 0;
 }
@@ -3568,6 +3652,7 @@ static void enic_terminal_dma_quarantine(struct enic *enic)
 	 * ordinary stop path can return before freeing IRQs when a queue does
 	 * not stop, so each ownership-aware operation must be idempotent.
 	 */
+	WRITE_ONCE(enic->mbox_send_disabled, true);
 	for (i = 0; i < enic->intr_count; i++) {
 		vnic_intr_mask(&enic->intr[i]);
 		(void)vnic_intr_masked(&enic->intr[i]);
@@ -3604,6 +3689,8 @@ static void enic_remove(struct pci_dev *pdev)
 		 * touching a netdev that is being torn down.
 		 */
 		if (enic_is_sriov_vf_v2(enic)) {
+			int close_err;
+
 			if (READ_ONCE(enic->vf_registered)) {
 				int unreg_err = enic_mbox_vf_unregister(enic);
 
@@ -3612,7 +3699,11 @@ static void enic_remove(struct pci_dev *pdev)
 						    "Failed to unregister from PF: %d\n",
 						    unreg_err);
 			}
-			enic_admin_channel_close(enic);
+			close_err = enic_admin_channel_close(enic);
+			if (close_err)
+				netdev_warn(netdev,
+					    "Admin close returned: %d\n",
+					    close_err);
 		}
 
 		unregister_netdev(netdev);
@@ -3630,9 +3721,11 @@ static void enic_remove(struct pci_dev *pdev)
 				pci_disable_sriov(pdev);
 		}
 #endif
+		if (enic_admin_channel_is_quarantined(enic))
+			enic_admin_channel_recheck_on_remove(enic);
 		if (enic_datapath_is_quarantined(enic))
 			enic_datapath_recheck_on_remove(enic);
-		if (enic_datapath_is_quarantined(enic)) {
+		if (enic_dma_quarantined(enic)) {
 			dev_err(&pdev->dev,
 				"Queue DMA ownership unresolved; retaining resources until reboot\n");
 			enic_terminal_dma_quarantine(enic);
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 5c93ca49552a..e106ee7d411d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -150,6 +150,7 @@ static int enic_mbox_send_msg_id(struct enic *enic, u8 msg_type,
 		 * Mark the channel unusable so no further sends are attempted.
 		 */
 		WRITE_ONCE(enic->mbox_send_disabled, true);
+		WRITE_ONCE(enic->mbox_tx_poisoned, true);
 	}
 
 	netdev_dbg(enic->netdev,

-- 
2.43.0