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

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

On 26-10-2026 12:59, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] net: mana: non-waiting CQ poll in mana_dealloc_queues() triggers false FLR
> - [Medium] net: mana: mana_hwc_send_request() blocks on hwc->sema before checking hwc_dead
> --
> 
> commit 7823c203253d16c1fb640cce922ae30200b9f486
> Author: Sahil Chandna <sahilchandna@linux.microsoft.com>
> 
> net: mana: Avoid waiting on dead HWC after device reset
> 
> This commit replaces the sleep-based TX queue drain in mana_dealloc_queues()
> with synchronous CQE polling and marks the hardware channel dead via hwc_dead
> after a device reset so subsequent HWC commands fail fast with -ETIMEDOUT.
> 
>> 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
> 
> [ ... ]
> 
>> @@ -862,6 +867,7 @@ int mana_hwc_send_request(struct hw_channel_context *hwc, u32 req_len,
>>  	u32 dest_vrcq;
>>  	u32 dest_vrq;
>>  	u32 command;
>> +	u32 timeout = 0;
>>  	u16 msg_id;
>>  	int err;
>>  
>> @@ -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]
> Can mana_hwc_send_request() still block for the full hwc_timeout on
> hwc->sema before reaching this hwc_dead check?
> 
> At the start of mana_hwc_send_request(), mana_hwc_get_msg_index() is called:
> 
>     mana_hwc_send_request()
>       -> mana_hwc_get_msg_index()
>            -> down(&hwc->sema);
> 
> Because hwc->sema is initialized to 1 (HW_CHANNEL_VF_BOOTSTRAP_QUEUE_DEPTH),
> only one HWC command can be in flight at a time.
> 
> While the device is active, periodic work such as
> mana_gf_stats_work_handler() (running every 2 seconds without holding
> rtnl_lock) or auxiliary RDMA (mana_ib) callers can enter
> mana_hwc_send_request(), acquire hwc->sema, and sleep in
> wait_for_completion_timeout(&ctx->comp_event, ...) for the full 60s
> hwc_timeout when the hardware stops responding.
> 
> When mana_dealloc_queues() resets the device and sets hwc->hwc_dead = true,
> it does not complete any in-flight ctx->comp_event, so the subsequent
> mana_disable_vport_rx() -> mana_hwc_send_request() call in
> mana_dealloc_queues() will still block in down(&hwc->sema) while holding
> rtnl_lock until the concurrent caller's timeout expires.
> 
> Should hwc_timeout == 0 || hwc_dead also be checked before acquiring
> hwc->sema (or should in-flight waiters be completed when marking HWC dead)?
> 
Ack, this is valid, I will fix this in v2.>> 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
>> @@ -3753,16 +3753,19 @@ int mana_attach(struct net_device *ndev)
>>  static int mana_dealloc_queues(struct net_device *ndev)
>>  {
>>  	struct mana_port_context *apc = netdev_priv(ndev);
>> -	unsigned long timeout = jiffies + 120 * HZ;
>>  	struct gdma_dev *gd = apc->ac->gdma_dev;
>> +	struct hw_channel_context *hwc;
>> +	bool trigger_flr = false;
>> +	int flr_pending, i, err;
>>  	struct mana_txq *txq;
>>  	struct sk_buff *skb;
>> -	int i, err;
>> -	u32 tsleep;
>> +	u32 flr_txq_id = 0;
>>  
>>  	if (apc->port_is_up)
>>  		return -EINVAL;
>>  
>> +	hwc = gd->gdma_context->hwc.driver_data;
>> +
>>  	if (apc->rxqs)
>>  		mana_chn_setxdp(apc, NULL);
>>  
>> @@ -3774,42 +3777,92 @@ static int mana_dealloc_queues(struct net_device *ndev)
>>  	 * a txq because it may not timely see apc->port_is_up being cleared
>>  	 * to false, but it doesn't matter since mana_start_xmit() drops any
>>  	 * new packets due to apc->port_is_up being false.
>> -	 *
>> -	 * Drain all the in-flight TX packets.
>> -	 * A timeout of 120 seconds for all the queues is used.
>> -	 * This will break the while loop when h/w is not responding.
>> -	 * This value of 120 has been decided here considering max
>> -	 * number of queues.
>> +	 * Drain the in-flight TX completions for each queue. TX is already
>> +	 * stopped, so pending_sends can no longer grow. For each queue,
>> +	 * disable NAPI so it cannot run at the same time as this drain,
>> +	 * then process all completions still in the CQ. If some are still
>> +	 * missing after that, the hardware is not responding: reset device
>> +	 * and stop waiting on the now-unavailable HWC.
>>  	 */
>>  
>>  	if (apc->tx_qp) {
>>  		for (i = 0; i < apc->num_queues; i++) {
>> +			struct mana_cq *tx_cq;
>> +			struct napi_struct *napi;
>> +			int pending;
>> +
>> +			if (!apc->tx_qp[i])
>> +				continue;
>> +
>> +			tx_cq = &apc->tx_qp[i]->tx_cq;
>>  			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);
>>  			}
>> -			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;
>>  			}
>>  		}
>>  
>> +		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: High]
> Won't this non-waiting CQ poll trigger a premature Function Level Reset and
> permanently mark the HWC dead whenever TX packets are still in flight in the
> hardware during a normal detach or per-port queue reset?
> 
> In mana_detach(), setting apc->port_is_up = false and calling
> netif_tx_disable(ndev) prevents new packets from being queued in software,
> but packets posted to txq->gdma_sq right before netif_tx_disable() are still
> asynchronously in flight in the NIC hardware (DMA read, shaping, wire
> transmission, and CQE DMA write-back).
> 
> mana_poll_tx_cq() is non-blocking:
> 
>     mana_poll_tx_cq()
>       comp_read = mana_gd_poll_cq(cq->gdma_cq, completions, ...);
>       if (comp_read < 1)
>               return;
> 
> As soon as mana_gd_poll_cq() reaches a CQE slot that the hardware has not
> yet written back to host memory, it returns 0, mana_poll_tx_cq() returns
> early with tx_cq->work_done == 0, and the do-while loop exits immediately
> without waiting for the in-flight SQ entries to complete.
> 
> If any packet is still in flight on the NIC when the loop exits,
> atomic_read(&txq->pending_sends) > 0 sets trigger_flr = true, which calls
> mana_gd_dev_reset() and sets hwc->hwc_dead = true on healthy hardware.
> 
> Because hwc->hwc_dead is never cleared without destroying and recreating the
> HWC channel, any operation that detaches and reattaches queues under traffic
> (such as mana_close() followed by mana_open(), mana_change_mtu(),
> mana_set_channels(), mana_set_ringparam(), mana_set_priv_flags(), or
> mana_xdp_set()) will cause subsequent mana_attach() or mana_open() calls and
> any other vPorts or RDMA instances on the function to fail permanently with
> -ETIMEDOUT.
> 
> Similarly, single-port TX timeout recovery in
> mana_per_port_queue_reset_work_handler() runs when a TX queue is stalled
> (so pending_sends > 0 holds by definition):
> 
>     mana_per_port_queue_reset_work_handler()
>       -> mana_detach(ndev, false)
>            -> mana_dealloc_queues(ndev)
>       -> mana_attach(ndev)
> 
> Because mana_dealloc_queues() now resets the entire PCIe function and sets
> hwc->hwc_dead = true, the recovery call to mana_attach(ndev) immediately
> fails with -ETIMEDOUT instead of recovering the port.
> 
> Furthermore, if mana_gd_dev_reset() fails or is skipped because hwc_dead was
> already set, the cleanup loop immediately below unmaps and frees the
> in-flight TX SKBs via mana_unmap_skb() and dev_kfree_skb_any() without
> having waited for DMA completion or reset the device.
> 
Ack, this would need a bounded wait to verify queue is really stuck
rather than depend only on pending_sends, I will fix this in v2.