Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 8/1/2023 11:57 PM, Aleksandr Loktionov wrote:
> Decrease hw_semaphore_timeout size down to 32bits, because FW
> I40E_GLVFGEN_TIMER register is 32bits only anyway, but having
> both variables same u32 size simplifies code.
> Fix FW write semaphore expiration condition, taking into account
> that I40E_GLVFGEN_TIMER wraps, by checking the sign of substraction

s/substraction/subtraction

> of two 32 bit vaues.

s/vaues/values.

I'd suggest making this paragraph first since this is the issue you are 
fixing.

> 
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

net fixes need Fixes: tag

> ---
>   drivers/net/ethernet/intel/i40e/i40e_nvm.c  | 8 ++++----
>   drivers/net/ethernet/intel/i40e/i40e_type.h | 2 +-
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
> index 9da0c87..0fe8fc3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
> @@ -55,7 +55,7 @@ int i40e_init_nvm(struct i40e_hw *hw)
>   int i40e_acquire_nvm(struct i40e_hw *hw,
>   		     enum i40e_aq_resource_access_type access)
>   {
> -	u64 gtime, timeout;
> +	u32 gtime, timeout;
>   	u64 time_left = 0;
>   	int ret_code = 0;
>   
> @@ -78,7 +78,7 @@ int i40e_acquire_nvm(struct i40e_hw *hw,
>   	if (ret_code && time_left) {
>   		/* Poll until the current NVM owner timeouts */
>   		timeout = I40E_MS_TO_GTIME(I40E_MAX_NVM_TIMEOUT) + gtime;

Since timeout is gtime(u32) + I40E_MAX_NVM_TIMEOUT, does timeout need to 
stay u64 to hold the result properly?

> -		while ((gtime < timeout) && time_left) {
> +		while ((s32)(gtime - timeout) < 0 && time_left) {

How is this different than the original check? It seems like a different 
variation to check the same thing.

>   			usleep_range(10000, 20000);
>   			gtime = rd32(hw, I40E_GLVFGEN_TIMER);
>   			ret_code = i40e_aq_request_resource(hw,
> @@ -1192,9 +1192,9 @@ static int i40e_nvmupd_state_writing(struct i40e_hw *hw,
>   		u32 gtime;
>   
>   		gtime = rd32(hw, I40E_GLVFGEN_TIMER);
> -		if (gtime >= hw->nvm.hw_semaphore_timeout) {
> +		if ((s32)(gtime - hw->nvm.hw_semaphore_timeout) >= 0) {
>   			i40e_debug(hw, I40E_DEBUG_ALL,
> -				   "NVMUPD: write semaphore expired (%d >= %lld), retrying\n",
> +				   "NVMUPD: write semaphore expired (%d >= %d), retrying\n",
>   				   gtime, hw->nvm.hw_semaphore_timeout);
>   			i40e_release_nvm(hw);
>   			status = i40e_acquire_nvm(hw, I40E_RESOURCE_WRITE);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_type.h b/drivers/net/ethernet/intel/i40e/i40e_type.h
> index 388c3d3..efffe27 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_type.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_type.h
> @@ -328,7 +328,7 @@ enum i40e_aq_resource_access_type {
>   };
>   
>   struct i40e_nvm_info {
> -	u64 hw_semaphore_timeout; /* usec global time (GTIME resolution) */
> +	u32 hw_semaphore_timeout; /* usec global time (GTIME resolution) */

Can this hold a u32 + I40E_MAX_NVM_TIMEOUT?

>   	u32 timeout;              /* [ms] */
>   	u16 sr_size;              /* Shadow RAM size in words */
>   	bool blank_nvm_mode;      /* is NVM empty (no FW present)*/
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan