[Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Kohei Enju <enjuk at amazon.com>
> Sent: Thursday, January 8, 2026 1:04 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov at intel.com>
> Cc: andrew+netdev at lunn.ch; Nguyen, Anthony L
> <anthony.l.nguyen at intel.com>; davem at davemloft.net;
> edumazet at google.com; enjuk at amazon.com; intel-wired-
> lan at lists.osuosl.org; kuba at kernel.org; netdev at vger.kernel.org;
> pabeni at redhat.com; Kitszel, Przemyslaw <przemyslaw.kitszel at intel.com>;
> takkozu at amazon.com
> Subject: Re: RE: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow
> configuring RSS key via ethtool set_rxfh
> 
> On Thu, 8 Jan 2026 07:29:19 +0000, Loktionov, Aleksandr wrote:
> 
> >>
> >> -	igb_write_rss_indir_tbl(adapter);
> >> +	if (rxfh->key) {
> >> +		adapter->has_user_rss_key = true;
> >> +		memcpy(adapter->rss_key, rxfh->key, sizeof(adapter-
> >> >rss_key));
> >> +		igb_write_rss_key(adapter);
> >It leads to race between ethtool RSS update and concurrent resets.
> >Because igb_setup_mrqc() (called during resets) also calls
> igb_write_rss_key(adapter).
> >Non-fatal but breaks RSS configuration guarantees.
> 
> At my first glance, rtnl lock serializes those operation, so it
> doesn't seem to be racy as long as they are under the rtnl lock.
> 
> As far as I skimmed the codes, functions such as igb_open()/
> igb_up()/igb_reset_task(), which finally call igb_write_rss_key() are
> serialized by rtnl lock or serializes igb_write_rss_key() call by
> locking rtnl.
> 
> Please let me know if I'm missing something and it's truly racy.
I think you're right, and I've missed that missing rtnl_lock was added in upstream.

Thank you for clarification
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov at intel.com>

> 
> >
> >I think ethtool can/should wait of reset/watchdog task to finish.
> >I'm against adding locks, and just my personal opinion, it's better
> to implement igb_rss_key_update_task() in addition to reset and
> watchdog tasks to be used both in reset and ethtool path.
> >
> >What do you think?