On Mon, Oct 14, 2024 at 03:23:49PM -0700, Jacob Keller wrote:
>
>
> 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.dre...@intel.com>
> >>>> Signed-off-by: Michal Swiatkowski <michal.swiatkow...@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!
> >
>
> Yea, that makes sense. This is definitely not going to behave well in
> the event that one of the values is above 16-bit.
>
I blindly copied that, thanks for pointing it, will fix in next version.
Thanks,
Michal
> > 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
