netdev - Re: [iwl-next v4 3/8] ice: get rid of num_lan

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <534ab479-29a1-4a95-a9e1-d068b5290ebd@intel.com>
Date: Mon, 14 Oct 2024 15:23:49 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: David Laight <David.Laight@...LAB.COM>, Simon Horman <horms@...nel.org>,
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"pawel.chmielewski@...el.com" <pawel.chmielewski@...el.com>,
	"sridhar.samudrala@...el.com" <sridhar.samudrala@...el.com>,
	"pio.raczynski@...il.com" <pio.raczynski@...il.com>,
	"konrad.knitter@...el.com" <konrad.knitter@...el.com>,
	"marcin.szycik@...el.com" <marcin.szycik@...el.com>,
	"wojciech.drewek@...el.com" <wojciech.drewek@...el.com>,
	"nex.sw.ncis.nat.hpm.dev@...el.com" <nex.sw.ncis.nat.hpm.dev@...el.com>,
	"przemyslaw.kitszel@...el.com" <przemyslaw.kitszel@...el.com>,
	"jiri@...nulli.us" <jiri@...nulli.us>
Subject: Re: [iwl-next v4 3/8] ice: get rid of num_lan_msix field



On 10/14/2024 12:04 PM, David Laight wrote:
> From: Jacob Keller
>> Sent: 14 October 2024 19:51
>>
>> On 10/12/2024 8:13 AM, Simon Horman wrote:
>>> + David Laight
>>>
>>> On Mon, Sep 30, 2024 at 02:03:57PM +0200, Michal Swiatkowski wrote:
>>>> Remove the field to allow having more queues than MSI-X on VSI. As
>>>> default the number will be the same, but if there won't be more MSI-X
>>>> available VSI can run with at least one MSI-X.
>>>>
>>>> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>>>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>>>> ---
>>>>  drivers/net/ethernet/intel/ice/ice.h         |  1 -
>>>>  drivers/net/ethernet/intel/ice/ice_base.c    | 10 +++-----
>>>>  drivers/net/ethernet/intel/ice/ice_ethtool.c |  8 +++---
>>>>  drivers/net/ethernet/intel/ice/ice_irq.c     | 11 +++------
>>>>  drivers/net/ethernet/intel/ice/ice_lib.c     | 26 +++++++++++---------
>>>>  5 files changed, 27 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
>>>> index cf824d041d5a..1e23aec2634f 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>>> @@ -622,7 +622,6 @@ struct ice_pf {
>>>>  	u16 max_pf_txqs;	/* Total Tx queues PF wide */
>>>>  	u16 max_pf_rxqs;	/* Total Rx queues PF wide */
>>>>  	struct ice_pf_msix msix;
>>>> -	u16 num_lan_msix;	/* Total MSIX vectors for base driver */
>>>>  	u16 num_lan_tx;		/* num LAN Tx queues setup */
>>>>  	u16 num_lan_rx;		/* num LAN Rx queues setup */
>>>>  	u16 next_vsi;		/* Next free slot in pf->vsi[] - 0-based! */
>>>
>>> ...
>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>> b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>>> index 85a3b2326e7b..e5c56ec8bbda 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
>>>> @@ -3811,8 +3811,8 @@ ice_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info)
>>>>   */
>>>>  static int ice_get_max_txq(struct ice_pf *pf)
>>>>  {
>>>> -	return min3(pf->num_lan_msix, (u16)num_online_cpus(),
>>>> -		    (u16)pf->hw.func_caps.common_cap.num_txq);
>>>> +	return min_t(u16, num_online_cpus(),
>>>> +		     pf->hw.func_caps.common_cap.num_txq);
>>>
>>> It is unclear why min_t() is used here or elsewhere in this patch
>>> instead of min() as it seems that all the entities being compared
>>> are unsigned. Are you concerned about overflowing u16? If so, perhaps
>>> clamp, or some error handling, is a better approach.
>>>
>>> I am concerned that the casting that min_t() brings will hide
>>> any problems that may exist.
>>>
>> Ya, I think min makes more sense. min_t was likely selected out of habit
>> or looking at other examples in the driver.
> 
> My 'spot patches that use min_t()' failed to spot that one.
> 
> But it is just plain wrong - and always was.
> You want a result that is 16bits, casting the inputs is wrong.
> Consider a system with 64k cpus!
> 

Yea, that makes sense. This is definitely not going to behave well in
the event that one of the values is above 16-bit.

> Pretty much all the min_t() that specify u8 or u16 are likely to
> be actually broken.
> Most of the rest specify u32 or u64 in order to compare (usually)
> unsigned values of different sizes.
> But I found some that might be using 'long' on 64bit values
> on 32bit (and as disk sector numbers!).
> 
> In the current min() bleats, the code is almost certainly awry.
> 
> 	David