* [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue
@ 2023-08-02 6:57 Aleksandr Loktionov
2023-08-04 21:27 ` Tony Nguyen
0 siblings, 1 reply; 3+ messages in thread
From: Aleksandr Loktionov @ 2023-08-02 6:57 UTC (permalink / raw)
To: intel-wired-lan, anthony.l.nguyen, aleksandr.loktionov
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
of two 32 bit vaues.
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
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;
- while ((gtime < timeout) && time_left) {
+ while ((s32)(gtime - timeout) < 0 && time_left) {
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) */
u32 timeout; /* [ms] */
u16 sr_size; /* Shadow RAM size in words */
bool blank_nvm_mode; /* is NVM empty (no FW present)*/
--
2.25.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue
2023-08-02 6:57 [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue Aleksandr Loktionov
@ 2023-08-04 21:27 ` Tony Nguyen
2023-08-17 19:26 ` Loktionov, Aleksandr
0 siblings, 1 reply; 3+ messages in thread
From: Tony Nguyen @ 2023-08-04 21:27 UTC (permalink / raw)
To: Aleksandr Loktionov, intel-wired-lan
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue
2023-08-04 21:27 ` Tony Nguyen
@ 2023-08-17 19:26 ` Loktionov, Aleksandr
0 siblings, 0 replies; 3+ messages in thread
From: Loktionov, Aleksandr @ 2023-08-17 19:26 UTC (permalink / raw)
To: Nguyen, Anthony L, intel-wired-lan@lists.osuosl.org
> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Friday, August 4, 2023 11:28 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue
>
> 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.
>
fixed in v3
> >
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>
> net fixes need Fixes: tag
>
Fixed in v2
> > ---
> > 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?
Yes of course, it was tested.
>
> > - 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.
Please see the commit message v3, also see comments for #define time_after(a,b)
https://elixir.bootlin.com/linux/latest/source/include/linux/jiffies.h#L104
>
> > 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?
>
Yes of course, it was tested.
> > 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-17 19:27 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-02 6:57 [Intel-wired-lan] [PATCH iwl-net v1] i40e: fix 32bit FW gtime wrapping issue Aleksandr Loktionov
2023-08-04 21:27 ` Tony Nguyen
2023-08-17 19:26 ` Loktionov, Aleksandr
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox