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

[Sahil Chandna <sahilchandna@linux.microsoft.com>]

On 26-10-2026 13:51, netdev-bot+sashiko@kernel.org wrote:
> 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?
> 
Ack, will fix the hwc->sema based wait in v2.>> +
>>  	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);
>> +		}
> 
Ack, will add bounded wait in v2.> [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;
>> +			}
>>  		}
>>  	}
>
Ack, I will check above two comments and see if these can be fixed in v2.
> [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?
> 
> [ ... ]
> 
pw-bot: cr