Re: [PATCH net-next v10 4/4] net: mana: add dynamic HWC queue depth with reinit path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: mana: duplicate mana_hwc_teardown_queues() on HWC
  rebuild failure
--

commit 63009cdb17b7f248623a5c752d26416fb4226096
Author: Long Li <longli@microsoft.com>

net: mana: add dynamic HWC queue depth with reinit path

This commit adds dynamic HWC queue depth negotiation by rebuilding the
bootstrap hardware channel when the PF reports a supported queue depth
between 2 and 128, with fallback to fresh bootstrap queues if the rebuild
fails.

Link: https://lore.kernel.org/r/20260908035201.402424-5-longli@microsoft.com
Link: https://lore.kernel.org/r/179196575846.434549.7933818740976196422@kernel.org

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 8bc00001d20e5..867f8dd3ea547 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -1088,16 +1176,11 @@ static void mana_hwc_fence_channel(struct gdma_context *gc,
>  		mana_hwc_unpublish_cq(gc, hwc->cq->gdma_cq);
>  }
>  
> -static bool mana_hwc_release_channel(struct hw_channel_context *hwc)
> +static int mana_hwc_teardown_queues(struct hw_channel_context *hwc)
>  {
>  	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
>  	int err;
>  
> -	mana_hwc_stop_channel(hwc);
> -
> -	/* An unacknowledged destroy leaves the PF's mappings live.  Fence
> -	 * software dispatch, but retain every PF-visible allocation.
> -	 */
>  	err = mana_smc_teardown_hwc(&gc->shm_channel, false,
>  				    &hwc->setup_active);
>  	if (err) {
> @@ -1105,28 +1188,26 @@ static bool mana_hwc_release_channel(struct hw_channel_context *hwc)
>  			"HWC teardown failed: %d, retaining PF-visible resources\n",
>  			err);
>  		mana_hwc_fence_channel(gc, hwc);
> -		mana_hwc_retain_channel(hwc);
> -		return false;
> +		return err;
>  	}
>  
> -	mana_hwc_clear_cq_table(gc);
> -
> -	/* Fence the EQ before releasing the remaining HWC state. */
> -	if (hwc->cq)
> -		mana_hwc_destroy_cq(hwc->gdma_dev->gdma_context, hwc->cq);
> -
> -	if (hwc->txq)
> -		mana_hwc_destroy_wq(hwc, hwc->txq);
> +	mana_hwc_destroy_queues(hwc);
>  
> -	if (hwc->rxq)
> -		mana_hwc_destroy_wq(hwc, hwc->rxq);
> +	return 0;
> +}
>  
> -	kfree(hwc->caller_ctx);
> -	hwc->caller_ctx = NULL;
> +static bool mana_hwc_release_channel(struct hw_channel_context *hwc)
> +{
> +	struct gdma_context *gc = hwc->gdma_dev->gdma_context;
> +	int err;
>  
> -	mana_gd_free_res_map(&hwc->inflight_msg_res);
> +	mana_hwc_stop_channel(hwc);
>  
> -	hwc->num_inflight_msg = 0;
> +	err = mana_hwc_teardown_queues(hwc);
> +	if (err) {
> +		mana_hwc_retain_channel(hwc);
> +		return false;
> +	}

[ ... ]

> +static int mana_hwc_rebuild_channel(struct hw_channel_context *hwc,
> +				    u16 q_depth,
> +				    struct mana_hwc_init_report *report)
> +{
> +	u32 timeout_ms = mana_hwc_timeout_read(hwc);
> +	int cleanup_err;
> +	int err;
> +
> +	err = mana_hwc_teardown_queues(hwc);
> +	if (err)
> +		return err;
> +
> +	err = mana_hwc_build_channel(hwc, q_depth, report);
> +	if (!err)
> +		err = mana_hwc_validate_report(hwc, report, q_depth,
> +					       true, true);
> +	if (!err)
> +		err = mana_hwc_finish_channel(hwc, report);
> +	if (!err)
> +		return 0;
> +
> +	dev_warn(hwc->dev,
> +		 "HWC depth %u rebuild failed, restoring bootstrap: %d\n",
> +		 q_depth, err);
> +
> +	cleanup_err = mana_hwc_teardown_queues(hwc);
> +	if (cleanup_err)
> +		return cleanup_err;

[ ... ]

> @@ -1234,7 +1452,7 @@ int mana_hwc_create_channel(struct gdma_context *gc)
>  	gd->pdid = INVALID_PDID;
>  	gd->doorbell = INVALID_DOORBELL;
>  
> -	err = mana_hwc_create_bootstrap_channel(hwc);
> +	err = mana_hwc_prepare_channel(hwc);
>  	if (err) {
>  		mana_hwc_release_channel(hwc);
>  		return err;

[Severity: Medium]
Does mana_hwc_release_channel() issue a duplicate mana_hwc_teardown_queues()
when mana_hwc_rebuild_channel() fails inside mana_hwc_teardown_queues()?

mana_smc_teardown_hwc_locked() only clears hwc->setup_active when the SMC
teardown transaction succeeds.  If mana_hwc_teardown_queues() fails in
mana_hwc_rebuild_channel(), it fences the channel and returns the error with
hwc->setup_active still true, and the error path in mana_hwc_create_channel()
then calls mana_hwc_release_channel() -> mana_hwc_teardown_queues() a second
time on the same channel.  On an unresponsive shared-memory channel this
stalls for a second 20-40s poll timeout; if the PF completes the first
SMC_MSG_TYPE_DESTROY_HWC right after the first read times out and rejects the
second destroy with a status error, mana_hwc_release_channel() retains the
already-destroyed channel.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794208055.git.weh@linux.microsoft.com?part=4