Re: [PATCH net] net: mana: Avoid waiting on dead HWC after device reset

[netdev-bot+sashiko@kernel.org]

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

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

- [High] mana_dealloc_queues()
  (drivers/net/ethernet/microsoft/mana/mana_en.c) no longer waits for TX…
- [Medium] If mana_gd_dev_reset() fails without quiescing the device,
  mana_dealloc_queues() only logs the error.
- [Medium] The new fail-fast check in mana_hwc_send_request() comes after
  mana_hwc_get_msg_index(), which calls down(&hwc->sema) unconditionally.
- [Medium] mana_dealloc_queues() sets hwc_dead only when
  mana_gd_dev_reset() returns 0.
- [Low] Several comments still say the DIM counters cq->tx_dim_pkts and
  cq->tx_dim_bytes are touched only in NAPI context, which the patch…
- [Low] The comment in mana_destroy_txq() (mana_en.c, about line 2728)
  says 'Forget the SKBs freed without being completed after a drain…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the timeout
  path in mana_hwc_send_request() does a non-atomic check-then-write:…

--

net: mana: Avoid waiting on dead HWC after device reset

This commit replaces the 120 second sleep-based TX drain in
mana_dealloc_queues() with a single synchronous CQ sweep done while NAPI
is disabled. If sends are still pending after the sweep, it resets the
device. A successful reset sets a new hwc_dead flag, which makes
mana_hwc_send_request() fail fast with -ETIMEDOUT and skips
mana_fence_rqs() and the SHM teardown handshake.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 3bca4b683134b..c9337f49fbe15 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -897,6 +903,17 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  	dest_vrq = hwc->dest_vrq_id;
>  	dest_vrcq = hwc->dest_vrcq_id;
>  
> +	/* If the HWC is unavailable (e.g. hwc_timeout was set to 0 or
> +	 * hwc_dead set after device reset), do not post the WQE: the
> +	 * hardware cannot respond, so fail fast instead of waiting out
> +	 * the full timeout.
> +	 */
> +	timeout = READ_ONCE(hwc->hwc_timeout);
> +	if (timeout == 0 || READ_ONCE(hwc->hwc_dead)) {
> +		err = -ETIMEDOUT;
> +		goto out;
> +	}

[Severity: Medium]
Is this check reached early enough to avoid the RTNL stall after a reset?

Before getting here, mana_hwc_send_request() calls mana_hwc_get_msg_index(),
which does an unconditional down(&hwc->sema). The HWC has a single request
slot (HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH = 1).

Some HWC users run without RTNL, for example
mana_gf_stats_work_handler()->mana_query_gf_stats() every 2 seconds, and
mana_ib verbs. Suppose one of them has already taken the slot, passed this
check, and is sitting in wait_for_completion_timeout() with the full
timeout (60s on the tested system). The reset in mana_dealloc_queues()
then guarantees that request never completes.

The teardown thread then does:

mana_dealloc_queues()
  mana_disable_vport_rx()
    ...
      mana_hwc_send_request()
        mana_hwc_get_msg_index()
          down(&hwc->sema);    <-- waits out the other request's timeout

Setting hwc_dead does not wake the waiter that is already in flight. Can
RTNL still be held here for the full hwc_timeout, which is the symptom the
commit message says is fixed?

> +
>  	err = mana_hwc_post_tx_wqe(txq, tx_wr, dest_vrq, dest_vrcq, false);
>  	if (err) {
>  		dev_err(hwc->dev, "HWC: Failed to post send WQE: %d\n", err);
> @@ -904,14 +921,13 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>  	}
>  
>  	if (!wait_for_completion_timeout(&ctx->comp_event,
> -					 (msecs_to_jiffies(hwc->hwc_timeout)))) {
> -		if (hwc->hwc_timeout != 0)
> -			dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> -				command, hwc->hwc_timeout);
> +					 (msecs_to_jiffies(timeout)))) {
> +		dev_err(hwc->dev, "Command 0x%x timed out: %u ms\n",
> +			command, timeout);
>  
>  		/* Reduce further waiting if HWC no response */
> -		if (hwc->hwc_timeout > 1)
> -			hwc->hwc_timeout = 1;
> +		if (READ_ONCE(hwc->hwc_timeout) > 1)
> +			WRITE_ONCE(hwc->hwc_timeout, 1);

[Severity: Low]
This isn't a bug introduced by this patch, but this read-then-write is not
atomic. If mana_serv_reset() runs WRITE_ONCE(hwc->hwc_timeout, 0) between
the READ_ONCE() and the WRITE_ONCE() here, the 0 is overwritten with 1.
An HWC_DATA_CFG_HWC_TIMEOUT update from mana_hwc_init_event_handler() can
be lost the same way.

The new fail-fast check depends on timeout == 0. With the 0 lost, later
commands would post WQEs and wait about a jiffy each instead of failing
immediately. Now that 0 means fail fast, would a cmpxchg() be worth using
here?

[ ... ]

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 6b7c027e497ed..00885d21a9af0 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c

[ ... ]

> @@ -3774,42 +3777,92 @@ static int mana_dealloc_queues(struct net_device *ndev)

[ ... ]

>  	if (apc->tx_qp) {
>  		for (i = 0; i < apc->num_queues; i++) {

[ ... ]

>  			txq = &apc->tx_qp[i]->txq;
> -			tsleep = 1000;
> -			while (atomic_read(&txq->pending_sends) > 0 &&
> -			       time_before(jiffies, timeout)) {
> -				usleep_range(tsleep, tsleep + 1000);
> -				tsleep <<= 1;
> +			napi = &tx_cq->napi;
> +
> +			if (txq->napi_initialized) {
> +				/* Stop the NAPI poll so it cannot run
> +				 * concurrently with the synchronous drain.
> +				 */
> +				napi_synchronize(napi);
> +				napi_disable_locked(napi);
> +				cancel_work_sync(&tx_cq->dim.work);
> +
> +				/* Reap all TX completions currently in the CQ.
> +				 * A zero budget frees skbs via the non-NAPI
> +				 * path, which is required outside softirq.
> +				 */
> +				tx_cq->budget = 0;
> +				do {
> +					tx_cq->work_done = 0;
> +					mana_poll_tx_cq(tx_cq);
> +				} while (tx_cq->work_done);
>  			}

[Severity: Low]
This isn't a bug, but this adds a process context caller of
mana_poll_tx_cq(), which still updates cq->tx_dim_pkts and
cq->tx_dim_bytes. The struct mana_cq comment in include/net/mana/mana.h
says:

	/* Cumulative TX completions fed to DIM. Updated and read only in
	 * NAPI context (mana_poll_tx_cq() / mana_update_tx_dim()), so they
	 * measure the hardware completion rate and need no u64_stats_sync.
	 */

mana_poll_tx_cq() and mana_update_tx_dim() have similar comments. NAPI is
disabled and dim.work is cancelled before this loop runs. Should those
comments be updated to mention this drain path?

> -			if (atomic_read(&txq->pending_sends)) {
> -				err = mana_gd_dev_reset(gd->gdma_context);
> -				if (err) {
> -					netdev_err(ndev, "device reset failed %d with %d pkts pending in txq %u\n",
> -						   err,
> -					    atomic_read(&txq->pending_sends),
> -					    txq->gdma_txq_id);
> -				}
> -				break;
> +
> +			pending = atomic_read(&txq->pending_sends);
> +			if (!trigger_flr && pending > 0) {
> +				trigger_flr = true;
> +				flr_txq_id = txq->gdma_txq_id;
> +				flr_pending = pending;
>  			}
>  		}

[Severity: High]
Can this reset a healthy device during routine reconfiguration?

The do/while loop above stops as soon as mana_gd_poll_cq() finds no CQE
already written. NAPI is disabled by then, so a completion that arrives
later is never reaped. Before this patch the driver waited with NAPI
enabled, for up to 120s, before treating pending_sends as a hardware
failure.

mana_detach() calls this right after stopping TX:

mana_detach()
	netif_tx_disable(ndev);

	if (apc->port_st_save) {
		err = mana_dealloc_queues(ndev);

The SQ can hold up to tx_queue_size WQEs (default 256, max 16384), and
each can be a 64KB TSO frame. Under TX load, pending_sends > 0 after one
sweep looks likely on a working device, which leads to:

mana_change_mtu() / mana_close() / mana_set_channels() / mana_xdp_set() ...
  mana_detach()
    mana_dealloc_queues()
      mana_gd_dev_reset()      <-- pcie_flr() on PCI, cdx_dev_reset() on CDX
      WRITE_ONCE(hwc->hwc_dead, true);
  mana_attach()
    ...
      mana_hwc_send_request()  <-- -ETIMEDOUT since hwc_dead is set

The FLR also takes down the HWC, the EQs, the vPorts of every port on the
function and the mana_ib device. None of them is notified.

On PCI, recovery only comes indirectly: the gf_stats worker sees
-ETIMEDOUT and schedules mana_serv_reset(), which sleeps for
MANA_SERVICE_PERIOD seconds. CDX has no service reset path, so the port
stays down until reprobe.

The commit message says the device is reset "only if sends remain
unacknowledged". After this change, that means "not yet completed at the
instant of the sweep". Should there be a bounded wait for pending_sends
before deciding the hardware is unresponsive, for example polling the CQ
with a timeout?

> +		if (trigger_flr && !(hwc && hwc->hwc_dead)) {
> +			/* Completions never arrived: the device is
> +			 * unresponsive. Issue device reset and
> +			 * mark the HWC dead so subsequent HWC commands
> +			 * fail fast instead of each waiting the full
> +			 * hwc_timeout.
> +			 */
> +			err = mana_gd_dev_reset(gd->gdma_context);
> +			if (err)
> +				netdev_err(ndev, "device reset failed %d with %d pkts pending in txq %u\n",
> +					   err,
> +					   flr_pending,
> +					   flr_txq_id);
> +			else if (hwc)
> +				WRITE_ONCE(hwc->hwc_dead, true);
> +		}

[Severity: Medium]
Is it safe to leave hwc_dead clear whenever mana_gd_dev_reset() returns an
error?

On PCI the reset is mana_pci_dev_reset()->pcie_flr(). Once pcie_flr() gets
past its early checks, it always issues the reset and then waits for the
device:

	pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
	...
	msleep(100);

	ret = pci_dev_wait(dev, "FLR", PCIE_RESET_READY_POLL_MS);

If the device is not ready within about 60s, pci_dev_wait() fails. The
function has been reset, but hwc_dead stays false and hwc_timeout keeps
the firmware value.

The next command, MANA_CONFIG_VPORT_RX from mana_disable_vport_rx(), then
passes the new check in mana_hwc_send_request(). It posts to the reset
function and waits the full timeout with RTNL held. mana_fence_rqs() and
the SHM teardown handshake also still run.

A device that has stopped producing TX completions seems the most likely
one to also come back late. Could this path keep the stall the commit
message describes, on top of the pci_dev_wait() time?

>  		for (i = 0; i < apc->num_queues; i++) {

[ ... ]

>  			txq = &apc->tx_qp[i]->txq;
>  			while ((skb = skb_dequeue(&txq->pending_skbs))) {
>  				mana_unmap_skb(skb, apc);
>  				dev_kfree_skb_any(skb);
>  			}
>  			atomic_set(&txq->pending_sends, 0);

[Severity: Medium]
What happens here if mana_gd_dev_reset() failed without quiescing the
device?

In that case the error is only logged. These SKBs are then unmapped and
freed while the SQ is still live in hardware; the SQ is only destroyed
later, in mana_destroy_txq()->mana_destroy_wq_obj().

pcie_flr() can return before issuing the FLR:

	ret = pci_dev_reset_iommu_prepare(dev);
	if (ret) {
		pci_err(dev, "failed to stop IOMMU for a PCI reset: %d\n", ret);
		return ret;
	}

On CDX, cdx_dev_reset() can likewise fail in dev_configure().

Before this patch, this sequence only ran after up to 120s of waiting, by
which point a working device had finished. With the single sweep,
outstanding WQEs on a healthy device are normal.

Can the device then DMA from unmapped or freed TX buffers? Under an IOMMU
that would fault. Without strict IOMMU, it could send the new contents of
reused pages.

> +
> +			if (txq->napi_initialized) {
> +				netif_napi_del_locked(napi);
> +				txq->napi_initialized = false;
> +			}
>  		}
>  	}

[Severity: Low]
This isn't a bug, but since NAPI is now deleted here and napi_initialized
is cleared, the NAPI teardown block in mana_destroy_txq() no longer runs
on this path. Its comment also refers to the drain timeout that this patch
removes:

		/* Forget the SKBs freed without being completed after a drain
		 * timeout, now that NAPI can't complete any. Doing it here also
		 * covers the queues that a lower channel count won't recreate,
		 * which could otherwise stay stopped.
		 */
		netdev_tx_reset_queue(apc->tx_qp[i]->txq.net_txq);

Should this comment be updated to match the new drain?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025072838.1991785-1-sahilchandna%40linux.microsoft.com