Re: [Intel-wired-lan] [iwl-next v4 3/8] ice: get rid of num_lan_msix field

[David Laight <David.Laight@ACULAB.COM>]

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@intel.com>
> >> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.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!

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

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)