From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f41.google.com (mail-wg0-f41.google.com [74.125.82.41]) by dpdk.org (Postfix) with ESMTP id E7B48C33A for ; Tue, 2 Jun 2015 16:32:49 +0200 (CEST) Received: by wgbgq6 with SMTP id gq6so141957333wgb.3 for ; Tue, 02 Jun 2015 07:32:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=CutRvgn1pPRtSgvt9xEvBLVC8vxE85+5Fl3xmmq3fRw=; b=I6zPp0Ci/iFiQyz6ao56TjzUbj7KxdJTlgOdnIxkZX4k7mktC5dCtqq0ZI4feEr2HO 275mDGnZV44C85gs3NFG4TwF2PSzDrI45V8R1sIw5yCi87h5Qzo3LnrE83oo9ZlebkbL 6u+zuFYI+c05JixiarEAxw39HHOXlZcPpHOG5WP5BcKMALDC7GC2CEW2oJaOQ4EnjsTN COtUp/9ciFEG0lsr6R38t1NvbXWE9HY9SXN+Z4ngBFsu97gkFX8BzxNL/Z38XeMgUJ1e wic+650bRdyF53MfcUbq5zqQwITdEo0eVwWdTgsTlN931hvwxw0wUvEUJ8uB5hzIRMv+ oZ2g== X-Gm-Message-State: ALoCoQnhfDnMs5fSr18MvDVhq+dgBmnOpW3Pb2HIyqHXbsYezwEENz7gwEH5IxJRvD1K/0YWg6pP X-Received: by 10.180.90.202 with SMTP id by10mr31117669wib.62.1433255569743; Tue, 02 Jun 2015 07:32:49 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id m2sm21969691wiy.7.2015.06.02.07.32.48 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Jun 2015 07:32:48 -0700 (PDT) Date: Tue, 02 Jun 2015 07:32:48 -0700 (PDT) X-Google-Original-Date: Tue, 02 Jun 2015 16:32 +0200 From: Thomas Monjalon To: "Wang, Liang-min" Message-ID: <3076202.B6CvAKP4DR@xps13> Organization: 6WIND User-Agent: KMail/4.14.8 (Linux/4.0.4-2-ARCH; KDE/4.14.8; x86_64; ; ) In-Reply-To: References: <1432927612-12244-1-git-send-email-liang-min.wang@intel.com> <2827467.MLJnnY93Dx@xps13> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Jun 2015 14:32:50 -0000 Wang, hope it's clear that any new development is welcomed. One step before integration is to clearly explain why your code is needed. That's why a nack vote may help to discuss and decide. Comments below 2015-06-02 13:15, Wang, Liang-min: > >2015-05-29 15:26, Liang-Min Larry Wang: > >> adding a new library based upon ethdev APIs to provide API's that bear > >> the same functionality as ethtool_ops (linux/ethtool.h) and > >> net_device_ops (linux/netdevice.h). > >> > >> Signed-off-by: Liang-Min Larry Wang > >> --- > >> MAINTAINERS | 4 + > >> config/common_linuxapp | 5 + > >> lib/Makefile | 1 + > >> lib/librte_ethtool/Makefile | 56 +++++++ > >> lib/librte_ethtool/rte_ethtool.c | 155 +++++++++++++++++ > >> lib/librte_ethtool/rte_ethtool.h | 257 +++++++++++++++++++++++++++++ > >> lib/librte_ethtool/rte_ethtool_version.map | 18 ++ > >> mk/rte.app.mk | 1 + > > > >NACK for several reasons: > >- It's unclear what benefits this ethdev wrapper is bringing > > Since ethtool is provided to assist users migrating from kernel ethtool/net_device_op based design to user-space DPDK device management. The ethtool API's are created to closely maintain its original interface, therefore this library depends on . To avoid pollute the existing ethdev interface, a new library is created. To minimize code replication and maintain closely 1:1 API definition with kernel space API, this interface is designed based upon available ethdev APIs and add additional dev_ops if it's necessary. > > >- There is no obvious interest (how is it supposed to be used?) > There are already two acknowledge on this release. Earlier comment on this patch has that " ... The API's for ethtool like things are valuable ..." Stephen had some doubts about the real need and 2 people from Cisco (who never contributed before) give their ack without justification. Saying it's "valuable" or "very useful" is not enough. A new library needs to demonstrate in which scenario the added-value is. Sorry but you have to prove that it deserves to be maintained inside the dpdk project. > >- There is no update in the doc/ directory > Need more guidance on that. You probably have to add a new chapter in the programmer's guide. > >Other comments: > >- the patches are not versioned > > There is version file. Not sure what do you mean "the patches are not versioned" I mean there is no v2/v3 in the Subject. Please read http://dpdk.org/dev#send > >- the copyright starts in 2010 > > Will fix that. > > >I'm curious to understand how renaming rte_eth_dev_set_mtu() to > >rte_ethtool_net_change_mtu() will help anyone. > > As described, this interface is designed to provide API closely to kernel space ethtool ops and net_device_op. But the application still needs to adapt the code to call rte_* functions. So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.