Re: [PATCH net] net: mana: do not discard an ethtool-configured RSS table on a queue rebuild

[Jakub Kicinski]

On Fri,  4 Sep 2026 17:44:01 -0700 Long Li wrote:
> mana_alloc_queues() regenerates the RSS indirection table from the driver
> default every time the queues are built, so a table installed with
> "ethtool -X" is silently replaced by any operation that rebuilds them:
> an MTU change, a ring-size, channel-count or private-flag change, an XDP
> attach, TX-timeout reset recovery, or resume.
> 
> The driver never clears the core's IFF_RXFH_CONFIGURED, so
> netif_is_rxfh_configured() keeps reporting a user table while the
> hardware has been reprogrammed with the default one. "ethtool -x" then
> shows a table the user did not ask for, with no indication it changed:
> 
>   # ethtool -X ens1 equal 2
>   # ethtool -x ens1
>   RX flow hash indirection table for ens1 with 16 RX ring(s):
>       0:      0     1     0     1     0     1     0     1
>       8:      0     1     0     1     0     1     0     1
>   # ip link set ens1 mtu 1400
>   # ethtool -x ens1
>   RX flow hash indirection table for ens1 with 16 RX ring(s):
>       0:      0     1     2     3     4     5     6     7
>       8:      8     9    10    11    12    13    14    15
> 
> Keep the table instead, and rebuild it only when it is driver-generated
> or cannot be honoured. An entry may not be kept if it points past the
> last queue: mana_config_rss() uses these entries to index apc->rxqs[],
> which holds apc->num_queues pointers. That is reachable because
> mana_attach() calls mana_init_port(), which lowers apc->num_queues to the
> maximum the device reports, so a table configured for more queues can
> outlive them.

To be clear this is only acceptable if the number of queues drops due 
to re-negotiation of caps with the device, not for example if XDP
requires some queues to be used for other purposes. In the latter case
just refuse the config change.

> A table that cannot be kept is still replaced by the default silently,
> without ethtool_rxfh_indir_lost(). That helper sends
> ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock, and
> mana_alloc_queues() runs both with that lock held, from ndo_open, and
> without it, from mana_attach() on the reset and resume paths. Leaving
> the core's view untouched is what the driver did for every table before
> this change.

Okay, so you have a problem of not having the lock...

> Opt the RSS ethtool operations into rtnl_lock() while here. Reading the
> table in mana_alloc_queues() has to be serialized against mana_set_rxfh()
> replacing it, and the two had no lock in common: mana_set_rxfh() ran under
> the netdev instance lock alone, while mana_alloc_queues() reaches this
> point holding only RTNL, from ndo_open and from mana_attach() on the reset
> and resume paths. Taking the instance lock there instead is not possible,
> since ndo_open already runs with it held. The same flag covers the netlink
> and ioctl entry points.

.. and yet your fix is not to try to take it but the reverse, to add 
a different lock? You need to explain why reset path can't take the
instance lock. Of course you can't take it _inside_ ndo_open, but the
caller should be able to.
 
> Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
> Signed-off-by: Long Li <longli@xxxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 37 ++++++++++++++++++-
>  .../ethernet/microsoft/mana/mana_ethtool.c    |  3 +-
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index 7a1ac853e3abcd28c4a1e5c6987ec631a18ad840..97386e17b9421aedefa25bab6fbf0b7b1f2996d4 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -3306,6 +3306,28 @@ static void mana_rss_table_init(struct mana_port_context *apc)
>  			ethtool_rxfh_indir_default(i, apc->num_queues);
>  }
>  
> +/* Whether the current indirection table can be kept for apc->num_queues,
> + * rather than rebuilt from the driver default.
> + *
> + * Only a user table ("ethtool -X") is worth keeping; a driver-generated one
> + * is rebuilt so that it spreads over every queue. An entry pointing past the
> + * last queue cannot be kept: mana_config_rss() uses these entries to index
> + * apc->rxqs[], which holds apc->num_queues pointers.
> + */
> +static bool mana_rss_table_keep(struct mana_port_context *apc)
> +{
> +	u32 i;
> +
> +	if (!netif_is_rxfh_configured(apc->ndev))
> +		return false;
> +
> +	for (i = 0; i < apc->indir_table_sz; i++)
> +		if (apc->indir_table[i] >= apc->num_queues)
> +			return false;
> +
> +	return true;
> +}
> +
>  int mana_disable_vport_rx(struct mana_port_context *apc)
>  {
>  	return mana_cfg_vport_steering(apc, TRI_STATE_FALSE, false, false,
> @@ -3621,7 +3643,20 @@ int mana_alloc_queues(struct net_device *ndev)
>  		goto destroy_rxq;
>  	}
>  
> -	mana_rss_table_init(apc);
> +	/* Keep a user-configured table across the rebuild: its entries are
> +	 * queue indices and stay meaningful while they are all still in range.
> +	 * Only a driver-generated table is regenerated here.
> +	 *
> +	 * A table that cannot be kept is replaced by the default without
> +	 * telling the core, which keeps reporting the table as user
> +	 * configured. That is what this function did for every table before,
> +	 * and reporting it here is not an option: ethtool_rxfh_indir_lost()
> +	 * sends ETHTOOL_MSG_RSS_NTF, which requires the netdev instance lock,
> +	 * and this runs both with that lock held (ndo_open) and without it
> +	 * (mana_attach() from the reset and resume paths).
> +	 */
> +	if (!mana_rss_table_keep(apc))
> +		mana_rss_table_init(apc);
>  
>  	err = mana_config_rss(apc, TRI_STATE_TRUE, true, true);
>  	if (err) {
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> index ece7ff9cc409a806b6a6de70a85b44874bfa6dad..e3acaf17efabcd62f7cffabdb8404cf16b917272 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
> @@ -871,7 +871,8 @@ const struct ethtool_ops mana_ethtool_ops = {
>  	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
>  				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
>  				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS |
> -				  ETHTOOL_OP_NEEDS_RTNL_GLINK,
> +				  ETHTOOL_OP_NEEDS_RTNL_GLINK |
> +				  ETHTOOL_OP_NEEDS_RTNL_RSS,
>  	.get_ethtool_stats	= mana_get_ethtool_stats,
>  	.get_sset_count		= mana_get_sset_count,
>  	.get_strings		= mana_get_strings,