Re: [Intel-wired-lan] [iwl-next v4 3/8] ice: get rid of num_lan_msix field
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
> >> Signed-off-by: Michal Swiatkowski
> >> ---
> >> 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)
Re: [Intel-wired-lan] [iwl-next v6 2/9] ice: devlink PF MSI-X max and min parameter
From: Michal Swiatkowski
> Sent: 04 November 2024 07:03
...
> > The type of the devlink parameters msix_vec_per_pf_{min,max} is
> > specified as u32, so you must use value.vu32 everywhere you work with
> > them, not vu16.
> >
>
> I will change it.
You also need a pretty good reason to use u16 anywhere at all.
Just because the domain of the value is small doesn't mean the
best type isn't [unsigned] int.
Any arithmetic (particularly on non x86) is likely to increase
the code size above any perceived data saving.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [Intel-wired-lan] [PATCH iwl-net] ice: health.c: fix compilation on gcc 7.5
On Wed, 5 Feb 2025 20:45:46 +
Simon Horman wrote:
> + Jiri
>
> On Wed, Feb 05, 2025 at 11:42:12AM +0100, Przemek Kitszel wrote:
> > GCC 7 is not as good as GCC 8+ in telling what is a compile-time const,
> > and thus could be used for static storage. So we could not use variables
> > for that, no matter how much "const" keyword is sprinkled around.
> >
> > Excerpt from the report:
> > My GCC is: gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0.
> >
> > CC [M] drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.o
> > drivers/net/ethernet/intel/ice/devlink/health.c:35:3: error: initializer
> > element is not constant
> >ice_common_port_solutions, {ice_port_number_label}},
> >^
> > drivers/net/ethernet/intel/ice/devlink/health.c:35:3: note: (near
> > initialization for 'ice_health_status_lookup[0].solution')
> > drivers/net/ethernet/intel/ice/devlink/health.c:35:31: error: initializer
> > element is not constant
> >ice_common_port_solutions, {ice_port_number_label}},
> >^
> > drivers/net/ethernet/intel/ice/devlink/health.c:35:31: note: (near
> > initialization for 'ice_health_status_lookup[0].data_label[0]')
> > drivers/net/ethernet/intel/ice/devlink/health.c:37:46: error: initializer
> > element is not constant
> >"Change or replace the module or cable.", {ice_port_number_label}},
> > ^
> > drivers/net/ethernet/intel/ice/devlink/health.c:37:46: note: (near
> > initialization for 'ice_health_status_lookup[1].data_label[0]')
> > drivers/net/ethernet/intel/ice/devlink/health.c:39:3: error: initializer
> > element is not constant
> >ice_common_port_solutions, {ice_port_number_label}},
> >^
> >
> > Fixes: 85d6164ec56d ("ice: add fw and port health reporters")
> > Reported-by: Qiuxu Zhuo
> > Closes:
> > https://lore.kernel.org/netdev/cy8pr11mb7134bf7a46d71e50d25fa7a989...@cy8pr11mb7134.namprd11.prod.outlook.com
> > Signed-off-by: Przemek Kitszel
> > ---
> > I would really like to bump min gcc to 8.5 (RH 8 family),
> > instead of supporting old Ubuntu. However SLES 15 is also stuck with gcc
> > 7.5 :(
> >
> > CC: Linus Torvalds
> > CC: Kees Cook
> > CC: Nick Desaulniers
>
> Hi Prezemek,
>
> I ran into a similar problem not so long ago and I'm wondering if
> the following, based on a suggestion by Jiri Slaby, resolves your
> problem.
I'm sure I remember from somewhere that although the variables are
'static const' they have to be real variables because they can still
be patched.
Which stops you using their contents as initialisers.
Maybe I'm mis-remembering it.
David
>
> diff --git a/drivers/net/ethernet/intel/ice/devlink/health.c
> b/drivers/net/ethernet/intel/ice/devlink/health.c
> index ea40f7941259..19c3d37aa768 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/health.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/health.c
> @@ -25,10 +25,10 @@ struct ice_health_status {
> * The below lookup requires to be sorted by code.
> */
>
> -static const char *const ice_common_port_solutions =
> +static const char ice_common_port_solutions[] =
> "Check your cable connection. Change or replace the module or cable.
> Manually set speed and duplex.";
> -static const char *const ice_port_number_label = "Port Number";
> -static const char *const ice_update_nvm_solution = "Update to the latest NVM
> image.";
> +static const char ice_port_number_label[] = "Port Number";
> +static const char ice_update_nvm_solution[] = "Update to the latest NVM
> image.";
>
> static const struct ice_health_status ice_health_status_lookup[] = {
> {ICE_AQC_HEALTH_STATUS_ERR_UNKNOWN_MOD_STRICT, "An unsupported module
> was detected.",
>
>
> Link:
> https://lore.kernel.org/netdev/485dbc5a-a04b-40c2-9481-955eaa5ce...@kernel.org/
> Link: https://git.kernel.org/netdev/net-next/c/36fb51479e3c
>
Re: [Intel-wired-lan] [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform
On Wed, 25 Jun 2025 15:05:01 +0200 Jacek Kowalski wrote: > +#define NVM_CHECKSUM_FACTORY_DEFAULT 0x > >>> > >>> Perhaps it is too long, but I liked Vlad's suggestion of naming this > >>> NVM_CHECKSUM_WORD_FACTORY_DEFAULT. > > So the proposals are: > > 1. NVM_CHECKSUM_WORD_FACTORY_DEFAULT > 2. NVM_CHECKSUM_FACTORY_DEFAULT > 3. NVM_CHECKSUM_INVALID > 4. NVM_CHECKSUM_MISSING > 5. NVM_CHECKSUM_EMPTY > 6. NVM_NO_CHECKSUM > > Any other contenders? > 0x With a comment saying some manufacturers don't calculate the checksum. Then you don't needs to search the definition to find out what is going on. David
Re: [Intel-wired-lan] [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16
On Tue, 8 Jul 2025 10:16:52 +0200
Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
>
> Signed-off-by: Jacek Kowalski
> Suggested-by: Simon Horman
Reviewed-by: David Laight
For all the patches, perhaps changing 'unnecessary' to 'pointless'.
All the cast values are immediately promoted to 'signed int' and
then possibly promoted to 'unsigned int' depending of the type of
the other arithmetic operands.
> ---
> drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
> drivers/net/ethernet/intel/e1000/e1000_hw.c | 4 ++--
> drivers/net/ethernet/intel/e1000/e1000_main.c| 2 +-
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> index d06d29c6c037..d152026a027b 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> @@ -806,7 +806,7 @@ static int e1000_eeprom_test(struct e1000_adapter
> *adapter, u64 *data)
> }
>
> /* If Checksum is not Correct return error else test passed */
> - if ((checksum != (u16)EEPROM_SUM) && !(*data))
> + if ((checksum != EEPROM_SUM) && !(*data))
> *data = 2;
>
> return *data;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> index f9328f2e669f..0e5de52b1067 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> @@ -3970,7 +3970,7 @@ s32 e1000_validate_eeprom_checksum(struct e1000_hw *hw)
> return E1000_SUCCESS;
>
> #endif
> - if (checksum == (u16)EEPROM_SUM)
> + if (checksum == EEPROM_SUM)
> return E1000_SUCCESS;
> else {
> e_dbg("EEPROM Checksum Invalid\n");
> @@ -3997,7 +3997,7 @@ s32 e1000_update_eeprom_checksum(struct e1000_hw *hw)
> }
> checksum += eeprom_data;
> }
> - checksum = (u16)EEPROM_SUM - checksum;
> + checksum = EEPROM_SUM - checksum;
> if (e1000_write_eeprom(hw, EEPROM_CHECKSUM_REG, 1, &checksum) < 0) {
> e_dbg("EEPROM Write Error\n");
> return -E1000_ERR_EEPROM;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
> b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d8595e84326d..09acba2ed483 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -313,7 +313,7 @@ static void e1000_update_mng_vlan(struct e1000_adapter
> *adapter)
> } else {
> adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
> }
> - if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> + if ((old_vid != E1000_MNG_VLAN_NONE) &&
> (vid != old_vid) &&
> !test_bit(old_vid, adapter->active_vlans))
> e1000_vlan_rx_kill_vid(netdev, htons(ETH_P_8021Q),
Re: [Intel-wired-lan] [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16
On Tue, 8 Jul 2025 21:40:12 +0200
Jacek Kowalski wrote:
> >> - if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> >> + if ((old_vid != E1000_MNG_VLAN_NONE) &&
> >
> > Ditto.
> >
> > But more importantly, both Clang 20.1.7 W=1 builds (or at any rate,
> > builds with -Wtautological-constant-out-of-range-compare), and Smatch
> > complain that the comparison above is now always true because
> > E1000_MNG_VLAN_NONE is -1, while old_vid is unsigned.
I'm guessing 'old_vid' is actually u16 (or the compiler knows the
value came from a u16).
In either case the compiler can 'know' that the condition is always
true - but a 'u16 old_vid' is promoted to 'signed int' prior to
the compare with -1, whereas if a 'u32 old_vid' is known to contain
a 16bit value it is the -1 that is converted to ~0u.
>
> You are right - I have missed that E1000_MNG_VLAN_NONE is negative.
> Therefore (u16)E1000_MNG_VLAN_NONE has a side effect of causing a
> wraparound.
>
> It's even more interesting that (inadvertently) I have not made a
> similar change in e1000e:
>
> ./drivers/net/ethernet/intel/e1000e/netdev.c:
> if (adapter->mng_vlan_id != (u16)E1000_MNG_VLAN_NONE) {
>
>
> > Perhaps E1000_MNG_VLAN_NONE should be updated to be UINT16_MAX?
>
> There's no UINT16_MAX in kernel as far as I know. I'd rather leave it as
> it was or, if you insist on further refactoring, use either one of:
>
> #define E1000_MNG_VLAN_NONE (u16)(~((u16) 0))
> mimick ACPI: #define ACPI_UINT16_MAX (u16)(~((u16) 0))
>
> #define E1000_MNG_VLAN_NONE ((u16)-1)
> move the cast into the constant
>
> #define E1000_MNG_VLAN_NONE 0x
> use ready-made value
Possibly better is 0xu.
Then 'u16 old_val' is first promoted to 'signed int' and then implicitly
cast to 'unsigned int' prior to the comparison.
Remember C does all maths as [un]signed int (except for types bigger than int).
Local variables, function parameters and function results should really
be [un]signed int provided the domain of value doesn't exceed that of int.
Otherwise the compile is forced to generate explicit instructions to mask
the result of arithmetic operations to the smaller type.
David
>
> (parentheses left only due to the constant being "(-1)" and not "-1").
>
Re: [Intel-wired-lan] [PATCH 4/4] igc: drop checksum constant cast to u16 in comparisons
On Wed, 25 Jun 2025 13:22:39 +0100
Simon Horman wrote:
> On Tue, Jun 24, 2025 at 09:31:08PM +0200, Jacek Kowalski wrote:
> > Signed-off-by: Jacek Kowalski
> > Suggested-by: Simon Horman
> > ---
> > drivers/net/ethernet/intel/igc/igc_nvm.c | 2 +-
>
> I think we should add this:
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_nvm.c
> b/drivers/net/ethernet/intel/igc/igc_nvm.c
> index c4fb35071636..a47b8d39238c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_nvm.c
> +++ b/drivers/net/ethernet/intel/igc/igc_nvm.c
> @@ -155,7 +155,7 @@ s32 igc_update_nvm_checksum(struct igc_hw *hw)
> }
> checksum += nvm_data;
> }
> - checksum = (u16)NVM_SUM - checksum;
> + checksum = NVM_SUM - checksum;
Indeed - especially as the '-' code is really:
checksum = (int)(u16)NVM_SUM - checksum;
David
> ret_val = hw->nvm.ops.write(hw, NVM_CHECKSUM_REG, 1, &checksum);
> if (ret_val)
> hw_dbg("NVM Write Error while updating checksum.\n");
>
