Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: set max queues in alloc_etherdev_mqs()

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/24/26 11:03, Paul Menzel wrote:
> Dear Michal,
> 
> 
> Thank you for the patch.
> 
> Am 23.02.26 um 13:51 schrieb Michal Swiatkowski:
>> When allocating netdevice using alloc_etherdev_mqs() the maximum
>> supported queues number should be passed. The vsi->alloc_txq/rxq is
>> storing current number of queues, not the maximum ones.
>>
>> Use the same function for getting max Tx and Rx queues which is used
>> during ethtool -l call to set maximum number of queues during netdev
>> allocation.
>>
>> Reproduction steps:
>> $ethtool -l $pf # says current 16, max 64
>> $ethtool -S $pf # fine
>> $ethtool -L $pf combined 40 # crash
>>
>> [491187.472594] Call Trace:
>> [491187.472829]  <TASK>
>> [491187.473067]  netif_set_xps_queue+0x26/0x40
>> [491187.473305]  ice_vsi_cfg_txq+0x265/0x3d0 [ice]
>> [491187.473619]  ice_vsi_cfg_lan_txqs+0x68/0xa0 [ice]
>> [491187.473918]  ice_vsi_cfg_lan+0x2b/0xa0 [ice]
>> [491187.474202]  ice_vsi_open+0x71/0x170 [ice]
>> [491187.474484]  ice_vsi_recfg_qs+0x17f/0x230 [ice]
>> [491187.474759]  ? dev_get_min_mp_channel_count+0xab/0xd0
>> [491187.474987]  ice_set_channels+0x185/0x3d0 [ice]
>> [491187.475278]  ethnl_set_channels+0x26f/0x340
>>
>> Fixes: ee13aa1a2c5a ("ice: use netif_get_num_default_rss_queues()")
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice.h         | 22 ++++++++++++++++++++
>>   drivers/net/ethernet/intel/ice/ice_ethtool.c | 18 ----------------
>>   drivers/net/ethernet/intel/ice/ice_main.c    |  4 ++--
>>   3 files changed, 24 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ 
>> ethernet/intel/ice/ice.h
>> index f6a56a864459..725b130dd3a2 100644
>> --- a/drivers/net/ethernet/intel/ice/ice.h
>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>> @@ -839,6 +839,28 @@ static inline void ice_tx_xsk_pool(struct ice_vsi 
>> *vsi, u16 qid)
>>       WRITE_ONCE(ring->xsk_pool, ice_get_xp_from_qid(vsi, qid));
>>   }
>> +/**
>> + * ice_get_max_txq - return the maximum number of Tx queues for in a PF
>> + * @pf: PF structure
>> + *
>> + * Return: maximum number of Tx queues
>> + */
>> +static inline int ice_get_max_txq(struct ice_pf *pf)
>> +{
>> +    return min(num_online_cpus(), pf->hw.func_caps.common_cap.num_txq);
>> +}
>> +
>> +/**
>> + * ice_get_max_rxq - return the maximum number of Rx queues for in a PF
>> + * @pf: PF structure
>> + *
>> + * Return: maximum number of Rx queues
>> + */
>> +static inline int ice_get_max_rxq(struct ice_pf *pf)
>> +{
>> +    return min(num_online_cpus(), pf->hw.func_caps.common_cap.num_rxq);
>> +}
>> +
>>   /**
>>    * ice_get_main_vsi - Get the PF VSI
>>    * @pf: PF instance
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/ 
>> net/ethernet/intel/ice/ice_ethtool.c
>> index a897a6b22495..a0b0416f5aea 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> @@ -3780,24 +3780,6 @@ ice_get_ts_info(struct net_device *dev, struct 
>> kernel_ethtool_ts_info *info)
>>       return 0;
>>   }
>> -/**
>> - * ice_get_max_txq - return the maximum number of Tx queues for in a PF
>> - * @pf: PF structure
>> - */
>> -static int ice_get_max_txq(struct ice_pf *pf)
>> -{
>> -    return min(num_online_cpus(), pf->hw.func_caps.common_cap.num_txq);
>> -}
>> -
>> -/**
>> - * ice_get_max_rxq - return the maximum number of Rx queues for in a PF
>> - * @pf: PF structure
>> - */
>> -static int ice_get_max_rxq(struct ice_pf *pf)
>> -{
>> -    return min(num_online_cpus(), pf->hw.func_caps.common_cap.num_rxq);
>> -}
>> -
>>   /**
>>    * ice_get_combined_cnt - return the current number of combined 
>> channels
>>    * @vsi: PF VSI pointer
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ 
>> ethernet/intel/ice/ice_main.c
>> index fa4095037be5..863ac81eebce 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -4699,8 +4699,8 @@ static int ice_cfg_netdev(struct ice_vsi *vsi)
>>       struct net_device *netdev;
>>       u8 mac_addr[ETH_ALEN];
>> -    netdev = alloc_etherdev_mqs(sizeof(*np), vsi->alloc_txq,
>> -                    vsi->alloc_rxq);
>> +    netdev = alloc_etherdev_mqs(sizeof(*np), ice_get_max_txq(vsi->back),
>> +                    ice_get_max_rxq(vsi->back));
>>       if (!netdev)
>>           return -ENOMEM;
> 
> Should also some checks be added to `ethnl_set_channels()` to avoid 
> crashes?

alloc_etherdev_mqs() gets absolute max from the driver, it will not
even call driver .set_channels() if user tries to exceed that

with that said, we have too much "extra checks" in this handler ;)

> 
> The commit looks good:
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> I am missing the mentioning of the crash in commit message title/ 
> summary, but I do not have a better suggestion right now.
> 
> 
> Kind regards,
> 
> Paul