Message ID | 1473389167-2758-1-git-send-email-zhouyates@gmail.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Ferruh Yigit |
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 8F59A37A6; Thu, 8 Sep 2016 16:21:44 +0200 (CEST) Received: from mail-pf0-f193.google.com (mail-pf0-f193.google.com [209.85.192.193]) by dpdk.org (Postfix) with ESMTP id ACB21379B for <dev@dpdk.org>; Thu, 8 Sep 2016 16:21:42 +0200 (CEST) Received: by mail-pf0-f193.google.com with SMTP id 128so2589576pfb.0 for <dev@dpdk.org>; Thu, 08 Sep 2016 07:21:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=2N5N/0EKjWDIvAVhjROzVfySy+4SH49wcRfdsMsHY6o=; b=CvuVPksUg9oLCJRs+wsYJpDU07fiT4b2XVOS2f5czrbSJNBCsO53GuYpmHZKhHG1Up fHrP0xnAxjKUxlyCvXzEDkzGCqJZ6xrp4eP6UCOwNSFplivlAcm7G+pBOQY66cGaPVBE DJ9x3pTnStGnyqwgKmwX40wxG7BAb6PZzn90kmG8898graG5shjDYVHd7FrUZnUa/o0h 0ZxkSn9BrZEApDvSEcLlA6tbpHaZdLx1u/IDROuLA+OpUYoN+wqq7RMILuLgw6L1P6GF VqMKbLu2YV5Au6ss8/vYcREWTxuq7atDV/RFH1qbEtI4X8NDnoSvODavLxorGji5Pdgc p6ww== 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; bh=2N5N/0EKjWDIvAVhjROzVfySy+4SH49wcRfdsMsHY6o=; b=LuaXLDl69jysn0HiNjhzJJlb+MRErsyKAvUBBHuePKtifwHbzCqhfe/wQ5/WUnCric lNKc2lsmDn2SwC+AzjhB4x1lZrx60LISarZS6TRHLoMtkQ+z8b2GjwJ1W7TnbMUkxv0h tLI5htjHDF0P2K5hpCy8FfRAor4f8T6P5b83nxp4SImjWfftBYwBysCJX1vlj48UuUPl KXIpyAFuMDlecoSS3bHn7BhsiOgEvXVwkF4ax9j0P2GLT8fP3KROXGTbRVC13IQdm8ZU +QuRRDVRwLzpsTXmIvlMp1GaLqTWPMHl5aiqIFsGqgfcmNEw/WSk2IfNVidluYou080Z 7rww== X-Gm-Message-State: AE9vXwNmwiuMQw4fahz7OamMvACyJCcfT/gwM0BvxYA43WYjBMv+mSY9+giDRVNv2+VNbA== X-Received: by 10.98.32.200 with SMTP id m69mr7201206pfj.15.1473344501672; Thu, 08 Sep 2016 07:21:41 -0700 (PDT) Received: from localhost.localdomain ([211.157.188.187]) by smtp.gmail.com with ESMTPSA id ty6sm56991369pac.18.2016.09.08.07.21.40 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Sep 2016 07:21:41 -0700 (PDT) From: zhouyangchao <zhouyates@gmail.com> To: ferruh.yigit@intel.com Cc: dev@dpdk.org, zhouyangchao <zhouyates@gmail.com> Date: Fri, 9 Sep 2016 10:46:07 +0800 Message-Id: <1473389167-2758-1-git-send-email-zhouyates@gmail.com> X-Mailer: git-send-email 1.7.1 Subject: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device could cause a kernel crash 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
Matt
Sept. 9, 2016, 2:46 a.m. UTC
Signed-off-by: zhouyangchao <zhouyates@gmail.com>
---
lib/librte_eal/linuxapp/kni/kni_misc.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
Comments
On Fri, 9 Sep 2016 10:46:07 +0800 zhouyangchao <zhouyates@gmail.com> wrote: > Signed-off-by: zhouyangchao <zhouyates@gmail.com> > --- > lib/librte_eal/linuxapp/kni/kni_misc.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c > index 67e9b7d..ad4e603 100644 > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c > @@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev) > igb_kni_remove(dev->pci_dev); > > if (dev->net_dev) { > - unregister_netdev(dev->net_dev); > + if (dev->netdev->reg_state == NETREG_REGISTERED){ ----------------------------------------------------------------^ Incorrect whitespace. But then again the whole KNI driver fails completely when running kernel style check.
On 9/9/2016 3:46 AM, zhouyangchao wrote: > Signed-off-by: zhouyangchao <zhouyates@gmail.com> > --- > lib/librte_eal/linuxapp/kni/kni_misc.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c > index 67e9b7d..ad4e603 100644 > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c > @@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev) > igb_kni_remove(dev->pci_dev); > > if (dev->net_dev) { > - unregister_netdev(dev->net_dev); > + if (dev->netdev->reg_state == NETREG_REGISTERED){ Although this will work, I believe we shouldn't know more about netdev internals unless we don't have to, and for this case we don't need to know it. More Linux internal knowledge means more ways to be broken in the future. Also I am not sure if reg_state intended to be used by network drivers. I am for first version of your patch, with missing free_netdev() added into rollback code. pseudo code: net_dev = alloc_netdev() ... ret = register_netdev() if (ret) kni_dev_remove() free_netdev() return kni->net_dev = net_dev OR if don't want to move where kni->net_dev assigned net_dev = alloc_netdev() kni->net_dev = net_dev ... ret = register_netdev() if (ret) kni->net_dev = NULL kni_dev_remove() free_netdev() return > + unregister_netdev(dev->net_dev); > + } > free_netdev(dev->net_dev); > } > >
On 9/8/2016 5:44 PM, Stephen Hemminger wrote: ... > But then again the whole KNI driver fails completely when > running kernel style check. > Yes, it generates lots of warnings. I can fix them (excluding ethtool/*), that wouldn't take much time but how syntax only patches welcomed? Another concern is it trashes git blame. Regards, ferruh
2016-09-08 18:15, Ferruh Yigit: > On 9/8/2016 5:44 PM, Stephen Hemminger wrote: > > ... > > > But then again the whole KNI driver fails completely when > > running kernel style check. > > > > Yes, it generates lots of warnings. > I can fix them (excluding ethtool/*), that wouldn't take much time but > how syntax only patches welcomed? Another concern is it trashes git blame. You ask a question and give the answer ;) I think it depends just on the balance of the pros/cons - to be evaluated.
> -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon > Sent: Friday, September 9, 2016 1:40 PM > To: Yigit, Ferruh <ferruh.yigit@intel.com> > Cc: Stephen Hemminger <stephen@networkplumber.org>; zhouyangchao > <zhouyates@gmail.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] kni: unregister an unregisterd net_device > could cause a kernel crash > > 2016-09-08 18:15, Ferruh Yigit: > > On 9/8/2016 5:44 PM, Stephen Hemminger wrote: > > > > ... > > > > > But then again the whole KNI driver fails completely when running > > > kernel style check. > > > > > > > Yes, it generates lots of warnings. > > I can fix them (excluding ethtool/*), that wouldn't take much time but > > how syntax only patches welcomed? Another concern is it trashes git > blame. > > You ask a question and give the answer ;) I think it depends just on the > balance of the pros/cons - to be evaluated. Hi, I think in general we would prefer to avoid any large scale code beautification since, as pointed out, it breaks the option to git blame. However, in the case of the KNI code the main author in git is "Intel" so git blame doesn't tell you a lot. Also, Ferruh is the maintainer, has made most of the recent changes, and is actively maintaining/improving it. So I think if the syntax fix came from him it would be okay. At least it would allow us to apply the checkpatch checks. John
2016-09-09 14:33, Mcnamara, John: > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon > > 2016-09-08 18:15, Ferruh Yigit: > > > On 9/8/2016 5:44 PM, Stephen Hemminger wrote: > > > > > > ... > > > > > > > But then again the whole KNI driver fails completely when running > > > > kernel style check. > > > > > > > > > > Yes, it generates lots of warnings. > > > I can fix them (excluding ethtool/*), that wouldn't take much time but > > > how syntax only patches welcomed? Another concern is it trashes git > > blame. > > > > You ask a question and give the answer ;) I think it depends just on the > > balance of the pros/cons - to be evaluated. > > Hi, > > I think in general we would prefer to avoid any large scale code beautification since, as pointed out, it breaks the option to git blame. > > However, in the case of the KNI code the main author in git is "Intel" so git blame doesn't tell you a lot. Also, Ferruh is the maintainer, has made most of the recent changes, and is actively maintaining/improving it. So I think if the syntax fix came from him it would be okay. At least it would allow us to apply the checkpatch checks. Yes seems reasonnable
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c index 67e9b7d..ad4e603 100644 --- a/lib/librte_eal/linuxapp/kni/kni_misc.c +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c @@ -361,7 +361,9 @@ kni_dev_remove(struct kni_dev *dev) igb_kni_remove(dev->pci_dev); if (dev->net_dev) { - unregister_netdev(dev->net_dev); + if (dev->netdev->reg_state == NETREG_REGISTERED){ + unregister_netdev(dev->net_dev); + } free_netdev(dev->net_dev); }