Re: [PATCH net-next v3 11/13] net: mana: keep a user-configured RSS table across a queue rebuild

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

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

- [Medium] In mana_alloc_queues() the loss is reported before the driver
  state it describes is updated: ethtool_rxfh_indir_lost(ndev) is called…
- [Low] Commit message and the in-code comment describe a narrower rule
  than the code implements.
- [Low] The patch is a user-visible bug fix (a user-installed `ethtool
  -X` indirection table was silently replaced by the driver default on…

--

net: mana: keep a user-configured RSS table across a queue rebuild

The patch stops mana from unconditionally regenerating the RSS indirection
table when the queues are rebuilt, so a table installed with "ethtool -X"
survives a reconfiguration.  A table that cannot be applied to the new
queue count is reported to the core with ethtool_rxfh_indir_lost(), and for
the swap path that report is deferred until the new set is live.

A couple of questions below about the described condition, the missing
Fixes: tag, and the ordering of the loss report in mana_alloc_queues().

Carry it over instead when the queue count is unchanged: the entries are
queue indices, so they stay meaningful.

[Severity: Low]
Is "when the queue count is unchanged" the condition the code actually
implements?  mana_rss_table_keep() tests netif_is_rxfh_configured() plus a
per-entry bounds check against the requested count:

	if (!netif_is_rxfh_configured(apc->ndev))
		return false;

	for (i = 0; i < apc->indir_table_sz; i++) {
		if (apc->indir_table[i] >= num_queues) {

and mana_alloc_qset() passes the new count, which mana_set_channels() only
reaches when new_count differs from apc->num_queues.

So "ethtool -L" also carries a user table over now instead of rebuilding
it.  Growing 4 -> 8 combined channels leaves every old entry in range, the
table is copied verbatim, and the four new RX queues get no steered
traffic.  mana_get_tx_queue() indexes apc->indir_table[] too, so the new TX
queues stay unused as well until the user re-runs "ethtool -X".

The semantics match what other drivers do (ice, mlx5, bnxt, idpf and others
skip default regeneration when netif_is_rxfh_configured() is set), so could
the changelog state the real predicate (entries still in range for the new
count) and mention the "ethtool -L" effect?

[Severity: Low]
Should this carry a Fixes: tag?  In the baseline tree mana_alloc_queues()
called mana_rss_table_init(apc) unconditionally, so every detach/attach
cycle (reset recovery, resume, MTU/ringparam/channel/priv-flag/XDP
reconfiguration) overwrote apc->indir_table with the driver default while
the core kept IFF_RXFH_CONFIGURED set, since netif_is_rxfh_configured()
reads dev->ethtool->rss_indir_user_size and the driver never cleared it.

That is a user-visible loss of configuration, which makes this part a fix.
The unconditional initialization came in with ca9c54d2d6a5 ("net: mana: Add
a driver for Microsoft Azure Network Adapter (MANA)"), which also added
mana_set_rxfh().  Would a Fixes: trailer, or splitting the
mana_alloc_queues() change out as a standalone fix, be appropriate here?

[ ... ]

[Severity: Medium]
Can this report the wrong table to userspace?  ethtool_rxfh_indir_lost() is
not just a flag clear:

net/ethtool/common.c:ethtool_rxfh_indir_lost() {
	...
	dev->ethtool->rss_indir_user_size = 0;
	ethtool_rss_notify(dev, ETHTOOL_MSG_RSS_NTF, 0);
	...
}

That notification re-enters the driver synchronously:

ethtool_rss_notify()->ethnl_notify()->ethnl_default_notify()->
  rss_prepare_data()->rss_prepare()->rss_prepare_get()

net/ethtool/rss.c:rss_prepare_get() {
	...
	ret = ops->get_rxfh(dev, &rxfh);
	...
}

and mana_get_rxfh() copies apc->indir_table[] verbatim.  At that point
mana_rss_table_init(apc) has not run yet, so the broadcast RSS_NTF carries
the table that mana_rss_table_keep() just rejected (it has at least one
entry >= apc->num_queues), and mana_config_rss() has not programmed
anything.  No further notification is emitted once the default table is
installed, so a listener caching notification state keeps the discarded
table.

Reachable in the documented case: mana_init_port() clamps apc->num_queues
down to a smaller device-reported max after a reset, and a user table with
a higher entry then makes mana_rss_table_keep() set *lost.

Would swapping the two statements, so the loss is reported after
mana_rss_table_init(apc), be better?  That matches the ordering of the
other new call site, which reports only once the new set is live:

	if (newq->rxfh_indir_lost)
		ethtool_rxfh_indir_lost(ndev);

 
 	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
 	if (err) {

[ ... ]

[ ... ]

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