From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Neftin, Sasha" Subject: Re: [Intel-wired-lan] [PATCH] e1000e: apply burst mode settings only on default Date: Sun, 27 Aug 2017 11:32:15 +0300 Message-ID: References: <20170825150626.2843-1-willemdebruijn.kernel@gmail.com> <291b863f-552e-2f3b-f658-e812d0848949@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Willem de Bruijn , intel-wired-lan@lists.osuosl.org To: Willem de Bruijn , "jeffrey.t.kirsher@intel.com; alexander.h.duyck@intel.com; raanan.avargil@intel.com; dima.ruinskiy"@intel.com Return-path: Received: from mga01.intel.com ([192.55.52.88]:63872 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750830AbdH0IcS (ORCPT ); Sun, 27 Aug 2017 04:32:18 -0400 In-Reply-To: <291b863f-552e-2f3b-f658-e812d0848949@intel.com> Sender: netdev-owner@vger.kernel.org List-ID: On 8/27/2017 11:30, Neftin, Sasha wrote: > On 8/25/2017 18:06, Willem de Bruijn wrote: >> From: Willem de Bruijn >> >> 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 >> --- >> 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@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan