Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh
> -Original Message- > From: "Loktionov, Aleksandr" > To: "Loktionov, Aleksandr" , > Kohei Enju > Cc: "[email protected]" , > "Nguyen, Anthony L" , > "[email protected]" , > "[email protected]" , > "[email protected]" > , > "[email protected]" , > "[email protected]" , > "[email protected]" , > "Kitszel, Przemyslaw" , > "[email protected]" > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring > RSS key via ethtool set_rxfh > Date: Thu, 8 Jan 2026 13:03:12 + [thread overview] > Message-ID: > > (raw) > In-Reply-To: > > > > > > -Original Message- > > From: Intel-wired-lan On Behalf > > Of Loktionov, Aleksandr > > Sent: Thursday, January 8, 2026 1:28 PM > > To: Kohei Enju > > Cc: [email protected]; Nguyen, Anthony L > > ; [email protected]; > > [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; Kitszel, > > Przemyslaw ; [email protected] > > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow > > configuring RSS key via ethtool set_rxfh > > > > > > > > > -Original Message- > > > From: Kohei Enju > > > Sent: Thursday, January 8, 2026 1:04 PM > > > To: Loktionov, Aleksandr > > > Cc: [email protected]; Nguyen, Anthony L > > > ; [email protected]; > > > [email protected]; [email protected]; intel-wired- > > > [email protected]; [email protected]; [email protected]; > > > [email protected]; Kitszel, Przemyslaw > > ; > > > [email protected] > > > 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 +, 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 > > > > Afterthought, I think it could be nice to place ASSERT_RTNL() to show it > explicitly. > What do you think about this idea? Sorry for the late reply. I think it's a good idea. I will add ASSERT_RTNL().
Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh
On Thu, 8 Jan 2026 14:20:15 +0900 Takashi Kozu wrote: > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c > b/drivers/net/ethernet/intel/igb/igb_main.c > index da0f550de605..d42b3750f0b1 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -4526,7 +4526,8 @@ static void igb_setup_mrqc(struct igb_adapter *adapter) > u32 mrqc, rxcsum; > u32 j, num_rx_queues; > > - netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key)); > + if (!adapter->has_user_rss_key) > + netdev_rss_key_fill(adapter->rss_key, sizeof(adapter->rss_key)); > igb_write_rss_key(adapter); This is an unusual construct. adapter->rss_key is driver state, does something wipe it? It's normal to have to write the key into the device after reset but initializing the driver state is usually done at probe, just once. Then you don't have to worry whether the state is coming from random or user. Note that netdev_rss_key_fill() initializes its state once per boot so it will not change its 'return' value without reboot. Last but not least - would you be able to run: tools/testing/selftests/drivers/net/hw/toeplitz.py tools/testing/selftests/drivers/net/hw/rss_api.py against this device? Some more help: https://github.com/linux-netdev/nipa/wiki/Running-driver-tests
Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh
> -Original Message- > From: Intel-wired-lan On Behalf > Of Loktionov, Aleksandr > Sent: Thursday, January 8, 2026 1:28 PM > To: Kohei Enju > Cc: [email protected]; Nguyen, Anthony L > ; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; Kitszel, > Przemyslaw ; [email protected] > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow > configuring RSS key via ethtool set_rxfh > > > > > -Original Message- > > From: Kohei Enju > > Sent: Thursday, January 8, 2026 1:04 PM > > To: Loktionov, Aleksandr > > Cc: [email protected]; Nguyen, Anthony L > > ; [email protected]; > > [email protected]; [email protected]; intel-wired- > > [email protected]; [email protected]; [email protected]; > > [email protected]; Kitszel, Przemyslaw > ; > > [email protected] > > 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 +, 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 > Afterthought, I think it could be nice to place ASSERT_RTNL() to show it explicitly. What do you think about this idea?
Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh
> -Original Message- > From: Kohei Enju > Sent: Thursday, January 8, 2026 1:04 PM > To: Loktionov, Aleksandr > Cc: [email protected]; Nguyen, Anthony L > ; [email protected]; > [email protected]; [email protected]; intel-wired- > [email protected]; [email protected]; [email protected]; > [email protected]; Kitszel, Przemyslaw ; > [email protected] > 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 +, 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 > > > > >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?
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 +, 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 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?
Re: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow configuring RSS key via ethtool set_rxfh
> -Original Message- > From: Intel-wired-lan On Behalf > Of Takashi Kozu > Sent: Thursday, January 8, 2026 6:20 AM > To: Nguyen, Anthony L > Cc: Kitszel, Przemyslaw ; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; Takashi Kozu ; Kohei Enju > > Subject: [Intel-wired-lan] [PATCH iwl-next v2 3/3] igb: allow > configuring RSS key via ethtool set_rxfh > > Change igb_set_rxfh() to accept and save a userspace-provided RSS key. > When a key is provided, store it in the adapter and write the > E1000 registers accordingly. > > This can be tested using `ethtool -X hkey `. > > Tested-by: Kohei Enju > Signed-off-by: Takashi Kozu > --- > drivers/net/ethernet/intel/igb/igb.h | 1 + > drivers/net/ethernet/intel/igb/igb_ethtool.c | 49 +++ > - > drivers/net/ethernet/intel/igb/igb_main.c| 3 +- > 3 files changed, 30 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb.h > b/drivers/net/ethernet/intel/igb/igb.h > index 8c9b02058cec..2509ec30acf3 100644 > --- a/drivers/net/ethernet/intel/igb/igb.h > +++ b/drivers/net/ethernet/intel/igb/igb.h > @@ -657,6 +657,7 @@ struct igb_adapter { > u32 rss_indir_tbl_init; > u8 rss_indir_tbl[IGB_RETA_SIZE]; > u8 rss_key[IGB_RSS_KEY_SIZE]; > + bool has_user_rss_key; > > unsigned long link_check_timeout; > int copper_tries; > diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c > b/drivers/net/ethernet/intel/igb/igb_ethtool.c > index 2953d079ebae..ac045fbebade 100644 > --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c > +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c > @@ -3345,35 +3345,40 @@ static int igb_set_rxfh(struct net_device > *netdev, > u32 num_queues; > > /* We do not allow change in unsupported parameters */ > - if (rxfh->key || > - (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && > - rxfh->hfunc != ETH_RSS_HASH_TOP)) > + if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE && > + rxfh->hfunc != ETH_RSS_HASH_TOP) > return -EOPNOTSUPP; > - if (!rxfh->indir) > - return 0; > > - num_queues = adapter->rss_queues; > + if (rxfh->indir) { > + num_queues = adapter->rss_queues; > > - switch (hw->mac.type) { > - case e1000_82576: > - /* 82576 supports 2 RSS queues for SR-IOV */ > - if (adapter->vfs_allocated_count) > - num_queues = 2; > - break; > - default: > - break; > - } > + switch (hw->mac.type) { > + case e1000_82576: > + /* 82576 supports 2 RSS queues for SR-IOV */ > + if (adapter->vfs_allocated_count) > + num_queues = 2; > + break; > + default: > + break; > + } > > - /* Verify user input. */ > - for (i = 0; i < IGB_RETA_SIZE; i++) > - if (rxfh->indir[i] >= num_queues) > - return -EINVAL; > + /* Verify user input. */ > + for (i = 0; i < IGB_RETA_SIZE; i++) > + if (rxfh->indir[i] >= num_queues) > + return -EINVAL; > > > - for (i = 0; i < IGB_RETA_SIZE; i++) > - adapter->rss_indir_tbl[i] = rxfh->indir[i]; > + for (i = 0; i < IGB_RETA_SIZE; i++) > + adapter->rss_indir_tbl[i] = rxfh->indir[i]; > + > + igb_write_rss_indir_tbl(adapter); > + } > > - 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. 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? > + } > > return 0; > } > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c > b/drivers/net/ethernet/intel/igb/igb_main.c > index da0f550de605..d42b3750f0b1 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -4526,7 +4526,8 @@ static void igb_setup_mrqc(struct igb_adapter > *adapter) > u32 mrqc, rxcsum; > u32 j, num_rx_queues; > > - netdev_rss_key_fill(adapter->rss_key, sizeof(adapter- > >rss_key)); > + if (!adapter->has_user_rss_key) > + netdev_rss_key_fill(adapter->rss_key, sizeof(adapter- >
