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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] Commit message describes a change the patch does not make:
  "Scheduling queue_reset_work() on failure goes with it, leaving the TX…
- [Low] Understated failure impact in the commit message: "a failure
  returns the error with both the queues and the flag word untouched"…

Pre-existing issues:
- [Low] Claimed comment-vs-code mismatch: the kernel-doc comment on
  struct mana_port_context::channel_changing states it is "Set by…

--

net: mana: swap queue sets in mana_set_priv_flags

This converts the MANA_PRIV_FLAG_USE_FULL_PAGE_RXBUF toggle from an
in-place detach/attach cycle into a pre-allocate-then-swap using
mana_qset_scratch_alloc(), mana_alloc_qset(), mana_publish_qset() and
mana_free_qset().  The flag value is passed into the new queue set instead
of being written to apc->priv_flags before the rebuild, and the existing
shortcuts (down port, jumbo MTU, attached XDP program) are folded into one
condition.

No Link: tags are present in the commit header.

A couple of questions about the commit message and one note on an existing
comment follow.

The commit message says:

    Scheduling queue_reset_work() on failure goes with it, leaving the TX
    timeout handler as its only user.

[Severity: Medium]
Is this description accurate for this tree?

The pre-patch mana_set_priv_flags() in mana_ethtool.c does not schedule
queue_reset_work() at all - the removed code only calls
mana_pre_alloc_rxbufs(), mana_detach(), mana_attach() and
mana_pre_dealloc_rxbufs().  A grep for queue_reset_work at the parent
revision matches only mana_en.c and mana.h, never mana_ethtool.c, so there
appears to be nothing of that kind to remove here.

After the patch, queue_reset_work() still has three queue_work() sites in
mana_en.c:

  mana_tx_timeout()
        queue_work(ac->per_port_queue_reset_wq, &apc->queue_reset_work);

  mana_dealloc_queues()
        netdev_err(ac->ports[i],
                   "queues reset by a sibling port, scheduling rebuild\n");
        queue_work(ac->per_port_queue_reset_wq,
                   &sib->queue_reset_work);

  mana_free_qset()
        netdev_err(scratch->ndev,
                   "device reset while retiring a queue set, scheduling port reset\n");
        queue_work(ac->per_port_queue_reset_wq,
                   &sib->queue_reset_work);

The last one is reached from the new success and publish-failure paths added
by this patch, so is the TX timeout handler really the only user now?
Could this paragraph be dropped or reworded?

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

[ ... ]

> @@ -882,54 +886,53 @@ static int mana_set_priv_flags(struct net_device *ndev, u32 priv_flags)

[ ... ]

> -		/* Block RDMA from grabbing the vport during detach/attach */
> -		mutex_lock(&apc->vport_mutex);
> -		apc->channel_changing = true;
> +	/* Block RDMA from acquiring the vport for the duration. */
> +	mutex_lock(&apc->vport_mutex);
> +	if (apc->channel_changing) {
>  		mutex_unlock(&apc->vport_mutex);
> +		return -EBUSY;
> +	}
> +	apc->channel_changing = true;
> +	mutex_unlock(&apc->vport_mutex);

[Severity: Low]
This isn't a bug introduced by this patch, but the kernel-doc comment on
channel_changing in struct mana_port_context (include/net/mana/mana.h) is
getting further out of date:

        /* Set by mana_set_channels() under vport_mutex to block RDMA
         * from grabbing the vport during the detach/attach window.
         * Checked by mana_cfg_vport() when called from the RDMA path.
         */
        bool channel_changing;

It already named only mana_set_channels() at the baseline while
mana_set_priv_flags() and mana_per_port_queue_reset_work_handler() also set
the flag, and the region guarded here is now a pre-allocate/swap rather than
a detach/attach window.  Would it make sense to refresh that comment along
with this conversion?

> -		err = mana_pre_alloc_rxbufs(apc, ndev->mtu, apc->num_queues);
> -		if (err) {
> -			netdev_err(ndev,
> -				   "Insufficient memory for new allocations\n");
> -			apc->priv_flags = old_priv_flags;
> -			goto clear_flag;
> -		}
> +	scratch = mana_qset_scratch_alloc(apc);
> +	if (!scratch) {
> +		err = -ENOMEM;
> +		goto clear_flag;
> +	}
>  
> -		err = mana_detach(ndev, false);
> -		if (err) {
> -			netdev_err(ndev, "mana_detach failed: %d\n", err);
> -			apc->priv_flags = old_priv_flags;
> -			goto out;
> -		}
> +	err = mana_alloc_qset(apc, scratch, apc->num_queues, apc->rx_queue_size,
> +			      apc->tx_queue_size, priv_flags, &newq);
> +	if (err)
> +		goto free_scratch; /* current qset and priv_flags untouched */
>  
> -		err = mana_attach(ndev);
> -		if (err) {
> -			netdev_err(ndev, "mana_attach failed: %d\n", err);
> -			apc->priv_flags = old_priv_flags;
> -		}
> +	err = mana_publish_qset(apc, &newq, &oldq);
> +	if (err) {
> +		mana_free_qset(scratch, &newq);
> +		goto free_scratch;
>  	}
>  
> -out:
> -	mana_pre_dealloc_rxbufs(apc);
> +	mana_free_qset(scratch, &oldq);
> +
> +free_scratch:
> +	mana_publish_close_if_needed(apc);
> +	mana_qset_scratch_free(scratch);

[Severity: Low]
The commit message states:

    a failure returns the error with both the queues and the flag word
    untouched

Does that hold for a failure inside mana_publish_qset()?

mana_publish_qset() can fail on mana_config_rss(), which is an HWC command
and can fail on an unhealthy device.  Its rollback re-runs
mana_config_rss(), and if that fails too it calls mana_publish_give_up():

        apc->carrier_forced_off = netif_carrier_ok(apc->ndev);
        netif_carrier_off(apc->ndev);
        apc->publish_dead_end = true;

mana_set_priv_flags() then reaches free_scratch and calls
mana_publish_close_if_needed(apc) unconditionally, which does:

        if (mana_dealloc_queues(apc->ndev))
                netdev_err(apc->ndev,
                           "failed to close the port after a failed rollback\n");

So ethtool --set-priv-flags can return an error while the port is left
closed with the carrier down until a manual down/up.  The flag word does
look preserved (mana_qset_install() restores qset->priv_flags), so only the
queues half of the claim seems affected.  Could the guarantee in the commit
message be qualified for the dead-end rollback case?

> clear_flag:
>  	mutex_lock(&apc->vport_mutex);
>  	apc->channel_changing = false;
>  	mutex_unlock(&apc->vport_mutex);
> -
>  	return err;
>  }

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