[Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Kalyan Kodamagula]

From: Marcin Szycik <marcin.szycik@intel.com>

Fix implicit cast by changing argument types of two functions to correct
types.

Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
index d71ed210f9c4..830fa53b5e0a 100644
--- a/drivers/net/ethernet/intel/ice/ice_ddp.c
+++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
@@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct ice_hw *hw)
 	return bld;
 }
 
-static bool ice_is_gtp_u_profile(u16 prof_idx)
+static bool ice_is_gtp_u_profile(u32 prof_idx)
 {
 	return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
 		prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
 	       prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
 }
 
-static bool ice_is_gtp_c_profile(u16 prof_idx)
+static bool ice_is_gtp_c_profile(u32 prof_idx)
 {
 	switch (prof_idx) {
 	case ICE_PROFID_IPV4_GTPC_TEID:
-- 
2.39.2

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Paul Menzel]

Dear Kalyan, dear Marcin,


Am 28.02.23 um 09:49 schrieb Kalyan Kodamagula:
> From: Marcin Szycik <marcin.szycik@intel.com>
> 
> Fix implicit cast by changing argument types of two functions to correct
> types.
> 
> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index d71ed210f9c4..830fa53b5e0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct ice_hw *hw)
>   	return bld;
>   }
>   
> -static bool ice_is_gtp_u_profile(u16 prof_idx)
> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>   {
>   	return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>   		prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>   	       prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>   }
>   
> -static bool ice_is_gtp_c_profile(u16 prof_idx)
> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>   {
>   	switch (prof_idx) {
>   	case ICE_PROFID_IPV4_GTPC_TEID:

Is there a reason to limit the length or could `unsigned int` be used?


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Alexander Lobakin]

From: Paul Menzel <pmenzel@molgen.mpg.de>
Date: Tue, 28 Feb 2023 11:01:00 +0100

> Dear Kalyan, dear Marcin,
> 
> 
> Am 28.02.23 um 09:49 schrieb Kalyan Kodamagula:
>> From: Marcin Szycik <marcin.szycik@intel.com>
>>
>> Fix implicit cast by changing argument types of two functions to correct
>> types.
>>
>> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
>> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> index d71ed210f9c4..830fa53b5e0a 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct
>> ice_hw *hw)
>>       return bld;
>>   }
>>   -static bool ice_is_gtp_u_profile(u16 prof_idx)
>> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>>   {
>>       return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>>           prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>>              prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>>   }
>>   -static bool ice_is_gtp_c_profile(u16 prof_idx)
>> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>>   {
>>       switch (prof_idx) {
>>       case ICE_PROFID_IPV4_GTPC_TEID:
> 
> Is there a reason to limit the length or could `unsigned int` be used?

You mean the string length? But what's the point of using `unsigned int`
if we have shorter and more elegant `u32`, which at the same time
explicitly states its width? :)
I've been encouraging lots o' folks to prefer the "shorties" where
possible (I basically only use {,unsigned} long from the "basic" types)
and now this :p I'm not saying any opinion is correct or incorrect here,
since it's a matter of taste mostly I believe, just curious.

> 
> 
> Kind regards,
> 
> Paul
Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Alexander Lobakin]

From: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
Date: Tue, 28 Feb 2023 09:49:15 +0100

> From: Marcin Szycik <marcin.szycik@intel.com>
> 
> Fix implicit cast by changing argument types of two functions to correct
> types.
> 
> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>

Regarding the actual patch (below),

> ---
>  drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
> index d71ed210f9c4..830fa53b5e0a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct ice_hw *hw)
>  	return bld;
>  }
>  
> -static bool ice_is_gtp_u_profile(u16 prof_idx)
> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>  {
>  	return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>  		prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>  	       prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>  }
>  
> -static bool ice_is_gtp_c_profile(u16 prof_idx)
> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>  {
>  	switch (prof_idx) {
>  	case ICE_PROFID_IPV4_GTPC_TEID:

What is this change really about? It might've been a part of some bigger
series, isn't it? Does it fix any truncating-related bugs or improve
codegen, which could be observed by objdiff or bloat-o-meter? It feels
completely out of context.

Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Paul Menzel]

Dear Alexander,


Am 28.02.23 um 16:29 schrieb Alexander Lobakin:
> From: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
> Date: Tue, 28 Feb 2023 09:49:15 +0100
> 
>> From: Marcin Szycik <marcin.szycik@intel.com>
>>
>> Fix implicit cast by changing argument types of two functions to correct
>> types.
>>
>> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
>> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
> 
> Regarding the actual patch (below),
> 
>> ---
>>   drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> index d71ed210f9c4..830fa53b5e0a 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct ice_hw *hw)
>>   	return bld;
>>   }
>>   
>> -static bool ice_is_gtp_u_profile(u16 prof_idx)
>> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>>   {
>>   	return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>>   		prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>>   	       prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>>   }
>>   
>> -static bool ice_is_gtp_c_profile(u16 prof_idx)
>> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>>   {
>>   	switch (prof_idx) {
>>   	case ICE_PROFID_IPV4_GTPC_TEID:
> 
> What is this change really about? It might've been a part of some bigger
> series, isn't it? Does it fix any truncating-related bugs or improve
> codegen, which could be observed by objdiff or bloat-o-meter? It feels
> completely out of context.

I asked myself the same thing right now reading your other reply:

```
/**
  * ice_get_sw_prof_type - determine switch profile type
  * @hw: pointer to the HW structure
  * @fv: pointer to the switch field vector
  * @prof_idx: profile index to check
  */
static enum ice_prof_type ice_get_sw_prof_type(struct ice_hw *hw,
                                                struct ice_fv *fv, u32 
prof_idx)
{
         u16 i;

         if (ice_is_gtp_c_profile(prof_idx))
                 return ICE_PROF_TUN_GTPC;

         if (ice_is_gtp_u_profile(prof_idx))
                 return ICE_PROF_TUN_GTPU;

[…]
```

I think they mean that the signature of ice_prof_type 
ice_get_sw_prof_type() takes prof_idx as u32.


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Paul Menzel]

Dear Alexander,


Am 28.02.23 um 16:21 schrieb Alexander Lobakin:
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Date: Tue, 28 Feb 2023 11:01:00 +0100

>> Am 28.02.23 um 09:49 schrieb Kalyan Kodamagula:
>>> From: Marcin Szycik <marcin.szycik@intel.com>
>>>
>>> Fix implicit cast by changing argument types of two functions to correct
>>> types.
>>>
>>> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
>>> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> index d71ed210f9c4..830fa53b5e0a 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct
>>> ice_hw *hw)
>>>        return bld;
>>>    }
>>>    -static bool ice_is_gtp_u_profile(u16 prof_idx)
>>> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>>>    {
>>>        return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>>>            prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>>>               prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>>>    }
>>>    -static bool ice_is_gtp_c_profile(u16 prof_idx)
>>> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>>>    {
>>>        switch (prof_idx) {
>>>        case ICE_PROFID_IPV4_GTPC_TEID:
>>
>> Is there a reason to limit the length or could `unsigned int` be used?
> 
> You mean the string length? But what's the point of using `unsigned int`
> if we have shorter and more elegant `u32`, which at the same time
> explicitly states its width? :)
> I've been encouraging lots o' folks to prefer the "shorties" where
> possible (I basically only use {,unsigned} long from the "basic" types)
> and now this :p I'm not saying any opinion is correct or incorrect here,
> since it's a matter of taste mostly I believe, just curious.

If in future architectures, the smallest native size is 64 bit, than 
unnecessarily truncating the length, would create not optimal code. 
Judging from the defined macros, `prof_idx` also does not need to be 32 
bit wide. (But it’s all microoptimization.)


Kind regards,

Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Marcin Szycik]

Hi Paul,

On 28.02.2023 16:40, Paul Menzel wrote:
> Dear Alexander,
> 
> 
> Am 28.02.23 um 16:29 schrieb Alexander Lobakin:
>> From: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
>> Date: Tue, 28 Feb 2023 09:49:15 +0100
>>
>>> From: Marcin Szycik <marcin.szycik@intel.com>
>>>
>>> Fix implicit cast by changing argument types of two functions to correct
>>> types.
>>>
>>> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
>>> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
>>
>> Regarding the actual patch (below),
>>
>>> ---
>>>   drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> index d71ed210f9c4..830fa53b5e0a 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct ice_hw *hw)
>>>       return bld;
>>>   }
>>>   -static bool ice_is_gtp_u_profile(u16 prof_idx)
>>> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>>>   {
>>>       return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>>>           prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>>>              prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>>>   }
>>>   -static bool ice_is_gtp_c_profile(u16 prof_idx)
>>> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>>>   {
>>>       switch (prof_idx) {
>>>       case ICE_PROFID_IPV4_GTPC_TEID:
>>
>> What is this change really about? It might've been a part of some bigger
>> series, isn't it? Does it fix any truncating-related bugs or improve
>> codegen, which could be observed by objdiff or bloat-o-meter? It feels
>> completely out of context.
> 
> I asked myself the same thing right now reading your other reply:
> 
> ```
> /**
>  * ice_get_sw_prof_type - determine switch profile type
>  * @hw: pointer to the HW structure
>  * @fv: pointer to the switch field vector
>  * @prof_idx: profile index to check
>  */
> static enum ice_prof_type ice_get_sw_prof_type(struct ice_hw *hw,
>                                                struct ice_fv *fv, u32 prof_idx)
> {
>         u16 i;
> 
>         if (ice_is_gtp_c_profile(prof_idx))
>                 return ICE_PROF_TUN_GTPC;
> 
>         if (ice_is_gtp_u_profile(prof_idx))
>                 return ICE_PROF_TUN_GTPU;
> 
> […]
> ```
> 
> I think they mean that the signature of ice_prof_type ice_get_sw_prof_type() takes prof_idx as u32.

This patch was made to avoid type warnings in certain internal builds.
I agree that the change is pretty useless by itself, and I apologize for
not catching it in internal review, it shouldn't have passed to IWL.
I suggest to ignore this patch.

> 
> 
> Kind regards,
> 
> Paul
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Regards,
Marcin
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix implicit cast u32 to u16

[Alexander Lobakin]

From: Paul Menzel <pmenzel@molgen.mpg.de>
Date: Tue, 28 Feb 2023 16:44:17 +0100

> Dear Alexander,
> 
> 
> Am 28.02.23 um 16:21 schrieb Alexander Lobakin:
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> Date: Tue, 28 Feb 2023 11:01:00 +0100
> 
>>> Am 28.02.23 um 09:49 schrieb Kalyan Kodamagula:
>>>> From: Marcin Szycik <marcin.szycik@intel.com>
>>>>
>>>> Fix implicit cast by changing argument types of two functions to
>>>> correct
>>>> types.
>>>>
>>>> Signed-off-by: Marcin Szycik <marcin.szycik@intel.com>
>>>> Signed-off-by: Kalyan Kodamagula <kalyan.kodamagula@intel.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/ice/ice_ddp.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>>> b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>>> index d71ed210f9c4..830fa53b5e0a 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c
>>>> @@ -701,14 +701,14 @@ struct ice_buf_build *ice_pkg_buf_alloc(struct
>>>> ice_hw *hw)
>>>>        return bld;
>>>>    }
>>>>    -static bool ice_is_gtp_u_profile(u16 prof_idx)
>>>> +static bool ice_is_gtp_u_profile(u32 prof_idx)
>>>>    {
>>>>        return (prof_idx >= ICE_PROFID_IPV6_GTPU_TEID &&
>>>>            prof_idx <= ICE_PROFID_IPV6_GTPU_IPV6_TCP_INNER) ||
>>>>               prof_idx == ICE_PROFID_IPV4_GTPU_TEID;
>>>>    }
>>>>    -static bool ice_is_gtp_c_profile(u16 prof_idx)
>>>> +static bool ice_is_gtp_c_profile(u32 prof_idx)
>>>>    {
>>>>        switch (prof_idx) {
>>>>        case ICE_PROFID_IPV4_GTPC_TEID:
>>>
>>> Is there a reason to limit the length or could `unsigned int` be used?
>>
>> You mean the string length? But what's the point of using `unsigned int`
>> if we have shorter and more elegant `u32`, which at the same time
>> explicitly states its width? :)
>> I've been encouraging lots o' folks to prefer the "shorties" where
>> possible (I basically only use {,unsigned} long from the "basic" types)
>> and now this :p I'm not saying any opinion is correct or incorrect here,
>> since it's a matter of taste mostly I believe, just curious.
> 
> If in future architectures, the smallest native size is 64 bit, than
> unnecessarily truncating the length, would create not optimal code.

So one day u32 will start forcing CPU to mask the results of operations?
O_________O
I thought HW will always be like "32 bit is fine" :s
But wait, does it mean `unsigned int` will become 8 byte there? This
would break a lot of stuff in the kernel I believe...

> Judging from the defined macros, `prof_idx` also does not need to be 32
> bit wide. (But it’s all microoptimization.)
> 
> 
> Kind regards,
> 
> Paul
Thanks,
Olek
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan