diff mbox series

[v1] igc: Increase timeout value for Speed 100/1000/2500

Message ID 20210709234516.24753-1-muhammad.husaini.zulkifli@intel.com
State Superseded
Delegated to: Anthony Nguyen
Headers show
Series [v1] igc: Increase timeout value for Speed 100/1000/2500 | expand

Commit Message

Zulkifli, Muhammad Husaini July 9, 2021, 11:45 p.m. UTC
As the cycle time is set to maximum of 1s, the TX Hang timeout need to
be increase to avoid possible TX Hangs caused by using long Qbv cycles.

Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Sasha Neftin July 13, 2021, 4:44 p.m. UTC | #1
On 7/10/2021 02:45, Muhammad Husaini Zulkifli wrote:
> As the cycle time is set to maximum of 1s, the TX Hang timeout need to
> be increase to avoid possible TX Hangs caused by using long Qbv cycles.
>
> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index c8abd7fb70e5..99fb5641297d 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -5322,7 +5322,9 @@ static void igc_watchdog_task(struct work_struct *work)
>   				adapter->tx_timeout_factor = 14;
>   				break;
>   			case SPEED_100:
> -				/* maybe add some timeout factor ? */
> +			case SPEED_1000:
> +			case SPEED_2500:
> +				adapter->tx_timeout_factor = 7;
>   				break;
>   			}
>   

Acked-by: Sasha Neftin <sasha.neftin@intel.com>
Zulkifli, Muhammad Husaini July 14, 2021, 1:41 a.m. UTC | #2
Thanks sasha and Vinicius!!

From: Neftin, Sasha <sasha.neftin@intel.com>
Sent: Wednesday, July 14, 2021 12:44 AM
To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>; intel-wired-lan@osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Gomes, Vinicius <vinicius.gomes@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH v1] igc: Increase timeout value for Speed 100/1000/2500

On 7/10/2021 02:45, Muhammad Husaini Zulkifli wrote:

As the cycle time is set to maximum of 1s, the TX Hang timeout need to

be increase to avoid possible TX Hangs caused by using long Qbv cycles.



Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com><mailto:muhammad.husaini.zulkifli@intel.com>

---

 drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-

 1 file changed, 3 insertions(+), 1 deletion(-)



diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c

index c8abd7fb70e5..99fb5641297d 100644

--- a/drivers/net/ethernet/intel/igc/igc_main.c

+++ b/drivers/net/ethernet/intel/igc/igc_main.c

@@ -5322,7 +5322,9 @@ static void igc_watchdog_task(struct work_struct *work)

                               adapter->tx_timeout_factor = 14;

                               break;

                        case SPEED_100:

-                              /* maybe add some timeout factor ? */

+                       case SPEED_1000:

+                       case SPEED_2500:

+                              adapter->tx_timeout_factor = 7;

                               break;

                        }



Acked-by: Sasha Neftin <sasha.neftin@intel.com><mailto:sasha.neftin@intel.com>
Paul Menzel July 14, 2021, 6:23 a.m. UTC | #3
Dear Muhammad,


Am 10.07.21 um 01:45 schrieb Muhammad Husaini Zulkifli:
> As the cycle time is set to maximum of 1s, the TX Hang timeout need to
> be increase to avoid possible TX Hangs caused by using long Qbv cycles.

s/increase/increased/

Do you have a way how to reproduce the issue?

> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index c8abd7fb70e5..99fb5641297d 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -5322,7 +5322,9 @@ static void igc_watchdog_task(struct work_struct *work)
>   				adapter->tx_timeout_factor = 14;
>   				break;
>   			case SPEED_100:
> -				/* maybe add some timeout factor ? */
> +			case SPEED_1000:
> +			case SPEED_2500:
> +				adapter->tx_timeout_factor = 7;

Please mention in the commit message, how the timeout factor of 7 was 
determined. Why not any other number.

>   				break;
>   			}


Kind regards,

Paul
Zulkifli, Muhammad Husaini July 17, 2021, 2:16 p.m. UTC | #4
Hi Paul,

Thanks for the comment. 
Replied inline

>-----Original Message-----
>From: Paul Menzel <pmenzel@molgen.mpg.de>
>Sent: Wednesday, July 14, 2021 2:24 PM
>To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>Cc: intel-wired-lan@osuosl.org
>Subject: Re: [Intel-wired-lan] [PATCH v1] igc: Increase timeout value for
>Speed 100/1000/2500
>
>Dear Muhammad,
>
>
>Am 10.07.21 um 01:45 schrieb Muhammad Husaini Zulkifli:
>> As the cycle time is set to maximum of 1s, the TX Hang timeout need to
>> be increase to avoid possible TX Hangs caused by using long Qbv cycles.
>
>s/increase/increased/
>
>Do you have a way how to reproduce the issue?
>
>> Signed-off-by: Muhammad Husaini Zulkifli
>> <muhammad.husaini.zulkifli@intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>> b/drivers/net/ethernet/intel/igc/igc_main.c
>> index c8abd7fb70e5..99fb5641297d 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -5322,7 +5322,9 @@ static void igc_watchdog_task(struct work_struct
>*work)
>>   				adapter->tx_timeout_factor = 14;
>>   				break;
>>   			case SPEED_100:
>> -				/* maybe add some timeout factor ? */
>> +			case SPEED_1000:
>> +			case SPEED_2500:
>> +				adapter->tx_timeout_factor = 7;
>
>Please mention in the commit message, how the timeout factor of 7 was
>determined. Why not any other number.

There is no dedicated number specific in data sheet for this timeout factor.
This time out factor was determined during the debugging  to solve the "tx hang" issues that
we were seeing in some cases especially during Scheduled packet Transmission(etf).

>
>>   				break;
>>   			}
>
>
>Kind regards,
>
>Paul
Paul Menzel July 17, 2021, 2:18 p.m. UTC | #5
Dear Muhammad,


Thank you for your response.

Am 17.07.21 um 16:16 schrieb Zulkifli, Muhammad Husaini:

>> -----Original Message-----
>> From: Paul Menzel <pmenzel@molgen.mpg.de>
>> Sent: Wednesday, July 14, 2021 2:24 PM
>> To: Zulkifli, Muhammad Husaini <muhammad.husaini.zulkifli@intel.com>
>> Cc: intel-wired-lan@osuosl.org
>> Subject: Re: [Intel-wired-lan] [PATCH v1] igc: Increase timeout value for Speed 100/1000/2500

>> Am 10.07.21 um 01:45 schrieb Muhammad Husaini Zulkifli:
>>> As the cycle time is set to maximum of 1s, the TX Hang timeout need to
>>> be increase to avoid possible TX Hangs caused by using long Qbv cycles.
>>
>> s/increase/increased/
>>
>> Do you have a way how to reproduce the issue?
>>
>>> Signed-off-by: Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/igc/igc_main.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
>>> b/drivers/net/ethernet/intel/igc/igc_main.c
>>> index c8abd7fb70e5..99fb5641297d 100644
>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>>> @@ -5322,7 +5322,9 @@ static void igc_watchdog_task(struct work_struct
>> *work)
>>>    				adapter->tx_timeout_factor = 14;
>>>    				break;
>>>    			case SPEED_100:
>>> -				/* maybe add some timeout factor ? */
>>> +			case SPEED_1000:
>>> +			case SPEED_2500:
>>> +				adapter->tx_timeout_factor = 7;
>>
>> Please mention in the commit message, how the timeout factor of 7 was
>> determined. Why not any other number.
> 
> There is no dedicated number specific in data sheet for this timeout factor.
> This time out factor was determined during the debugging  to solve the "tx hang" issues that
> we were seeing in some cases especially during Scheduled packet Transmission(etf).

Thank you for the clarification. Please mention that in the commit 
message of v2.

>>>    				break;
>>>    			}


Kind regards,

Paul
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index c8abd7fb70e5..99fb5641297d 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5322,7 +5322,9 @@  static void igc_watchdog_task(struct work_struct *work)
 				adapter->tx_timeout_factor = 14;
 				break;
 			case SPEED_100:
-				/* maybe add some timeout factor ? */
+			case SPEED_1000:
+			case SPEED_2500:
+				adapter->tx_timeout_factor = 7;
 				break;
 			}