From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Neftin, Sasha" Subject: Re: [net-next 6/9] e1000e: fix buffer overrun while the I219 is processing DMA transactions Date: Mon, 16 Oct 2017 13:39:55 +0300 Message-ID: <2c60b0c2-b257-fa86-c1c8-c8aa1fd40908@intel.com> References: <20171010172139.77914-1-jeffrey.t.kirsher@intel.com> <20171010172139.77914-7-jeffrey.t.kirsher@intel.com> <063D6719AE5E284EB5DD2968C1650D6DD0090E17@AcuExch.aculab.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" , "nhorman@redhat.com" , "sassmann@redhat.com" , "jogreene@redhat.com" To: David Laight , 'Jeff Kirsher' , "davem@davemloft.net" Return-path: Received: from mga06.intel.com ([134.134.136.31]:61085 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750914AbdJPKkA (ORCPT ); Mon, 16 Oct 2017 06:40:00 -0400 In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD0090E17@AcuExch.aculab.com> Sender: netdev-owner@vger.kernel.org List-ID: On 10/11/2017 12:07, David Laight wrote: > From: Jeff Kirsher >> Sent: 10 October 2017 18:22 >> Intel 100/200 Series Chipset platforms reduced the round-trip >> latency for the LAN Controller DMA accesses, causing in some high >> performance cases a buffer overrun while the I219 LAN Connected >> Device is processing the DMA transactions. I219LM and I219V devices >> can fall into unrecovered Tx hang under very stressfully UDP traffic >> and multiple reconnection of Ethernet cable. This Tx hang of the LAN >> Controller is only recovered if the system is rebooted. Slightly slow >> down DMA access by reducing the number of outstanding requests. >> This workaround could have an impact on TCP traffic performance >> on the platform. Disabling TSO eliminates performance loss for TCP >> traffic without a noticeable impact on CPU performance. >> >> Please, refer to I218/I219 specification update: >> https://www.intel.com/content/www/us/en/embedded/products/networking/ >> ethernet-connection-i218-family-documentation.html >> >> Signed-off-by: Sasha Neftin >> Reviewed-by: Dima Ruinskiy >> Reviewed-by: Raanan Avargil >> Tested-by: Aaron Brown >> Signed-off-by: Jeff Kirsher >> --- >> drivers/net/ethernet/intel/e1000e/netdev.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c >> index ee9de3500331..14b096f3d1da 100644 >> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >> @@ -3021,8 +3021,8 @@ static void e1000_configure_tx(struct e1000_adapter *adapter) >> >> hw->mac.ops.config_collision_dist(hw); >> >> - /* SPT and CNP Si errata workaround to avoid data corruption */ >> - if (hw->mac.type >= e1000_pch_spt) { >> + /* SPT and KBL Si errata workaround to avoid data corruption */ >> + if (hw->mac.type == e1000_pch_spt) { >> u32 reg_val; >> >> reg_val = er32(IOSFPC); >> @@ -3030,7 +3030,9 @@ static void e1000_configure_tx(struct e1000_adapter *adapter) >> ew32(IOSFPC, reg_val); >> >> reg_val = er32(TARC(0)); >> - reg_val |= E1000_TARC0_CB_MULTIQ_3_REQ; >> + /* SPT and KBL Si errata workaround to avoid Tx hang */ >> + reg_val &= ~BIT(28); >> + reg_val |= BIT(29); > Shouldn't some more of the commit message about what this is doing > be in the comment? There is provided link on specification update: https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/i218-i219-ethernet-connection-spec-update.pdf?asset=9561. This is Intel's public release. > And shouldn't the 28 and 28 be named constants? (28 and 29) - you can easy understand from code that same value has been changed from 3 to 2. There is no point add flag here I thought. > >> ew32(TARC(0), reg_val); > David > Thanks, Sasha