| Message ID | 1425823498-30385-3-git-send-email-vladz@cloudius-systems.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id B7D639A91; Sun, 8 Mar 2015 15:05:07 +0100 (CET) Received: from mail-we0-f169.google.com (mail-we0-f169.google.com [74.125.82.169]) by dpdk.org (Postfix) with ESMTP id 94BF16A80 for <dev@dpdk.org>; Sun, 8 Mar 2015 15:05:05 +0100 (CET) Received: by wesu56 with SMTP id u56so1687045wes.12 for <dev@dpdk.org>; Sun, 08 Mar 2015 07:05:05 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=hFqb5I1GagjfmLPBkSqMNXRWzBgMI0Ry6YjPIBS22pE=; b=PPb+ugQKv1olqEGiJvPMuSDIwd+2DJL54wL4I1tMapWPXHH2S0Q1fu8zmoVonXnVPY GqsF/QRqTWdvNrmEEnKSgx4LDz+1p9OJZiwJwUECeJiT2n65G1HWD8awu4lsoa1HnkHr SoRbxIXBcRuoxon8UGd/g7c8arf9pY3WFBKFdyoW055iS/n3wIar7sh7k1P8U0Gl/Hov yFGZpqVVhb0OY0q9w11oQnHR5T+6v37R924R5OVT56Q0AdV/5kWfTry6O7gfUd0jK+9m M778S4BtKvJlz8Y7868OrJ+bMVA25cWn+JGNJL406O4KnxjrPwvKZbjpjjm0xsnXhVzV /pcg== X-Gm-Message-State: ALoCoQlB70M9zpIEsXDEkeHQ/qXLMzCoFw5PZHmQ0mLWeRfXwQbHOeFk9TXPP9GUFbseGWiO3n7P X-Received: by 10.180.212.70 with SMTP id ni6mr36786965wic.8.1425823505482; Sun, 08 Mar 2015 07:05:05 -0700 (PDT) Received: from vladz-laptop.cloudius-systems.com. ([212.143.139.214]) by mx.google.com with ESMTPSA id fo8sm11196154wib.14.2015.03.08.07.05.04 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 08 Mar 2015 07:05:04 -0700 (PDT) From: Vlad Zolotarov <vladz@cloudius-systems.com> To: dev@dpdk.org Date: Sun, 8 Mar 2015 16:04:55 +0200 Message-Id: <1425823498-30385-3-git-send-email-vladz@cloudius-systems.com> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1425823498-30385-1-git-send-email-vladz@cloudius-systems.com> References: <1425823498-30385-1-git-send-email-vladz@cloudius-systems.com> Subject: [dpdk-dev] [PATCH v4 2/5] ixgbe: Bug fix: Properly configure Rx CRC stripping for x540 devices X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Vladislav Zolotarov
March 8, 2015, 2:04 p.m. UTC
According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
be configured to the same value as HLREG0.RXCRCSTRP.
Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
but seems harmless.
Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Comments
Hi Vlad, 2015-03-08 16:04, Vlad Zolotarov: > According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should > be configured to the same value as HLREG0.RXCRCSTRP. > > Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec > but seems harmless. > > Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> You are mixing a fix (this patch) and enhancements (LRO) in the same series. Could you separate them please, as LRO is not going into 2.0 but this fix is a good candidate. Thanks
On 03/08/15 23:12, Thomas Monjalon wrote: > Hi Vlad, > > 2015-03-08 16:04, Vlad Zolotarov: >> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should >> be configured to the same value as HLREG0.RXCRCSTRP. >> >> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec >> but seems harmless. >> >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> > You are mixing a fix (this patch) and enhancements (LRO) in the same series. > Could you separate them please, as LRO is not going into 2.0 but this fix > is a good candidate. Pls, note that all patches in this series except for PATCH3 and PATCH5 are fixing real bugs. I can send them as a separate series if u'd like. Pls., confirm. > > Thanks
On 03/08/15 23:12, Thomas Monjalon wrote: > Hi Vlad, > > 2015-03-08 16:04, Vlad Zolotarov: >> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should >> be configured to the same value as HLREG0.RXCRCSTRP. >> >> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec >> but seems harmless. >> >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> > You are mixing a fix (this patch) and enhancements (LRO) in the same series. I'd also like to clarify that I've "mixed" them because the bugs were found in the area of the enhancement therefore I united them in the same series. Frankly, I wasn't thinking about the DPDK versions it's going into when I sent the series... ;) > Could you separate them please, as LRO is not going into 2.0 but this fix > is a good candidate. > > Thanks
2015-03-09 09:08, Vlad Zolotarov: > > On 03/08/15 23:12, Thomas Monjalon wrote: > > Hi Vlad, > > > > 2015-03-08 16:04, Vlad Zolotarov: > >> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should > >> be configured to the same value as HLREG0.RXCRCSTRP. > >> > >> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec > >> but seems harmless. > >> > >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> > > You are mixing a fix (this patch) and enhancements (LRO) in the same series. > > Could you separate them please, as LRO is not going into 2.0 but this fix > > is a good candidate. > > Pls, note that all patches in this series except for PATCH3 and PATCH5 > are fixing real bugs. I can send them as a separate series if u'd like. > Pls., confirm. Yes you're right, patch 1 is also a fix and patch 4 seems to solve other issues. However, patch 4 makes also some refactoring and seems a bit risky. We need an ixgbe maintainer to decide wether we can merge it before the release. Or is it possible to have fixes of the patch 4 without the refactoring? Thanks Vlad. Sorry to request such split but this PMD is sensible and I don't want to have a risk of making it worst in release 2.0.
On 03/09/15 09:58, Thomas Monjalon wrote: > 2015-03-09 09:08, Vlad Zolotarov: >> On 03/08/15 23:12, Thomas Monjalon wrote: >>> Hi Vlad, >>> >>> 2015-03-08 16:04, Vlad Zolotarov: >>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should >>>> be configured to the same value as HLREG0.RXCRCSTRP. >>>> >>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec >>>> but seems harmless. >>>> >>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >>> You are mixing a fix (this patch) and enhancements (LRO) in the same series. >>> Could you separate them please, as LRO is not going into 2.0 but this fix >>> is a good candidate. >> Pls, note that all patches in this series except for PATCH3 and PATCH5 >> are fixing real bugs. I can send them as a separate series if u'd like. >> Pls., confirm. > Yes you're right, patch 1 is also a fix and patch 4 seems to solve other > issues. However, patch 4 makes also some refactoring and seems a bit risky. > We need an ixgbe maintainer to decide wether we can merge it before the > release. Or is it possible to have fixes of the patch 4 without the > refactoring? > > Thanks Vlad. > Sorry to request such split but this PMD is sensible and I don't want to > have a risk of making it worst in release 2.0. Well, IMHO PATCH4 here has the most important and critical fix among other patches - the whole port is going to be configured according to the last queue configuration. Consider the case when queues are configured differently and the last one meets vector Rx requirements: bulk allocation requirements, descriptors number is a power of 2 (there are per-port requirements too). But the first queue may be configured to have a different number of descriptors, which is NOT a power of two. Currently the port's rx_pkt_burst callback will be configured to use a vercot Rx callback and we would hit the same issue reported by Stephen and "fixed" in 352078e8e.
On 03/09/15 09:58, Thomas Monjalon wrote: > 2015-03-09 09:08, Vlad Zolotarov: >> On 03/08/15 23:12, Thomas Monjalon wrote: >>> Hi Vlad, >>> >>> 2015-03-08 16:04, Vlad Zolotarov: >>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should >>>> be configured to the same value as HLREG0.RXCRCSTRP. >>>> >>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec >>>> but seems harmless. >>>> >>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >>> You are mixing a fix (this patch) and enhancements (LRO) in the same series. >>> Could you separate them please, as LRO is not going into 2.0 but this fix >>> is a good candidate. >> Pls, note that all patches in this series except for PATCH3 and PATCH5 >> are fixing real bugs. I can send them as a separate series if u'd like. >> Pls., confirm. > Yes you're right, patch 1 is also a fix and patch 4 seems to solve other > issues. However, patch 4 makes also some refactoring and seems a bit risky. > We need an ixgbe maintainer to decide wether we can merge it before the > release. Or is it possible to have fixes of the patch 4 without the > refactoring? PATCH4 doesn't have any refactoring - it fixes a design bug (actually two). The description of the patch is separated into two sections: one that describes how I did it and the second (starts with "Bugs fixed:") describes which bugs it fixes. When I re-read its description again now I see that I forgot to mention the second issue it fixes: there is the same problem for both Vector Rx callback initialization and for Bulk Rx callback initialization. In both cases in the current master tree the queue initialization code tries to initialize the port property (rx_pkt_bulk callback) which depends on all queues properties thus may be done only after all queues properties have been analyzed. More than that, some callbacks types may be used only when some combination of preconditions is met by all Rx queues: e.g. vector Rx may be used only when bulk (checks queue configuration) and vector (checks queue and port configuration) preconditions are met. Therefore I decided to introduce two port states that are a logical AND function of the appropriate queues' states. These states are used by a port configuration function that now can properly configure the callbacks. So, again - no refactoring here - a pure bug fix... ;) > > Thanks Vlad. > Sorry to request such split but this PMD is sensible and I don't want to > have a risk of making it worst in release 2.0.
On 03/09/15 09:58, Thomas Monjalon wrote: > 2015-03-09 09:08, Vlad Zolotarov: >> On 03/08/15 23:12, Thomas Monjalon wrote: >>> Hi Vlad, >>> >>> 2015-03-08 16:04, Vlad Zolotarov: >>>> According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should >>>> be configured to the same value as HLREG0.RXCRCSTRP. >>>> >>>> Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec >>>> but seems harmless. >>>> >>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com> >>> You are mixing a fix (this patch) and enhancements (LRO) in the same series. >>> Could you separate them please, as LRO is not going into 2.0 but this fix >>> is a good candidate. >> Pls, note that all patches in this series except for PATCH3 and PATCH5 >> are fixing real bugs. I can send them as a separate series if u'd like. >> Pls., confirm. > Yes you're right, patch 1 is also a fix and patch 4 seems to solve other > issues. However, patch 4 makes also some refactoring and seems a bit risky. > We need an ixgbe maintainer to decide wether we can merge it before the > release. Or is it possible to have fixes of the patch 4 without the > refactoring? > > Thanks Vlad. > Sorry to request such split but this PMD is sensible and I don't want to > have a risk of making it worst in release 2.0. I've separated the series into two: "bug fixes" and "cleanups + refactoring + LRO feature". Both series are already on the list. ;)
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c index fe362e4..e370e0a 100644 --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c @@ -3680,7 +3680,8 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev) IXGBE_WRITE_REG(hw, IXGBE_RXCSUM, rxcsum); - if (hw->mac.type == ixgbe_mac_82599EB) { + if (hw->mac.type == ixgbe_mac_82599EB || + hw->mac.type == ixgbe_mac_X540) { rdrxctl = IXGBE_READ_REG(hw, IXGBE_RDRXCTL); if (rx_conf->hw_strip_crc) rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;