| Message ID | 1460645904-19565-1-git-send-email-bjorn.topel@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Bruce Richardson |
| 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 1836D2BD3; Thu, 14 Apr 2016 16:58:51 +0200 (CEST) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 906F42BA1 for <dev@dpdk.org>; Thu, 14 Apr 2016 16:58:49 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 14 Apr 2016 07:58:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,485,1455004800"; d="scan'208";a="686261982" Received: from btopel-mobl1.isw.intel.com ([10.103.209.144]) by FMSMGA003.fm.intel.com with ESMTP; 14 Apr 2016 07:58:49 -0700 From: =?UTF-8?q?Bj=C3=B6rn=20T=C3=B6pel?= <bjorn.topel@intel.com> To: dev@dpdk.org Cc: =?UTF-8?q?Bj=C3=B6rn=20T=C3=B6pel?= <bjorn.topel@intel.com> Date: Thu, 14 Apr 2016 16:58:24 +0200 Message-Id: <1460645904-19565-1-git-send-email-bjorn.topel@intel.com> X-Mailer: git-send-email 2.7.4 MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Subject: [dpdk-dev] [PATCH] i40evf: Ignore disabled HW CRC strip for Linux PF hosts 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
Björn Töpel
April 14, 2016, 2:58 p.m. UTC
On Linux PF hosts, the VF has no means of changing the HW CRC strip
setting for a RX queue. It's implicitly enabled.
This patch ignores, and warns, if HW CRC stripping was disabled.
Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
---
drivers/net/i40e/i40e_ethdev_vf.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Comments
CC maintainers. On Thu, Apr 14, 2016 at 4:58 PM, Björn Töpel <bjorn.topel@intel.com> wrote: > On Linux PF hosts, the VF has no means of changing the HW CRC strip > setting for a RX queue. It's implicitly enabled. > > This patch ignores, and warns, if HW CRC stripping was disabled. > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > --- > drivers/net/i40e/i40e_ethdev_vf.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c > index 2bce69b..f88eb79 100644 > --- a/drivers/net/i40e/i40e_ethdev_vf.c > +++ b/drivers/net/i40e/i40e_ethdev_vf.c > @@ -1567,6 +1567,8 @@ i40evf_dev_configure(struct rte_eth_dev *dev) > { > struct i40e_adapter *ad = > I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private); > + struct rte_eth_conf *conf = &dev->data->dev_conf; > + struct i40e_vf *vf; > > /* Initialize to TRUE. If any of Rx queues doesn't meet the bulk > * allocation or vector Rx preconditions we will reset it. > @@ -1576,6 +1578,19 @@ i40evf_dev_configure(struct rte_eth_dev *dev) > ad->tx_simple_allowed = true; > ad->tx_vec_allowed = true; > > + /* For Linux PF hosts, VF has no ability to disable HW CRC strip, > + * and is implicitly enabled by the PF. > + */ > + if (!conf->rxmode.hw_strip_crc) { > + vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private); > + if ((vf->version_major == I40E_VIRTCHNL_VERSION_MAJOR) && > + (vf->version_minor <= I40E_VIRTCHNL_VERSION_MINOR)) { > + /* Peer is Linux PF host. */ > + PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip."); > + conf->rxmode.hw_strip_crc = 1; > + } > + } > + > return i40evf_init_vlan(dev); > } Not sure this is the right way to handle it. The driver should return an error rather than silently discard what the application asked. > > -- > 2.7.4 > > ---------------------------------------------------------------------- > Intel Sweden AB > Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden > Registration Number: 556189-6027 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. Please, remove this.
> -----Original Message----- > From: David Marchand [mailto:david.marchand@6wind.com] > Sent: Friday, April 15, 2016 3:41 PM > To: Topel, Bjorn <bjorn.topel@intel.com> > Cc: dev@dpdk.org; Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing > <jingjing.wu@intel.com> > Subject: Re: [dpdk-dev] [PATCH] i40evf: Ignore disabled HW CRC strip for Linux PF > hosts > > CC maintainers. > > On Thu, Apr 14, 2016 at 4:58 PM, Björn Töpel <bjorn.topel@intel.com> wrote: > > On Linux PF hosts, the VF has no means of changing the HW CRC strip > > setting for a RX queue. It's implicitly enabled. > > > > This patch ignores, and warns, if HW CRC stripping was disabled. > > > > Signed-off-by: Björn Töpel <bjorn.topel@intel.com> > > --- > > drivers/net/i40e/i40e_ethdev_vf.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c > > b/drivers/net/i40e/i40e_ethdev_vf.c > > index 2bce69b..f88eb79 100644 > > --- a/drivers/net/i40e/i40e_ethdev_vf.c > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c > > @@ -1567,6 +1567,8 @@ i40evf_dev_configure(struct rte_eth_dev *dev) { > > struct i40e_adapter *ad = > > > I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private); > > + struct rte_eth_conf *conf = &dev->data->dev_conf; > > + struct i40e_vf *vf; > > > > /* Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > * allocation or vector Rx preconditions we will reset it. > > @@ -1576,6 +1578,19 @@ i40evf_dev_configure(struct rte_eth_dev *dev) > > ad->tx_simple_allowed = true; > > ad->tx_vec_allowed = true; > > > > + /* For Linux PF hosts, VF has no ability to disable HW CRC strip, > > + * and is implicitly enabled by the PF. > > + */ > > + if (!conf->rxmode.hw_strip_crc) { > > + vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private); > > + if ((vf->version_major == I40E_VIRTCHNL_VERSION_MAJOR) > && > > + (vf->version_minor <= > I40E_VIRTCHNL_VERSION_MINOR)) { > > + /* Peer is Linux PF host. */ > > + PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC > Strip."); > > + conf->rxmode.hw_strip_crc = 1; > > + } > > + } > > + > > return i40evf_init_vlan(dev); > > } > > Not sure this is the right way to handle it. > The driver should return an error rather than silently discard what the application > asked. I also think it should return an error with checking if the host is kernel driver, and crc strip is disabled in VF. Thank you David! Regards, Helin > > > > > -- > > 2.7.4 > > > > ---------------------------------------------------------------------- > > Intel Sweden AB > > Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden > > Registration Number: 556189-6027 > > > > This e-mail and any attachments may contain confidential material for > > the sole use of the intended recipient(s). Any review or distribution > > by others is strictly prohibited. If you are not the intended > > recipient, please contact the sender and delete all copies. > > Please, remove this. > > > -- > David Marchand
>> Not sure this is the right way to handle it. The driver should >> return an error rather than silently discard what the >> application asked. > > I also think it should return an error with checking if the > host is kernel driver, and crc strip is disabled in VF. Thank > you David! Thanks for reviewing my patch, Helin and David. I agree that it's subtle to ignore the error, and just log the error. This is how ixgbevf behaves (refer to ixgbevf_dev_configure), so I figured that i40evf should behave analogous. I'll submit a v2 of the patch that returns an EINVAL and logs the failure. Would it make sense to change the ixgbevf_dev_configure as well, in a separate patch? >> ---------------------------------------------------------------------- >> Intel Sweden AB >> Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden >> Registration Number: 556189-6027 >> >> This e-mail and any attachments may contain confidential material for >> the sole use of the intended recipient(s). Any review or distribution >> by others is strictly prohibited. If you are not the intended >> recipient, please contact the sender and delete all copies. > > Please, remove this. Noted. Will make sure to fix that for future revisions. Thanks! Björn ---------------------------------------------------------------------- Intel Sweden AB Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden Registration Number: 556189-6027 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> -----Original Message----- > From: Topel, Bjorn > Sent: Tuesday, April 19, 2016 2:47 AM > To: Zhang, Helin <helin.zhang@intel.com>; David Marchand > <david.marchand@6wind.com> > Cc: dev@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com> > Subject: RE: [dpdk-dev] [PATCH] i40evf: Ignore disabled HW CRC strip for Linux PF > hosts > > >> Not sure this is the right way to handle it. The driver should > >> return an error rather than silently discard what the application > >> asked. > > > > I also think it should return an error with checking if the host is > > kernel driver, and crc strip is disabled in VF. Thank you David! > > Thanks for reviewing my patch, Helin and David. > > I agree that it's subtle to ignore the error, and just log the error. This is how > ixgbevf behaves (refer to ixgbevf_dev_configure), so I figured that i40evf should > behave analogous. > > I'll submit a v2 of the patch that returns an EINVAL and logs the failure. > > Would it make sense to change the ixgbevf_dev_configure as well, in a separate > patch? Yes, I agree with you that ixgbe and i40e should be consistent. Thank you! /Helin > > > >> --------------------------------------------------------------------- > >> - > >> Intel Sweden AB > >> Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, > >> Sweden Registration Number: 556189-6027 > >> > >> This e-mail and any attachments may contain confidential material for > >> the sole use of the intended recipient(s). Any review or distribution > >> by others is strictly prohibited. If you are not the intended > >> recipient, please contact the sender and delete all copies. > > > > Please, remove this. > > Noted. Will make sure to fix that for future revisions. Thanks! > > > Björn
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c index 2bce69b..f88eb79 100644 --- a/drivers/net/i40e/i40e_ethdev_vf.c +++ b/drivers/net/i40e/i40e_ethdev_vf.c @@ -1567,6 +1567,8 @@ i40evf_dev_configure(struct rte_eth_dev *dev) { struct i40e_adapter *ad = I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private); + struct rte_eth_conf *conf = &dev->data->dev_conf; + struct i40e_vf *vf; /* Initialize to TRUE. If any of Rx queues doesn't meet the bulk * allocation or vector Rx preconditions we will reset it. @@ -1576,6 +1578,19 @@ i40evf_dev_configure(struct rte_eth_dev *dev) ad->tx_simple_allowed = true; ad->tx_vec_allowed = true; + /* For Linux PF hosts, VF has no ability to disable HW CRC strip, + * and is implicitly enabled by the PF. + */ + if (!conf->rxmode.hw_strip_crc) { + vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private); + if ((vf->version_major == I40E_VIRTCHNL_VERSION_MAJOR) && + (vf->version_minor <= I40E_VIRTCHNL_VERSION_MINOR)) { + /* Peer is Linux PF host. */ + PMD_INIT_LOG(NOTICE, "VF can't disable HW CRC Strip."); + conf->rxmode.hw_strip_crc = 1; + } + } + return i40evf_init_vlan(dev); }