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:34:10 +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 mga04.intel.com ([192.55.52.120]:9456 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797AbdH0IeN (ORCPT ); Sun, 27 Aug 2017 04:34:13 -0400 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On 8/27/2017 11:32, Neftin, Sasha wrote: > 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 > > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan