Re: [PATCH net-next v3 04/13] net: mana: swap queue sets in mana_set_ringparam

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The changelog of "net: mana: swap queue sets in
  mana_set_ringparam" states: "This drops the fallback on failure.

--

net: mana: swap queue sets in mana_set_ringparam

This converts the ethtool ring size path to build a new queue set,
publish it with mana_publish_qset() and retire the old one, instead of
running mana_detach()/mana_attach() over the live configuration. It also
returns early when the rounded sizes already match the current ones.

> This drops the fallback on failure. Previously a failed mana_attach() was
> retried with the previous values, or the defaults, or the minimums, so a
> user who asked for a specific size could end up with a different one with
> no indication beyond dmesg. There is nothing to recover from now, so the
> error is returned.

[Severity: Medium]
Does the retry chain described here actually exist in the code being
replaced?  In the pre-image of mana_set_ringparam() a failed mana_attach()
only writes the old sizes back into apc and returns the error:

	err = mana_attach(ndev);
	if (err) {
		netdev_err(ndev, "mana_attach failed: %d\n", err);
		apc->tx_queue_size = old_tx;
		apc->rx_queue_size = old_rx;
	}
out:
	mana_pre_dealloc_rxbufs(apc);
	return err;

mana_attach() in mana_en.c makes a single attempt:

	err = mana_init_port(ndev);
	if (err)
		return err;

	if (apc->port_st_save) {
		err = mana_alloc_queues(ndev);
		if (err) {
			mana_cleanup_port_context(apc);
			return err;
		}
	}

There is no second attach at the previous values, the defaults or the
minimums; the only other writers of apc->tx_queue_size /
apc->rx_queue_size in the driver are the one-time defaults set in
mana_probe_port().

Also, the restored size fields do not describe a working port after that
failure: mana_detach(ndev, false) already ran, so
mana_dealloc_queues() -> mana_destroy_vport() -> mana_uncfg_vport() has
released the vport and the interface stays detached until an explicit
down/up, with the vport claimable by RDMA in the meantime.

Could the changelog be reworded to describe what the old path really did,
so the connectivity aspect of this change is not presented as a cosmetic
"user may end up with a different size" cleanup?

> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index 2ef3d461e7072..24353dfddde3c 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c

[ ... ]

> @@ -790,32 +788,61 @@ static int mana_set_ringparam(struct net_device *ndev,
>  	netdev_info(ndev, "Using nearest power of 2 values for Txq:%d Rxq:%d\n",
>  		    new_tx, new_rx);
>  
> -	/* pre-allocating new buffers to prevent failures in mana_attach() later */
> -	apc->rx_queue_size = new_rx;
> -	err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> -	apc->rx_queue_size = old_rx;
> -	if (err) {
> -		netdev_err(ndev, "Insufficient memory for new allocations\n");
> -		return err;
> +	if (new_rx == apc->rx_queue_size && new_tx == apc->tx_queue_size)
> +		return 0;

[ ... ]

> -	err = mana_attach(ndev);
> +	err = mana_publish_qset(apc, &newq, &oldq);
>  	if (err) {
> -		netdev_err(ndev, "mana_attach failed: %d\n", err);
> -		apc->tx_queue_size = old_tx;
> -		apc->rx_queue_size = old_rx;
> +		NL_SET_ERR_MSG_FMT(extack, "failed to change ring params: %d",
> +				   err);
> +		mana_free_qset(scratch, &newq);
> +		goto free_scratch;
>  	}
> -out:
> -	mana_pre_dealloc_rxbufs(apc);

[ ... ]

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