| Message ID | 20170825150626.2843-1-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
On 8/25/2017 18:06, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@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@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.
On 8/27/2017 11:30, Neftin, Sasha wrote: > On 8/25/2017 18:06, Willem de Bruijn wrote: >> From: Willem de Bruijn <willemb@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@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@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
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 <willemb@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@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@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
On Sun, Aug 27, 2017 at 1:30 AM, Neftin, Sasha <sasha.neftin@intel.com> wrote: > On 8/25/2017 18:06, Willem de Bruijn wrote: >> >> From: Willem de Bruijn <willemb@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@google.com> > > This patch looks good for me, but I would like hear second opinion. This code is an improvement over what was already there. I am good with this. Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
> From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of Willem de Bruijn > Sent: Friday, August 25, 2017 8:06 AM > To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Brandeburg, > Jesse <jesse.brandeburg@intel.com>; Willem de Bruijn > <willemb@google.com> > Subject: [PATCH] e1000e: apply burst mode settings only on default > > From: Willem de Bruijn <willemb@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@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(-) Tested-by: Aaron Brown <aaron.f.brown@intel.com>
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,