[Intel-wired-lan] [PATCH] e1000e: apply burst mode settings only on default

[Neftin, Sasha]

On 8/27/2017 11:30, Neftin, Sasha wrote:
> On 8/25/2017 18:06, Willem de Bruijn wrote:
>> From: Willem de Bruijn <willemb at google.com>
>>
>> Devices that support FLAG2_DMA_BURST have different default values
>> for RDTR and RADV. Apply burst mode default settings only when no
>> explicit value was passed at module load.
>>
>> The RDTR default is zero. If the module is loaded for low latency
>> operation with RxIntDelay=0, do not override this value with a burst
>> default of 32.
>>
>> Move the decision to apply burst values earlier, where explicitly
>> initialized module variables can be distinguished from defaults.
>>
>> Signed-off-by: Willem de Bruijn <willemb at google.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/e1000.h  |  4 ----
>>   drivers/net/ethernet/intel/e1000e/netdev.c |  8 --------
>>   drivers/net/ethernet/intel/e1000e/param.c  | 16 +++++++++++++++-
>>   3 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h 
>> b/drivers/net/ethernet/intel/e1000e/e1000.h
>> index 98e68888abb1..2311b31bdcac 100644
>> --- a/drivers/net/ethernet/intel/e1000e/e1000.h
>> +++ b/drivers/net/ethernet/intel/e1000e/e1000.h
>> @@ -94,10 +94,6 @@ struct e1000_info;
>>    */
>>   #define E1000_CHECK_RESET_COUNT        25
>>   -#define DEFAULT_RDTR            0
>> -#define DEFAULT_RADV            8
>> -#define BURST_RDTR            0x20
>> -#define BURST_RADV            0x20
>>   #define PCICFG_DESC_RING_STATUS        0xe4
>>   #define FLUSH_DESC_REQUIRED        0x100
>>   diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
>> b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 327dfe5bedc0..47b89aac7969 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -3223,14 +3223,6 @@ static void e1000_configure_rx(struct 
>> e1000_adapter *adapter)
>>            */
>>           ew32(RXDCTL(0), E1000_RXDCTL_DMA_BURST_ENABLE);
>>           ew32(RXDCTL(1), E1000_RXDCTL_DMA_BURST_ENABLE);
>> -
>> -        /* override the delay timers for enabling bursting, only if
>> -         * the value was not set by the user via module options
>> -         */
>> -        if (adapter->rx_int_delay == DEFAULT_RDTR)
>> -            adapter->rx_int_delay = BURST_RDTR;
>> -        if (adapter->rx_abs_int_delay == DEFAULT_RADV)
>> -            adapter->rx_abs_int_delay = BURST_RADV;
>>       }
>>         /* set the Receive Delay Timer Register */
>> diff --git a/drivers/net/ethernet/intel/e1000e/param.c 
>> b/drivers/net/ethernet/intel/e1000e/param.c
>> index 6d8c39abee16..bb696c98f9b0 100644
>> --- a/drivers/net/ethernet/intel/e1000e/param.c
>> +++ b/drivers/net/ethernet/intel/e1000e/param.c
>> @@ -73,17 +73,25 @@ E1000_PARAM(TxAbsIntDelay, "Transmit Absolute 
>> Interrupt Delay");
>>   /* Receive Interrupt Delay in units of 1.024 microseconds
>>    * hardware will likely hang if you set this to anything but zero.
>>    *
>> + * Burst variant is used as default if device has FLAG2_DMA_BURST.
>> + *
>>    * Valid Range: 0-65535
>>    */
>>   E1000_PARAM(RxIntDelay, "Receive Interrupt Delay");
>> +#define DEFAULT_RDTR            0
>> +#define BURST_RDTR            0x20
>>   #define MAX_RXDELAY 0xFFFF
>>   #define MIN_RXDELAY 0
>>     /* Receive Absolute Interrupt Delay in units of 1.024 microseconds
>> + *
>> + * Burst variant is used as default if device has FLAG2_DMA_BURST.
>>    *
>>    * Valid Range: 0-65535
>>    */
>>   E1000_PARAM(RxAbsIntDelay, "Receive Absolute Interrupt Delay");
>> +#define DEFAULT_RADV            8
>> +#define BURST_RADV            0x20
>>   #define MAX_RXABSDELAY 0xFFFF
>>   #define MIN_RXABSDELAY 0
>>   @@ -297,6 +305,9 @@ void e1000e_check_options(struct e1000_adapter 
>> *adapter)
>>                        .max = MAX_RXDELAY } }
>>           };
>>   +        if (adapter->flags2 & FLAG2_DMA_BURST)
>> +            opt.def = BURST_RDTR;
>> +
>>           if (num_RxIntDelay > bd) {
>>               adapter->rx_int_delay = RxIntDelay[bd];
>> e1000_validate_option(&adapter->rx_int_delay, &opt,
>> @@ -307,7 +318,7 @@ void e1000e_check_options(struct e1000_adapter 
>> *adapter)
>>       }
>>       /* Receive Absolute Interrupt Delay */
>>       {
>> -        static const struct e1000_option opt = {
>> +        static struct e1000_option opt = {
>>               .type = range_option,
>>               .name = "Receive Absolute Interrupt Delay",
>>               .err  = "using default of "
>> @@ -317,6 +328,9 @@ void e1000e_check_options(struct e1000_adapter 
>> *adapter)
>>                        .max = MAX_RXABSDELAY } }
>>           };
>>   +        if (adapter->flags2 & FLAG2_DMA_BURST)
>> +            opt.def = BURST_RADV;
>> +
>>           if (num_RxAbsIntDelay > bd) {
>>               adapter->rx_abs_int_delay = RxAbsIntDelay[bd];
>> e1000_validate_option(&adapter->rx_abs_int_delay, &opt,
>
> This patch looks good for me, but I would like hear second opinion.
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan