[dpdk-dev] [PATCH v9 1/3] Remove ABI requirement for external library builds.
> -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton > Sent: Friday, November 20, 2015 3:35 PM > To: dev at dpdk.org > Subject: [dpdk-dev] [PATCH v9 1/3] Remove ABI requirement for external > library builds. > > Signed-off-by: Andrew G. Harvey > --- > mk/rte.extlib.mk | 2 ++ > mk/rte.lib.mk| 6 ++ > 2 files changed, 8 insertions(+) > > diff --git a/mk/rte.extlib.mk b/mk/rte.extlib.mk > index ba066bc..4d459e4 100644 > --- a/mk/rte.extlib.mk > +++ b/mk/rte.extlib.mk > @@ -31,6 +31,8 @@ > > MAKEFLAGS += --no-print-directory > > +EXTLIB_BUILD := 1 > + > # we must create the output dir first and recall the same Makefile > # from this directory > ifeq ($(NOT_FIRST_CALL),) > diff --git a/mk/rte.lib.mk b/mk/rte.lib.mk > index fcc8e20..7262b02 100644 > --- a/mk/rte.lib.mk > +++ b/mk/rte.lib.mk > @@ -40,11 +40,13 @@ VPATH += $(SRCDIR) > > ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y) > LIB := $(patsubst %.a,%.so.$(LIBABIVER),$(LIB)) > +ifndef EXTLIB_BUILD > ifeq ($(CONFIG_RTE_NEXT_ABI),y) > LIB := $(LIB).1 > endif > CPU_LDFLAGS += --version-script=$(SRCDIR)/$(EXPORT_MAP) > endif > +endif > > > _BUILD = $(LIB) > @@ -174,12 +176,16 @@ $(RTE_OUTPUT)/lib/$(LIB): $(LIB) > @[ -d $(RTE_OUTPUT)/lib ] || mkdir -p $(RTE_OUTPUT)/lib > $(Q)cp -f $(LIB) $(RTE_OUTPUT)/lib > ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y) > +ifdef EXTLIB_BUILD > + $(Q)ln -s -f $< $(basename $@) > +else > ifeq ($(CONFIG_RTE_NEXT_ABI),y) > $(Q)ln -s -f $< $(basename $(basename $@)) > else > $(Q)ln -s -f $< $(basename $@) > endif > endif > +endif > > # > # Clean all generated files > -- > 1.9.3 ACK this patch as it has been reviewed.
[dpdk-dev] [PATCH v2 2/2] examples: new example: l2fwd-ethtool
Thomas, Could you explain why this patch is put on RFC? Thanks, Larry > -Original Message- > From: Wang, Liang-min > Sent: Wednesday, October 21, 2015 12:47 PM > To: 'Thomas Monjalon' > Cc: dev at dpdk.org; Andrew Harvey (agh) (agh at cisco.com) > Subject: RE: [dpdk-dev] [PATCH v2 2/2] examples: new example: l2fwd-ethtool > > Thomas, > Let's put this patch on defer list because there are related work might > take a different approach. Let's only review the make file change (PATCH 1/2). > I believe "export" is needed since the variable is shared by all the build > but it > might be already included due to the mk file inclusion. Since Andy is on > vacation, I am not sure if he could make a comment on that. > > Larry > > > -Original Message- > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > Sent: Wednesday, October 21, 2015 12:36 PM > > To: Wang, Liang-min > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v2 2/2] examples: new example: l2fwd- > > ethtool > > > > 2015-07-23 11:00, Liang-Min Larry Wang: > > > examples/Makefile|1 + > > > examples/l2fwd-ethtool/Makefile | 48 + > > > examples/l2fwd-ethtool/l2fwd-app/Makefile| 58 ++ > > > examples/l2fwd-ethtool/l2fwd-app/main.c | 1025 > > ++ > > > examples/l2fwd-ethtool/l2fwd-app/netdev_api.h| 770 > > > > > examples/l2fwd-ethtool/l2fwd-app/shared_fifo.h | 159 > > > examples/l2fwd-ethtool/lib/Makefile | 57 ++ > > > examples/l2fwd-ethtool/lib/rte_ethtool.c | 336 +++ > > > examples/l2fwd-ethtool/lib/rte_ethtool.h | 385 > > > examples/l2fwd-ethtool/nic-control/Makefile | 55 ++ > > > examples/l2fwd-ethtool/nic-control/nic_control.c | 614 + > > > 11 files changed, 3508 insertions(+) > > > > This patch is huge. > > Please split a bit. > > > > > --- a/examples/Makefile > > > +++ b/examples/Makefile > > > @@ -53,6 +53,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_KNI) += kni > > > DIRS-y += l2fwd > > > DIRS-$(CONFIG_RTE_LIBRTE_IVSHMEM) += l2fwd-ivshmem > > > DIRS-$(CONFIG_RTE_LIBRTE_JOBSTATS) += l2fwd-jobstats > > > +DIRS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += l2fwd-ethtool > > > DIRS-y += l3fwd > > > > Please keep the alphabetical order. > > > > I do not plan to review it more. > > If nobody complains, it means it's accepted.
[dpdk-dev] [PATCH v2 2/2] examples: new example: l2fwd-ethtool
Thomas, Let's put this patch on defer list because there are related work might take a different approach. Let's only review the make file change (PATCH 1/2). I believe "export" is needed since the variable is shared by all the build but it might be already included due to the mk file inclusion. Since Andy is on vacation, I am not sure if he could make a comment on that. Larry > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, October 21, 2015 12:36 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 2/2] examples: new example: l2fwd- > ethtool > > 2015-07-23 11:00, Liang-Min Larry Wang: > > examples/Makefile|1 + > > examples/l2fwd-ethtool/Makefile | 48 + > > examples/l2fwd-ethtool/l2fwd-app/Makefile| 58 ++ > > examples/l2fwd-ethtool/l2fwd-app/main.c | 1025 > ++ > > examples/l2fwd-ethtool/l2fwd-app/netdev_api.h| 770 > > > examples/l2fwd-ethtool/l2fwd-app/shared_fifo.h | 159 > > examples/l2fwd-ethtool/lib/Makefile | 57 ++ > > examples/l2fwd-ethtool/lib/rte_ethtool.c | 336 +++ > > examples/l2fwd-ethtool/lib/rte_ethtool.h | 385 > > examples/l2fwd-ethtool/nic-control/Makefile | 55 ++ > > examples/l2fwd-ethtool/nic-control/nic_control.c | 614 + > > 11 files changed, 3508 insertions(+) > > This patch is huge. > Please split a bit. > > > --- a/examples/Makefile > > +++ b/examples/Makefile > > @@ -53,6 +53,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_KNI) += kni > > DIRS-y += l2fwd > > DIRS-$(CONFIG_RTE_LIBRTE_IVSHMEM) += l2fwd-ivshmem > > DIRS-$(CONFIG_RTE_LIBRTE_JOBSTATS) += l2fwd-jobstats > > +DIRS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += l2fwd-ethtool > > DIRS-y += l3fwd > > Please keep the alphabetical order. > > I do not plan to review it more. > If nobody complains, it means it's accepted.
[dpdk-dev] [PATCH v17 0/5] User-space Ethtool
Thomas, Thanks for clarification. Our next step is to create hooks on virtio and vmxnet3 and other ethtool op. Thanks again for making prompt reply as I am aware it's very late in your time zone. Larry > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Thursday, July 16, 2015 6:09 PM > To: Wang, Liang-min > Cc: dev at dpdk.org; O'Driscoll, Tim > Subject: Re: [dpdk-dev] [PATCH v17 0/5] User-space Ethtool > > 2015-07-16 21:55, Wang, Liang-min: > > Thomas, > > Do you want me to create a separate patch just include the > example/l2fwd-ethtool? > > Yes > > > Do you also mean besides the identified Makefiles, you see more > rework needs to be done, > > or I just need to fix Makefile issue? If just Makefile issue, I could > > try > to make another attempt tomorrow? > > After checking the build there is probably more review to do. > Let's take more time to have something clean and maybe more complete in > 2.2. > This patchset is your first contribution to DPDK and is already a nice > achievement. > The new API must now be implemented in more drivers to be effective. > > > > > -Original Message- > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > > Sent: Thursday, July 16, 2015 5:48 PM > > > To: Wang, Liang-min > > > Cc: dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH v17 0/5] User-space Ethtool > > > > > > 2015-07-16 09:25, Liang-Min Larry Wang: > > > > This implementation is designed to provide a familar interface for > > > applications that rely on kernel-space driver to support ethtool_op and > > > net_device_op for device management. The initial implementation > focuses > > > on ops that can be implemented through existing netdev APIs. More ops > will > > > be supported in latter release. > > > > > > Applied without example which needs more work, thanks >
[dpdk-dev] [PATCH v17 0/5] User-space Ethtool
Thomas, Do you want me to create a separate patch just include the example/l2fwd-ethtool? Do you also mean besides the identified Makefiles, you see more rework needs to be done, or I just need to fix Makefile issue? If just Makefile issue, I could try to make another attempt tomorrow? Thanks, Larry > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Thursday, July 16, 2015 5:48 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v17 0/5] User-space Ethtool > > 2015-07-16 09:25, Liang-Min Larry Wang: > > This implementation is designed to provide a familar interface for > applications that rely on kernel-space driver to support ethtool_op and > net_device_op for device management. The initial implementation focuses > on ops that can be implemented through existing netdev APIs. More ops will > be supported in latter release. > > Applied without example which needs more work, thanks
[dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access device info
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, July 15, 2015 7:21 AM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access > device info > > 2015-07-15 10:48, Wang, Liang-min: > > > > > What means magic? Is it always a device id? > > > > This field is the same as defined in kernel ethtool data structure. > > > > > > Not an excellent argument :) > > > Actually you didn't answer. > > > > > I could not speak for other devices. For igb and ixgbe, it's device id. > > Based upon literature search, this field is used by device driver to provide > extra information. > > What would be your suggestion? > > I don't have any suggestion. > You are defining a new API in DPDK, so our concern must be to check > that it will be easily understood by users and PMD developers. > If you don't really know what this field will be for, others won't know. No, I meant it's device driver dependent. If I keep the same naming convention following kernel ethtool defined, and adding comment like "device-specific magic key, such as device-id". Would that be acceptable from you? This data structure is defined to be compatible with kernel ethtool API, and magic key is device specific and known to ethtool API users.
[dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access device info
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, July 15, 2015 6:27 AM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access
> device info
>
> 2015-07-15 10:07, Wang, Liang-min:
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > Please precise offset from which point?
> > > Why offset is needed?
> > >
> > Is C always 0-base?
> > The offset is introduced because one of the review requesting to support
> partial register read,
> > meaning reading single register or a set of registers.
> > As comment in my reply, this implementation only supports total register
> dump,
> > but the data structure design allows future expansion to support this
> request.
>
> OK, so the comment should be something like "Offset in register table"
>
> > > What means magic? Is it always a device id?
> > This field is the same as defined in kernel ethtool data structure.
>
> Not an excellent argument :)
> Actually you didn't answer.
>
I could not speak for other devices. For igb and ixgbe, it's device id.
Based upon literature search, this field is used by device driver to provide
extra information.
What would be your suggestion?
> > > > --- a/lib/librte_ether/rte_ether_version.map
> > > > +++ b/lib/librte_ether/rte_ether_version.map
> > > > @@ -114,5 +114,11 @@ DPDK_2.1 {
> > > > rte_eth_timesync_enable;
> > > > rte_eth_timesync_read_rx_timestamp;
> > > > rte_eth_timesync_read_tx_timestamp;
> > > > + rte_eth_dev_default_mac_addr_set;
> > > > + rte_eth_dev_reg_length;
> > > > + rte_eth_dev_reg_info;
> > > > + rte_eth_dev_eeprom_length;
> > > > + rte_eth_dev_get_eeprom;
> > > > + rte_eth_dev_set_eeprom;
> > >
> > > It is not in alphabetical order.
> > Is there a document on such requirement?
>
> No, only emails.
> Every .map are alphabetically sorted. It's easier to check symbols.
>
> > I'm asking this question because different API's are added at different
> time.
> > Does this comment apply on relative order or absolute order meaning
> order relative to mainline code?
>
> Relative to mainline. Conflicts are handled when applying.
[dpdk-dev] [PATCH v16 0/6] User-space Ethtool
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Wednesday, July 15, 2015 1:53 AM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v16 0/6] User-space Ethtool > > 2015-07-14 20:56, Wang, Liang-min: > > Thomas, > > The mac_addr_set is needed for ethtool net-op as described in > > examples/ethtool-ethtool/lib/rte_ethtool.c > (rte_ethtool_net_set/get_mac_addr). > > That's why it should be the first patch of this series. > You probably misread "another patch" which means different patch in same > series. > That's why this API and rest of API's added into librte_ether are all prepared in the same patch: 001. I'm confused on your comment here. Are you asking me to break the patch 001 into two patches in the same series? Meaning 001 and 002, would that be more confusing? > > Could you use the v15 for the release? > > No, there are more comments. > > > > > -Original Message- > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > > Sent: Tuesday, July 14, 2015 4:14 PM > > > To: Wang, Liang-min > > > Cc: dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH v16 0/6] User-space Ethtool > > > > > > 2015-07-14 09:11, Liang-Min Larry Wang: > > > > v16 changes: > > > > - Re-do patch v15 and separate changes between v15 and v14 into > patch > > > files 005 and 006 > > > > > > No, you misunderstood the need. > > > You must merge your fixes in appropriate patches. > > > But you must separate the mac_addr_set feature in another patch as it is > > > not related to ethtool dumps. > > > > > > You are winning the record of patch iterations :) >
[dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access device info
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Wednesday, July 15, 2015 2:16 AM
> To: Wang, Liang-min
> Cc: dev at dpdk.org; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access
> device info
>
> 2015-07-13 22:18, Liang-Min Larry Wang:
> > --- a/lib/librte_ether/Makefile
> > +++ b/lib/librte_ether/Makefile
> > @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c
> > SYMLINK-y-include += rte_ether.h
> > SYMLINK-y-include += rte_ethdev.h
> > SYMLINK-y-include += rte_eth_ctrl.h
> > +SYMLINK-y-include += rte_eth_dev_info.h
>
> This file is not related to ethernet so it could be named rte_dev_info.h
>
>
> > +struct rte_dev_reg_info {
> > + void *buf; /**< Buffer for register */
>
> Maybe data would be more accurate.
>
> > + uint32_t offset; /**< Offset for 1st register to fetch */
>
> Please precise offset from which point?
> Why offset is needed?
>
Is C always 0-base?
The offset is introduced because one of the review requesting to support
partial register read,
meaning reading single register or a set of registers.
As comment in my reply, this implementation only supports total register dump,
but the data structure design allows future expansion to support this request.
> > + uint32_t leng; /**< Number of registers to fetch */
>
> 2 more characters for free: length
>
>
> > +struct rte_dev_eeprom_info {
> > + void *buf; /**< Buffer for eeprom */
> > + uint32_t offset; /**< Offset for 1st eeprom location to access */
> > + uint32_t leng; /**< Length of eeprom region to access */
>
> Same as above for these 3 fields.
>
> > + uint32_t magic; /**< Device ID */
>
> What means magic? Is it always a device id?
This field is the same as defined in kernel ethtool data structure.
>
>
> > --- a/lib/librte_ether/rte_ether_version.map
> > +++ b/lib/librte_ether/rte_ether_version.map
> > @@ -114,5 +114,11 @@ DPDK_2.1 {
> > rte_eth_timesync_enable;
> > rte_eth_timesync_read_rx_timestamp;
> > rte_eth_timesync_read_tx_timestamp;
> > + rte_eth_dev_default_mac_addr_set;
> > + rte_eth_dev_reg_length;
> > + rte_eth_dev_reg_info;
> > + rte_eth_dev_eeprom_length;
> > + rte_eth_dev_get_eeprom;
> > + rte_eth_dev_set_eeprom;
>
> It is not in alphabetical order.
Is there a document on such requirement?
I'm asking this question because different API's are added at different time.
Does this comment apply on relative order or absolute order meaning order
relative to mainline code?
[dpdk-dev] [PATCH v16 0/6] User-space Ethtool
Thomas, The mac_addr_set is needed for ethtool net-op as described in examples/ethtool-ethtool/lib/rte_ethtool.c (rte_ethtool_net_set/get_mac_addr). Could you use the v15 for the release? Larry > -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Tuesday, July 14, 2015 4:14 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v16 0/6] User-space Ethtool > > 2015-07-14 09:11, Liang-Min Larry Wang: > > v16 changes: > > - Re-do patch v15 and separate changes between v15 and v14 into patch > files 005 and 006 > > No, you misunderstood the need. > You must merge your fixes in appropriate patches. > But you must separate the mac_addr_set feature in another patch as it is > not related to ethtool dumps. > > You are winning the record of patch iterations :)
[dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to const
Stephen, > -Original Message- > From: Stephen Hemminger [mailto:stephen at networkplumber.org] > Sent: Friday, June 26, 2015 9:37 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to > const > > On Fri, 26 Jun 2015 21:19:08 -0400 > Liang-Min Larry Wang wrote: > > > Changed registers tables in ixgbe and igb to const type. > > > > Signed-off-by: Andrew G. Harvey > > Signed-off-by: Liang-Min Larry Wang > > Since code is not yet accepted, all these changes should be rolled back into > next (v10) of the original patch. On v10, I had made changes according to your feedback. I did more changes on v11/v12/v13 based upon incremental changes on master repository. Are there any pending issue on this patch? Thanks, Larry
[dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to const
> -Original Message- > From: Stephen Hemminger [mailto:stephen at networkplumber.org] > Sent: Friday, June 26, 2015 9:37 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to > const > > On Fri, 26 Jun 2015 21:19:08 -0400 > Liang-Min Larry Wang wrote: > > > Changed registers tables in ixgbe and igb to const type. > > > > Signed-off-by: Andrew G. Harvey > > Signed-off-by: Liang-Min Larry Wang > > Since code is not yet accepted, all these changes should be rolled back into > next (v10) of the original patch. All the const changes are moved to the same patches on igb or ixgbe.
[dpdk-dev] [PATCH v9 2/5] ixgbe: add ops to support ethtool ops
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, June 26, 2015 9:34 PM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v9 2/5] ixgbe: add ops to support ethtool ops
>
> On Fri, 26 Jun 2015 21:19:05 -0400
> Liang-Min Larry Wang wrote:
>
> > + reg_group = reg_set[g_ind++];
> > + while (reg_group) {
> > + count += ixgbe_regs_group_count(reg_group);
> > + reg_group = reg_set[g_ind++];
> > + }
>
> I don't care what checkpatch says, this an example of a loop
> which reads better as:
>
> while ((reg_group = reg_set[g_ind++]))
>count += ixgbe_regs_group_count(reg_group);
There is no specific guideline on which to follow besides running checkpatch.
Maybe this type of exception should be listed on dpdk web-link.
This suggestion is taken on v10.
[dpdk-dev] [PATCH v9 2/5] ixgbe: add ops to support ethtool ops
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, June 26, 2015 9:33 PM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v9 2/5] ixgbe: add ops to support ethtool ops
>
> On Fri, 26 Jun 2015 21:19:05 -0400
> Liang-Min Larry Wang wrote:
>
> > +static int
> > +ixgbe_get_reg_length(struct rte_eth_dev *dev __rte_unused)
> > +{
> > + struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private)
>
> The dev arg is used, and therefore should not be marked __rte_unused.
Fixed at v10.
[dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to const
> -Original Message- > From: Stephen Hemminger [mailto:stephen at networkplumber.org] > Sent: Friday, June 26, 2015 9:37 PM > To: Wang, Liang-min > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH v9 5/5] ixgbe/igb: changed register tables to > const > > On Fri, 26 Jun 2015 21:19:08 -0400 > Liang-Min Larry Wang wrote: > > > Changed registers tables in ixgbe and igb to const type. > > > > Signed-off-by: Andrew G. Harvey > > Signed-off-by: Liang-Min Larry Wang > > Since code is not yet accepted, all these changes should be rolled back into > next (v10) of the original patch. Are you saying that we could not have a 5/5 needs to be applied to 2/5 and 3/5
[dpdk-dev] [PATCH v8 1/5] ethdev: add apis to support access device info
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, June 26, 2015 12:52 PM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v8 1/5] ethdev: add apis to support access
> device info
>
> On Fri, 26 Jun 2015 10:26:43 -0400
> Liang-Min Larry Wang wrote:
>
> > add new apis:
> > - rte_eth_dev_default_mac_addr_set
> > - rte_eth_dev_reg_length
> > - rte_eth_dev_reg_info
> > - rte_eth_dev_eeprom_length
> > - rte_eth_dev_get_eeprom
> > - rte_eth_dev_set_eeprom
> >
> > to enable reading device parameters (mac-addr, register,
> > eeprom) based upon ethtool alike
> > data parameter specification.
> >
> > Signed-off-by: Liang-Min Larry Wang
>
> I agree in principal, but has lots of style issues (see report from
> checkpatch).
>
> ERROR: spaces required around that '=' (ctx:VxW)
> #193: FILE: lib/librte_ether/rte_ethdev.c:3677:
> + if ((dev= _eth_devices[port_id]) == NULL) {
> ^
>
> ERROR: do not use assignment in if condition
> #193: FILE: lib/librte_ether/rte_ethdev.c:3677:
> + if ((dev= _eth_devices[port_id]) == NULL) {
>
> ERROR: spaces required around that '=' (ctx:VxW)
> #213: FILE: lib/librte_ether/rte_ethdev.c:3697:
> + if ((dev= _eth_devices[port_id]) == NULL) {
> ^
>
> ERROR: do not use assignment in if condition
> #213: FILE: lib/librte_ether/rte_ethdev.c:3697:
> + if ((dev= _eth_devices[port_id]) == NULL) {
>
> ERROR: spaces required around that '=' (ctx:VxW)
> #232: FILE: lib/librte_ether/rte_ethdev.c:3716:
> + if ((dev= _eth_devices[port_id]) == NULL) {
> ^
>
> ERROR: do not use assignment in if condition
> #232: FILE: lib/librte_ether/rte_ethdev.c:3716:
> + if ((dev= _eth_devices[port_id]) == NULL) {
>
> ERROR: spaces required around that '=' (ctx:VxW)
> #251: FILE: lib/librte_ether/rte_ethdev.c:3735:
> + if ((dev= _eth_devices[port_id]) == NULL) {
> ^
>
> ERROR: do not use assignment in if condition
> #251: FILE: lib/librte_ether/rte_ethdev.c:3735:
> + if ((dev= _eth_devices[port_id]) == NULL) {
>
> ERROR: spaces required around that '=' (ctx:VxW)
> #270: FILE: lib/librte_ether/rte_ethdev.c:3754:
> + if ((dev= _eth_devices[port_id]) == NULL) {
> ^
>
> ERROR: do not use assignment in if condition
> #270: FILE: lib/librte_ether/rte_ethdev.c:3754:
> + if ((dev= _eth_devices[port_id]) == NULL) {
Issues addressed over v9.
[dpdk-dev] [PATCH v7 1/4] ethdev: add apis to support access device info
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Thursday, June 25, 2015 9:44 AM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7 1/4] ethdev: add apis to support access
> device info
>
> On Wed, 17 Jun 2015 18:22:12 -0400
> Liang-Min Larry Wang wrote:
>
> > +int
> > +rte_eth_dev_reg_length(uint8_t port_id)
> > +{
> > + struct rte_eth_dev *dev;
> > +
> > + if ((dev= _eth_devices[port_id]) == NULL) {
> > + PMD_DEBUG_TRACE("Invalid port device\n");
> > + return -ENODEV;
> > + }
>
> Some minor nits:
> * for consistency you should add valid port check here.
> * style:
> - don't do assignment in if() unless it really helps readability
> - need whitespace
>
> if (!rte_eth_dev_is_valid_port(portid)) {
> PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> return -ENODEV;
> }
>
> dev = _eth_devices[port_id];
> if (dev == NULL) {
> PMD_DEBUG("Invalid port device\n");
> return -ENODEV:
> }
> ...
>
> This code pattern is so common it really should be a function.
>
> dev = rte_eth_dev_get(port_id);
> if (dev == NULL) {
> PMD_DEBUG("Invalid port device\n");
> return -ENODEV;
> }
>
> And then add a macro to generate this??
This is used through-out the rte_ethdev.c, should it be done to the entire file?
[dpdk-dev] [PATCH v7 1/4] ethdev: add apis to support access device info
> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Thursday, June 25, 2015 9:40 AM
> To: Wang, Liang-min
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7 1/4] ethdev: add apis to support access
> device info
>
> On Wed, 17 Jun 2015 18:22:12 -0400
> Liang-Min Larry Wang wrote:
>
> > int
> > +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr
> *addr)
> > +{
> > + struct rte_eth_dev *dev;
> > +
> > + if (!rte_eth_dev_is_valid_port(port_id)) {
> > + PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > + return -ENODEV;
> > + }
> > +
> > + if (!is_valid_assigned_ether_addr(addr))
> > + return -EINVAL;
> > +
> > + dev = _eth_devices[port_id];
> > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -
> ENOTSUP);
> > +
> > + /* Update default address in NIC data structure */
> > + ether_addr_copy(addr, >data->mac_addrs[0]);
> > +
> > + (*dev->dev_ops->mac_addr_set)(dev, addr);
>
> Would it be possible to directly set mac_addr[0] if device does not
> provide a device driver specific override?
I would yield this question to Konstantin since this information is used by get
mac addr API.
[dpdk-dev] [PATCH v7 0/4] User-space Ethtool
Stephen, From: Wang, Liang-min Sent: Thursday, June 18, 2015 8:47 AM To: Stephen Hemminger Cc: dev at dpdk.org Subject: RE: [dpdk-dev] [PATCH v7 0/4] User-space Ethtool >>I agree with having a more complete API, but have some nits to pick. >>Could the API be more abstract to reduce ABI issues in future? >Which API? Are you referring to the APIs over ethdev level, or something else? >More abstract on input/output data structure definition or else? Could you be >more specific? >>I know choosing names is hard, but as a Linux developer ethtool has a very >>specific meaning to me. >>This API encompasses things broader than Linux ethtool and has different >>semantics therefore >>not sure having something in DPDK with same name is really a good idea. >> >>It would be better to call it something else like netdev_?? Or dpnet_?? >Just to clarify the naming suggestion, in this patch, the prefix ?ethtool? >only appears on example and on this patch description. >Are you suggesting changing the name over example/l2fwd-ethtool or on this >patch description, or may be both? Have not heard your feedback on last request?
[dpdk-dev] [PATCH v7 0/4] User-space Ethtool
>I agree with having a more complete API, but have some nits to pick. >Could the API be more abstract to reduce ABI issues in future? Which API? Are you referring to the APIs over ethdev level, or something else? More abstract on input/output data structure definition or else? Could you be more specific? >I know choosing names is hard, but as a Linux developer ethtool has a very >specific meaning to me. >This API encompasses things broader than Linux ethtool and has different >semantics therefore >not sure having something in DPDK with same name is really a good idea. > >It would be better to call it something else like netdev_?? Or dpnet_?? Just to clarify the naming suggestion, in this patch, the prefix ?ethtool? only appears on example and on this patch description. Are you suggesting changing the name over example/l2fwd-ethtool or on this patch description, or may be both?
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
> -Original Message- > From: Ananyev, Konstantin > Sent: Monday, June 15, 2015 9:46 AM > To: Wang, Liang-min; dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access > device info > > > > > -Original Message- > > From: Wang, Liang-min > > Sent: Monday, June 15, 2015 2:26 PM > > To: Ananyev, Konstantin; dev at dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > access device info > > > > > > > > > -Original Message- > > > From: Ananyev, Konstantin > > > Sent: Friday, June 12, 2015 8:31 AM > > > To: Wang, Liang-min; dev at dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > access device info > > > > > > > > > > > > > -Original Message- > > > > From: Wang, Liang-min > > > > Sent: Thursday, June 11, 2015 10:51 PM > > > > To: Ananyev, Konstantin; dev at dpdk.org > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > access device info > > > > > > > > > > > > > > > > > -Original Message- > > > > > From: Ananyev, Konstantin > > > > > Sent: Thursday, June 11, 2015 9:07 AM > > > > > To: Wang, Liang-min; dev at dpdk.org > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > support access device info > > > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > From: Wang, Liang-min > > > > > > Sent: Thursday, June 11, 2015 1:58 PM > > > > > > To: Ananyev, Konstantin; dev at dpdk.org > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > support access device info > > > > > > > > > > > > Hi Konstantin, > > > > > > > > > > > > > -Original Message- > > > > > > > From: Ananyev, Konstantin > > > > > > > Sent: Thursday, June 11, 2015 8:26 AM > > > > > > > To: Wang, Liang-min; dev at dpdk.org > > > > > > > Cc: Wang, Liang-min > > > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > support access device info > > > > > > > > > > > > > > Hi Larry, > > > > > > > > > > > > > > > -Original Message- > > > > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of > > > > > > > > Liang-Min Larry Wang > > > > > > > > Sent: Wednesday, June 10, 2015 4:10 PM > > > > > > > > To: dev at dpdk.org > > > > > > > > Cc: Wang, Liang-min > > > > > > > > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > > > > support access device info > > > > > > > > > > > > > > > > add new apis: > > > > > > > > - rte_eth_dev_default_mac_addr_set > > > > > > > > - rte_eth_dev_reg_leng > > > > > > > > - rte_eth_dev_reg_info > > > > > > > > - rte_eth_dev_eeprom_leng > > > > > > > > - rte_eth_dev_get_eeprom > > > > > > > > - rte_eth_dev_set_eeprom > > > > > > > > - rte_eth_dev_get_ringparam > > > > > > > > - rte_eth_dev_set_ringparam > > > > > > > > > > > > > > > > to enable reading device parameters (mac-addr, register, > > > > > > > > eeprom, > > > > > > > > ring) based upon ethtool alike data parameter specification. > > > > > > > > > > > > > > > > Signed-off-by: Liang-Min Larry Wang > > > > > > > > > > > > > > > > --- > > > > > > > > lib/librte_ether/Makefile | 1 + > > > > > > > > lib/librte_ether/rte_eth_dev_info.h| 80 > + > > > > > > > > lib/librte_ether/rte_ethdev.c | 159 > > > > > > > + > > > > > > > > lib/librte_ether/rte_ethdev.h | 158 > > > >
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
> -Original Message- > From: Ananyev, Konstantin > Sent: Friday, June 12, 2015 8:31 AM > To: Wang, Liang-min; dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access > device info > > > > > -Original Message- > > From: Wang, Liang-min > > Sent: Thursday, June 11, 2015 10:51 PM > > To: Ananyev, Konstantin; dev at dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > access device info > > > > > > > > > -Original Message----- > > > From: Ananyev, Konstantin > > > Sent: Thursday, June 11, 2015 9:07 AM > > > To: Wang, Liang-min; dev at dpdk.org > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > access device info > > > > > > > > > > > > > -Original Message- > > > > From: Wang, Liang-min > > > > Sent: Thursday, June 11, 2015 1:58 PM > > > > To: Ananyev, Konstantin; dev at dpdk.org > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > access device info > > > > > > > > Hi Konstantin, > > > > > > > > > -Original Message- > > > > > From: Ananyev, Konstantin > > > > > Sent: Thursday, June 11, 2015 8:26 AM > > > > > To: Wang, Liang-min; dev at dpdk.org > > > > > Cc: Wang, Liang-min > > > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to > > > > > support access device info > > > > > > > > > > Hi Larry, > > > > > > > > > > > -Original Message- > > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Liang-Min > > > > > > Larry Wang > > > > > > Sent: Wednesday, June 10, 2015 4:10 PM > > > > > > To: dev at dpdk.org > > > > > > Cc: Wang, Liang-min > > > > > > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > > > access device info > > > > > > > > > > > > add new apis: > > > > > > - rte_eth_dev_default_mac_addr_set > > > > > > - rte_eth_dev_reg_leng > > > > > > - rte_eth_dev_reg_info > > > > > > - rte_eth_dev_eeprom_leng > > > > > > - rte_eth_dev_get_eeprom > > > > > > - rte_eth_dev_set_eeprom > > > > > > - rte_eth_dev_get_ringparam > > > > > > - rte_eth_dev_set_ringparam > > > > > > > > > > > > to enable reading device parameters (mac-addr, register, > > > > > > eeprom, > > > > > > ring) based upon ethtool alike data parameter specification. > > > > > > > > > > > > Signed-off-by: Liang-Min Larry Wang > > > > > > --- > > > > > > lib/librte_ether/Makefile | 1 + > > > > > > lib/librte_ether/rte_eth_dev_info.h| 80 + > > > > > > lib/librte_ether/rte_ethdev.c | 159 > > > > > + > > > > > > lib/librte_ether/rte_ethdev.h | 158 > > > > > > > > > > > lib/librte_ether/rte_ether_version.map | 8 ++ > > > > > > 5 files changed, 406 insertions(+) create mode 100644 > > > > > > lib/librte_ether/rte_eth_dev_info.h > > > > > > > > > > > > diff --git a/lib/librte_ether/Makefile > > > > > > b/lib/librte_ether/Makefile index c0e5768..05209e9 100644 > > > > > > --- a/lib/librte_ether/Makefile > > > > > > +++ b/lib/librte_ether/Makefile > > > > > > @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c SYMLINK-y-include += > > > > > > rte_ether.h SYMLINK-y-include += rte_ethdev.h > > > > > > SYMLINK-y-include > > > > > > += rte_eth_ctrl.h > > > > > > +SYMLINK-y-include += rte_eth_dev_info.h > > > > > > > > > > > > # this lib depends upon: > > > > > > DEPDIRS-y += lib/librte_eal lib/librte_mempool > > > > > > lib/librte_ring lib/librte_mbuf diff --git > > > > > > a/lib/librte_ether/rte_eth_dev_info.h > > > > > > b/lib/librte_ether/rte_eth_dev_info.h > > > > > > new fi
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
> -Original Message- > From: Ananyev, Konstantin > Sent: Thursday, June 11, 2015 9:07 AM > To: Wang, Liang-min; dev at dpdk.org > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access > device info > > > > > -Original Message- > > From: Wang, Liang-min > > Sent: Thursday, June 11, 2015 1:58 PM > > To: Ananyev, Konstantin; dev at dpdk.org > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > access device info > > > > Hi Konstantin, > > > > > -Original Message----- > > > From: Ananyev, Konstantin > > > Sent: Thursday, June 11, 2015 8:26 AM > > > To: Wang, Liang-min; dev at dpdk.org > > > Cc: Wang, Liang-min > > > Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > access device info > > > > > > Hi Larry, > > > > > > > -Original Message----- > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Liang-Min > > > > Larry Wang > > > > Sent: Wednesday, June 10, 2015 4:10 PM > > > > To: dev at dpdk.org > > > > Cc: Wang, Liang-min > > > > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support > > > > access device info > > > > > > > > add new apis: > > > > - rte_eth_dev_default_mac_addr_set > > > > - rte_eth_dev_reg_leng > > > > - rte_eth_dev_reg_info > > > > - rte_eth_dev_eeprom_leng > > > > - rte_eth_dev_get_eeprom > > > > - rte_eth_dev_set_eeprom > > > > - rte_eth_dev_get_ringparam > > > > - rte_eth_dev_set_ringparam > > > > > > > > to enable reading device parameters (mac-addr, register, eeprom, > > > > ring) based upon ethtool alike data parameter specification. > > > > > > > > Signed-off-by: Liang-Min Larry Wang > > > > --- > > > > lib/librte_ether/Makefile | 1 + > > > > lib/librte_ether/rte_eth_dev_info.h| 80 + > > > > lib/librte_ether/rte_ethdev.c | 159 > > > + > > > > lib/librte_ether/rte_ethdev.h | 158 > > > > > > > lib/librte_ether/rte_ether_version.map | 8 ++ > > > > 5 files changed, 406 insertions(+) create mode 100644 > > > > lib/librte_ether/rte_eth_dev_info.h > > > > > > > > diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile > > > > index c0e5768..05209e9 100644 > > > > --- a/lib/librte_ether/Makefile > > > > +++ b/lib/librte_ether/Makefile > > > > @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c SYMLINK-y-include += > > > > rte_ether.h SYMLINK-y-include += rte_ethdev.h SYMLINK-y-include > > > > += rte_eth_ctrl.h > > > > +SYMLINK-y-include += rte_eth_dev_info.h > > > > > > > > # this lib depends upon: > > > > DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring > > > > lib/librte_mbuf diff --git a/lib/librte_ether/rte_eth_dev_info.h > > > > b/lib/librte_ether/rte_eth_dev_info.h > > > > new file mode 100644 > > > > index 000..002c4b5 > > > > --- /dev/null > > > > +++ b/lib/librte_ether/rte_eth_dev_info.h > > > > @@ -0,0 +1,80 @@ > > > > +/*- > > > > + * BSD LICENSE > > > > + * > > > > + * Copyright(c) 2015 Intel Corporation. All rights reserved. > > > > + * All rights reserved. > > > > + * > > > > + * Redistribution and use in source and binary forms, with or without > > > > + * modification, are permitted provided that the following conditions > > > > + * are met: > > > > + * > > > > + * * Redistributions of source code must retain the above copyright > > > > + * notice, this list of conditions and the following disclaimer. > > > > + * * Redistributions in binary form must reproduce the above > copyright > > > > + * notice, this list of conditions and the following disclaimer > > > > in > > > > + * the documentation and/or other materials provided with the > > > > + * distribution. > > > > + * * Neither the name of Intel Corporation nor the names of its > > > > + * contributors may be used to endorse or promote products > derived > > > > + * from this software wit
[dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access device info
Hi Konstantin,
> -Original Message-
> From: Ananyev, Konstantin
> Sent: Thursday, June 11, 2015 8:26 AM
> To: Wang, Liang-min; dev at dpdk.org
> Cc: Wang, Liang-min
> Subject: RE: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access
> device info
>
> Hi Larry,
>
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Liang-Min Larry
> > Wang
> > Sent: Wednesday, June 10, 2015 4:10 PM
> > To: dev at dpdk.org
> > Cc: Wang, Liang-min
> > Subject: [dpdk-dev] [PATCH v4 1/4] ethdev: add apis to support access
> > device info
> >
> > add new apis:
> > - rte_eth_dev_default_mac_addr_set
> > - rte_eth_dev_reg_leng
> > - rte_eth_dev_reg_info
> > - rte_eth_dev_eeprom_leng
> > - rte_eth_dev_get_eeprom
> > - rte_eth_dev_set_eeprom
> > - rte_eth_dev_get_ringparam
> > - rte_eth_dev_set_ringparam
> >
> > to enable reading device parameters (mac-addr, register, eeprom, ring)
> > based upon ethtool alike data parameter specification.
> >
> > Signed-off-by: Liang-Min Larry Wang
> > ---
> > lib/librte_ether/Makefile | 1 +
> > lib/librte_ether/rte_eth_dev_info.h| 80 +
> > lib/librte_ether/rte_ethdev.c | 159
> +
> > lib/librte_ether/rte_ethdev.h | 158
>
> > lib/librte_ether/rte_ether_version.map | 8 ++
> > 5 files changed, 406 insertions(+)
> > create mode 100644 lib/librte_ether/rte_eth_dev_info.h
> >
> > diff --git a/lib/librte_ether/Makefile b/lib/librte_ether/Makefile
> > index c0e5768..05209e9 100644
> > --- a/lib/librte_ether/Makefile
> > +++ b/lib/librte_ether/Makefile
> > @@ -51,6 +51,7 @@ SRCS-y += rte_ethdev.c SYMLINK-y-include +=
> > rte_ether.h SYMLINK-y-include += rte_ethdev.h SYMLINK-y-include +=
> > rte_eth_ctrl.h
> > +SYMLINK-y-include += rte_eth_dev_info.h
> >
> > # this lib depends upon:
> > DEPDIRS-y += lib/librte_eal lib/librte_mempool lib/librte_ring
> > lib/librte_mbuf diff --git a/lib/librte_ether/rte_eth_dev_info.h
> > b/lib/librte_ether/rte_eth_dev_info.h
> > new file mode 100644
> > index 000..002c4b5
> > --- /dev/null
> > +++ b/lib/librte_ether/rte_eth_dev_info.h
> > @@ -0,0 +1,80 @@
> > +/*-
> > + * BSD LICENSE
> > + *
> > + * Copyright(c) 2015 Intel Corporation. All rights reserved.
> > + * All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + *
> > + * * Redistributions of source code must retain the above copyright
> > + * notice, this list of conditions and the following disclaimer.
> > + * * Redistributions in binary form must reproduce the above copyright
> > + * notice, this list of conditions and the following disclaimer in
> > + * the documentation and/or other materials provided with the
> > + * distribution.
> > + * * Neither the name of Intel Corporation nor the names of its
> > + * contributors may be used to endorse or promote products derived
> > + * from this software without specific prior written permission.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> BUT NOT
> > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> LOSS OF USE,
> > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> OF THE USE
> > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > + */
> > +
> > +#ifndef _RTE_ETH_DEV_INFO_H_
> > +#define _RTE_ETH_DEV_INFO_H_
> > +
> > +
> > +/*
> > + * Placeholder for accessing device registers */ struct
> > +rte_dev_reg_info {
> > + void *buf; /**< Buffer for register */
> > + uint32_t offset; /**< Offset for 1st
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Friday, June 05, 2015 9:41 AM > To: Wang, Liang-min > Cc: Andrew Harvey (agh); Stephen Hemminger; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide > ethtool-alike APIs > > 2015-06-05 11:25, Wang, Liang-min: > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > > > Stephen and me say the same thing about using the ethdev API. > > > We don't understand why using a fake ethtool lib would be easier. > > > Though you are saying it "facilitated [your] adoption to dpdk". > > > Please could you explain why using an ethtool-like API is easier > > > than using the existing ethdev API? > > > In any case, you have to develop a specific backend for DPDK > > > (rte_ethtool would be also DPDK-specific). > > > > As described earlier in this patch comment reply, there are other ethtool > ops that have been implemented. > > Those ops includes set/get eeprom, set/get pauseparam, set/get > ringparam which are not available in the exiting ethdev library. > > 1/ We cannot really consider code which is not public 2/ You may extend > ethdev if some functions are missing > > > For this release, we focus on releasing some basic functions (btw, > mac_addr_set is not available but is covered by this patch). > > Yes, you are extending ethdev by adding > rte_eth_dev_default_mac_addr_set. > > > The key reason that this set of library is not released as part of ethdev is > the ethtool API dependency on kernel include file. > > It is a good reason to separate the library. > But it doesn't justify its need. > > > To faithfully carry the ethtool ops and net dev ops API parameters, the > ethtool APIs are designed to follow the original definition except avoiding > carry kernel states. > > With that, to support ethtool APIs faithfully, we need to include > . > > As suggested by many DPDK veterans including Thomas (indicated over > your reply), you would prefer these APIs in a separate library. > > I think I'm starting to understand that you really need ethtool conversion > (implemented in rte_ethtool_get_drvinfo) but not the other functions which > are simple wrappers. Right? The rte_ethtool_get_drvinfo and many others ethtool ops have the same conversion requirement. As for ethtool and net dev ops that don't require conversion. For the sake of clean API interface, they are implemented in the same ethtool library.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
> -Original Message- > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] > Sent: Friday, June 05, 2015 6:47 AM > To: Andrew Harvey (agh) > Cc: Stephen Hemminger; Wang, Liang-min; dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide > ethtool-alike APIs > > 2015-06-04 22:10, Andrew Harvey: > > On 6/4/15, 7:58 AM, "Stephen Hemminger" > wrote: > > >"Andrew Harvey (agh)" wrote: > > >> I believe that their is value in this interface for software stacks > > >>not based on Linux being moved toward DPDK that need simple > > >>operations like getting the mac address. Some of these stacks have > > >>a dearth of resources available and dedicating a core/thread to KNI > > >>to get/set a mac address is considered excessive. There are also > > >>issues with 32/64 bit kernel integration using KNI. If the > > >>ethtool interface is not the correct interface then please help me > > >>understand what should/could have been used. If ethtool is > > >>considered 'old and clunky? Stephen's and your input would be > > >>valuable in designing another interface with similar properties. > > >>The use-case is pretty simple and there is no plans for moving > > >>anything back into the kernel on the contrary its the complete opposite. > > >> > > >> ? Andy > > > > > >We have DPDK API's to do this, and any added wrappers make it bigger. > > >I don't see why calling your ethtool API is better than calling > > >rte_eth* API. > > > > > >If there is a missing functionality in the rte_ethXXX api's for an > > >application then add that. For example: rte_eth_mac_addr_get() > > > > I am getting somewhat confused by your latest comments. Your first > > email (referenced below) looked really positive and I found your > > suggestions useful. Your latest post appears to contradict this and > > now the interface was there all the time. The wrapper fa?ade provided > > by the ethtool library provide a clean separation of concerns and will > > allow people to migrate from not only KNI but in our case from a > > legacy system. If a software stack has requirements to work with > > multiple IO abstractions then the ethtool approach is attractive. I > > would speculate that many other stacks moving towards dpdk will have > similar issues. > > > > Summarizing, for our use-cases the ethtool interface facilitated our > > adoption to dpdk while allowing us to support our legacy IO abstractions. > > Stephen and me say the same thing about using the ethdev API. > We don't understand why using a fake ethtool lib would be easier. > Though you are saying it "facilitated [your] adoption to dpdk". > Please could you explain why using an ethtool-like API is easier than using > the existing ethdev API? > In any case, you have to develop a specific backend for DPDK (rte_ethtool > would be also DPDK-specific). As described earlier in this patch comment reply, there are other ethtool ops that have been implemented. Those ops includes set/get eeprom, set/get pauseparam, set/get ringparam which are not available in the exiting ethdev library. For this release, we focus on releasing some basic functions (btw, mac_addr_set is not available but is covered by this patch). The key reason that this set of library is not released as part of ethdev is the ethtool API dependency on kernel include file. To faithfully carry the ethtool ops and net dev ops API parameters, the ethtool APIs are designed to follow the original definition except avoiding carry kernel states. With that, to support ethtool APIs faithfully, we need to include . As suggested by many DPDK veterans including Thomas (indicated over your reply), you would prefer these APIs in a separate library. > > It seems you already started to use such an ethtool implementation. > Please note that our goal is not to prevent Cisco from upstreaming (evidence > with enic driver integration) but we want to guide you, and others having the > same needs, to the best solution for everybody. > That's why we need to understand what we (or you) are missing. > Maybe that it would be clearer with some code examples (which would go in > the lib documentation if any). > > Thanks
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>I have the feeling we are not progressing in this discussion. >Please bring new explanations or I'll give up. >David Harton already acked it so maybe he could explain why it is useful. > >Comments below > >2015-06-02 17:06, Wang, Liang-min: >> >2015-06-02 15:47, Wang, Liang-min: >> >> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] >> >> > >>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. >> >> >> >> The benefit is single interface for users to access. Instead of looking >> >> into two different interfaces (ethtool, ether). >> > >> >Sorry it doesn't help to understand. >> >Today, there is an ethdev API. Why adding an ethtool-like API would help? >> >> Need to understand your specific concern. Do you oppose introducing new API >> to support ethtool ops and net_device_ops? > >They are not ethtool/netdev ops but fake ones. >In linux: > int dev_set_mtu(struct net_device *dev, int new_mtu) In dpdk: > int rte_ethtool_net_change_mtu(uint8_t port_id, int mtu); So yes, I'm > opposed to add an API which is neither ethdev, neither ethtool. >But I may be missing an obvious justification. So, the design purposely stays away from struct net_device to avoid carrying kernel states. We consciously choose port in place of net_device. The kni already provides ethtool for kernel space code, this interface is designed for user-space application (fast-path comparing to kni). >> Or your concern is on the implementation. If that's latter. >> Do you oppose adding a new library to implement ethtool ops and >> net_device_ops? > >Already answered above >> >> If so, are you satisfied with my previous answer on avoiding polluting >> ethdev APIs? >> If not, do you suggest integrating ethtool APIs into ethdev API? > >No, it's better as a separate library. > >> If not, is your concern on the implementation of common functionality >> between ethtool and ethdev APIs? >> If so, as explained, it is designed to provide a unified >> interface to assist users to migrate from kernel ethtool/net-device-op >> API to DPDK > >Do you mean it would help migrating some code from kernel space to dpdk? >How it would help since the functions and data are different from the kernel >ones? For application that was designed based upon kernel-space ethtool-op and net-device-op, the user-space APIs provide a path for quick integration. >> BTW, as my reply to Steve's comment, more ops are on their way. This patch >> is to open up the interface. > >Currently they are only some one-liners plus ethtool_drvinfo formatting so >there is no real benefit. >Why not wait to have more ops implemented? Due to amount of code change, I was advised to put into separate release and started with APIs that only requires minor changes on ethdev. The rest of API requires changes on NIC pmd driver to support ops that are not currently supported.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>2015-06-02 15:47, Wang, Liang-min: >> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com] >> > >>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. >> >> The benefit is single interface for users to access. Instead of looking into >> two different interfaces (ethtool, ether). > >Sorry it doesn't help to understand. >Today, there is an ethdev API. Why adding an ethtool-like API would help? Need to understand your specific concern. Do you oppose introducing new API to support ethtool ops and net_device_ops? Or your concern is on the implementation. If that's latter. Do you oppose adding a new library to implement ethtool ops and net_device_ops? If so, are you satisfied with my previous answer on avoiding polluting ethdev APIs? If not, do you suggest integrating ethtool APIs into ethdev API? If not, is your concern on the implementation of common functionality between ethtool and ethdev APIs? If so, as explained, it is designed to provide a unified interface to assist users to migrate from kernel ethtool/net-device-op API to DPDK BTW, as my reply to Steve's comment, more ops are on their way. This patch is to open up the interface.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
-Original Message- From: Thomas Monjalon [mailto:thomas.monja...@6wind.com] Sent: Tuesday, June 02, 2015 10:33 AM To: Wang, Liang-min Cc: dev at dpdk.org Subject: Re: [dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs >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. Not sure if the question is the usefulness of user-space ethtool (there was request for user-space ethtool @ dpdk.org last year, right, and Steve's comment ...) or the question on putting ethtool on separate library. For the latter concern, as described, the design is to avoid polluting ethdev library by this inevitable including >> >- 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. Sure, I would help doc team adding new section into programmer's guide and other if it's necessary. The first thing is to have this API approved for release first. > >>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 My bad, I will address this issue on next patch > >>- 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. The benefit is single interface for users to access. Instead of looking into two different interfaces (ethtool, ether).
[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address
>On Tue, 02 Jun 2015 14:23:22 +0200
>Thomas Monjalon wrote:
>> 2015-06-02 10:52, Ananyev, Konstantin:
> >> From: Wang, Liang-min
> > >> int
> > >>+rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct
> > >> +ether_addr *addr) {
> > >> +struct rte_eth_dev *dev;
> > >> +
> > >> +if (!rte_eth_dev_is_valid_port(port_id)) {
> > >> +PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > >> +return -ENODEV;
> > >> +}
> > >> +
> > >> +dev = _eth_devices[port_id];
> > >> +FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
> > >> +
> > >> +return (*dev->dev_ops->mac_addr_set)(dev, addr); }
> >>
> >> As I can see mac_addr_set() is implemented now only for virtio.
> >> Which means that for all Intel HW your new
> >> rte_eth_dev_default_mac_addr_set()
> >> would not work right now?
> >> Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
> >> If mac_addr_set() is implemented by dev, then use it, otherwise try
> >> to use addr_remove()/addr_add() (as your first version did)?
> >> Konstantin
>>
>> Not sure it is a good idea to use remove/add to set the default unicast mac
>> address.
>> It would be clearer to add comments to remove/add functions to specify
>> that they don't apply to the default adress but only to secondary
>> ones. Then use the same logic for both API and driver ops.
>> It is the responsibility of the driver implementation to use common
>> functions for default_set and remove/add functions.
>Only vmxnet3 and virtio need special treatment. virtio is already covered.
>Here is patch for vmxnet3. We have used this for several releases.
To be consistent with implementation of ethdev API, rte_eth_macaddr_get, should
mac_addr_set ops in both virtio and vmxnet3 update
dev->data->mac_addrs[0]. So, the mac_addr_set and mac_addr_get are consistent.
>From eef188102338c5687b9075d468eabbe87693b075 Mon Sep 17 00:00:00 2001
>From: Stephen Hemminger
>Date: Tue, 2 Jun 2015 07:49:53 -0700
>Subject: [PATCH] vmxnet3: support setting primary MAC address
>
>This allows setting primary MAC address on VMXNET3.
>
>Signed-off-by: Stephen Hemminger
>---
> drivers/net/vmxnet3/vmxnet3_ethdev.c | 45 +---
> 1 file changed, 31 insertions(+), 14 deletions(-)
>
>diff --git a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>index 1685ce4..6515f74 100644
>--- a/drivers/net/vmxnet3/vmxnet3_ethdev.c
>+++ b/drivers/net/vmxnet3/vmxnet3_ethdev.c
>@@ -85,6 +85,9 @@ static void vmxnet3_dev_stats_get(struct rte_eth_dev *dev,
> struct rte_eth_stats *stats);
> static void vmxnet3_dev_info_get(struct rte_eth_dev *dev,
> struct rte_eth_dev_info *dev_info);
>+static void vmxnet3_mac_addr_set(struct rte_eth_dev *dev,
>+ struct ether_addr *mac_addr);
>+
> #if PROCESS_SYS_EVENTS == 1
> static void vmxnet3_process_events(struct vmxnet3_hw *); #endif @@ -110,6
> +113,7 @@ static const struct eth_dev_ops vmxnet3_eth_dev_ops = {
> .allmulticast_disable = vmxnet3_dev_allmulticast_disable,
> .link_update = vmxnet3_dev_link_update,
> .stats_get= vmxnet3_dev_stats_get,
>+ .mac_addr_set = vmxnet3_mac_addr_set,
> .dev_infos_get= vmxnet3_dev_info_get,
> .rx_queue_setup = vmxnet3_dev_rx_queue_setup,
> .rx_queue_release = vmxnet3_dev_rx_queue_release,
>@@ -359,6 +363,23 @@ vmxnet3_dev_configure(struct rte_eth_dev *dev)
> return 0;
> }
>
>+static void
>+vmxnet3_write_mac(struct vmxnet3_hw *hw, const uint8_t *addr) {
>+ uint32_t val;
>+
>+ PMD_INIT_LOG(DEBUG,
>+ "Writing MAC Address : %02x:%02x:%02x:%02x:%02x:%02x",
>+ addr[0], addr[1], addr[2],
>+ addr[3], addr[4], addr[5]);
>+
>+ val = *(const uint32_t *)addr;
>+ VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACL, val);
>+
>+ val = (addr[5] << 8) | addr[4];
>+ VMXNET3_WRITE_BAR1_REG(hw, VMXNET3_REG_MACH, val); }
>+
> static int
> vmxnet3_setup_driver_shared(struct rte_eth_dev *dev) { @@ -366,8 +387,7 @@
> vmxnet3_setup_driver_shared(struct rte_eth_dev *dev)
> struct vmxnet3_hw *hw = dev->data->dev_private;
> Vmxnet3_DriverShared *shared = hw->shared;
> Vmxnet3_DSDevRead *devRead = >dev
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>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 ..." >- There is no update in the doc/ directory Need more guidance on that. >Other comments: >- the patches are not versioned There is version file. Not sure what do you mean "the patches are not versioned" >- 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.
[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address
>> -Original Message-
>> From: Wang, Liang-min
>> Sent: Friday, May 29, 2015 8:27 PM
>> To: dev at dpdk.org
>> Cc: Richardson, Bruce; Ananyev, Konstantin; dharton at cisco.com;
> >agh at cisco.com; Wang, Liang-min
>> Subject: [PATCH 1/2] ethdev: add api to set default mac address
> >
>> add a new api: rte_eth_dev_default_mac_addr_set to support changing
>> default mac address of a NIC
> >
> >Signed-off-by: Liang-Min Larry Wang
>> ---
> > lib/librte_ether/rte_ethdev.c | 16
> > lib/librte_ether/rte_ethdev.h | 14 ++
>> lib/librte_ether/rte_ether_version.map | 1 +
>> 3 files changed, 31 insertions(+)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c
>> b/lib/librte_ether/rte_ethdev.c index 024fe8b..96ee00e 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -2752,6 +2752,22 @@ rte_eth_dev_mac_addr_remove(uint8_t port_id,
>> struct ether_addr *addr) }
>>
>> int
>> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr
>> +*addr) {
>> +struct rte_eth_dev *dev;
>> +
>> +if (!rte_eth_dev_is_valid_port(port_id)) {
>> +PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +return -ENODEV;
>> +}
>> +
>> +dev = _eth_devices[port_id];
>> +FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_set, -ENOTSUP);
>> +
>> +return (*dev->dev_ops->mac_addr_set)(dev, addr); }
>As I can see mac_addr_set() is implemented now only for virtio.
>Which means that for all Intel HW your new rte_eth_dev_default_mac_addr_set()
>would not work right now?
>Probably rte_eth_dev_default_mac_addr_set() should combine both approaches:
>If mac_addr_set() is implemented by dev, then use it, otherwise try to use
>addr_remove()/addr_add() (as your first version did)?
>Konstantin
The implementation of mac_addr_set() through mac_addr_remove() and
mac_addr_add() is very creative, but it may not work for devices other than
igb/ixgbe. I will release a patch latter to add hooks on igb and ixgbe(). Just
want to keep this interface clean and not to be confined to igb/ixgbe
characteristics.
> +
> +int
> rte_eth_dev_set_vf_rxmode(uint8_t port_id, uint16_t vf,
> uint16_t rx_mode, uint8_t on)
> {
> diff --git a/lib/librte_ether/rte_ethdev.h
> b/lib/librte_ether/rte_ethdev.h index 16dbe00..5f07e0d 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2982,6 +2982,20 @@ int rte_eth_dev_mac_addr_add(uint8_t port,
> struct ether_addr *mac_addr, int rte_eth_dev_mac_addr_remove(uint8_t
> port, struct ether_addr *mac_addr);
>
> /**
> + * Set the default MAC address.
> + *
> + * @param port
> + * The port identifier of the Ethernet device.
> + * @param mac_addr
> + * New default MAC address.
> + * @return
> + * - (0) if successful, or *mac_addr* didn't exist.
> + * - (-ENOTSUP) if hardware doesn't support.
> + * - (-ENODEV) if *port* invalid.
> + */
> +int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr
> +*mac_addr);
> +
> +/**
> * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
> *
> * @param port
> diff --git a/lib/librte_ether/rte_ether_version.map
> b/lib/librte_ether/rte_ether_version.map
> index a2d25a6..2dbbaa7 100644
> --- a/lib/librte_ether/rte_ether_version.map
> +++ b/lib/librte_ether/rte_ether_version.map
> @@ -102,6 +102,7 @@ DPDK_2.0 {
> rte_eth_tx_queue_setup;
> rte_eth_xstats_get;
> rte_eth_xstats_reset;
> + rte_eth_dev_default_mac_addr_set;
>
> local: *;
> };
> --
> 2.1.4
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>>On Sat, 30 May 2015 19:40:46 +0000 >>"Wang, Liang-min" wrote: >> >>> > >>On Sat, 30 May 2015 16:16:01 + > >>"Wang, Liang-min" wrote: > >> > >> >>The design decision is to keep ethdev as THE interface for all the > >> >>external API, so ethtool APIs are designed based upon ethdev API. At the > >> >>meantime, the ethtool APIs are designed to enable users to migrate > >> >>designs based upon kernel-space ethtool. The open/close/start are put in > >> >>place to enable quick migration. > >>> > >>>But there is no open/close/start in ethtool in kernel. > >>>Anyway ethtool is currently on the disfavored list from kernel developers. > >>>What about netlink or something better? > >>> > >>>Remember each new API creates more long term compatiablity and ABI issues. > >>>So I am against introducing any new API that does the same thing as > >>>existing API's. > >> >>> Just to clarify APIs supported by this ethtool api: there are >>> net_open > >>and net_stop and no net_start. Both functions are put in place to > >>support net_device_ops::ndo_open and net_device_ops::ndo_close as > >>defined in linux/netdevice.h >> >> >>I get the feeling there is some use case you are not telling the list about. >>What kind of application would use this api only. Why or how would DPDK >>application be involved in net_device_ops. If you are planning on putting >>DPDK in the kernel there are lots of other issues >>including kernel ABI >>stability and licensing that need to be dealt with. > >(I'm manually adding ">" through my email, outlook, to make my reply. I >apology if I make any mistake on adding ">" in wrong place) No, we don't plan >to put DPDK into kernel space, and this patch has >nothing to do with >bifurcated driver that was announced for DPDK 2.0 then got scrubbed (or >deferred). In contrary, the entire ethtool API (more support is coming) is >designed to assist applications that >were designed based upon kernel ethtool >to migrate into user-space driver based DPDK libraries. Being said that, as >you are aware the kernel version of ndo_open/ndo_close is more than just start >and >stop device. The initial implementation is to provide minimum >functionality (strip off all the kernel related state management). In the >future release (we need comments like yours), we will continue >make >improvement. So this new API can be another alternative for applications to >run device management. Just to clarify my last reply: when I said "... based upon kernel ethtool to migrate ...", I was referring to both ethtool and net_device_op.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>On Sat, 30 May 2015 19:40:46 + >"Wang, Liang-min" wrote: > >> > >On Sat, 30 May 2015 16:16:01 + > >"Wang, Liang-min" wrote: > > > >> >The design decision is to keep ethdev as THE interface for all the > >> >external API, so ethtool APIs are designed based upon ethdev API. At the > >> >meantime, the ethtool APIs are designed to enable users to migrate > >> >designs based upon kernel-space ethtool. The open/close/start are put in > >> >place to enable quick migration. > >> > >>But there is no open/close/start in ethtool in kernel. > >>Anyway ethtool is currently on the disfavored list from kernel developers. > >>What about netlink or something better? > >> > >>Remember each new API creates more long term compatiablity and ABI issues. > >>So I am against introducing any new API that does the same thing as > >>existing API's. > > >> Just to clarify APIs supported by this ethtool api: there are net_open > >and net_stop and no net_start. Both functions are put in place to > >support net_device_ops::ndo_open and net_device_ops::ndo_close as > >defined in linux/netdevice.h > > >I get the feeling there is some use case you are not telling the list about. >What kind of application would use this api only. Why or how would DPDK >application be involved in net_device_ops. If you are planning on putting DPDK >in the kernel there are lots of other issues >including kernel ABI stability >and licensing that need to be dealt with. (I'm manually adding ">" through my email, outlook, to make my reply. I apology if I make any mistake on adding ">" in wrong place) No, we don't plan to put DPDK into kernel space, and this patch has nothing to do with bifurcated driver that was announced for DPDK 2.0 then got scrubbed (or deferred). In contrary, the entire ethtool API (more support is coming) is designed to assist applications that were designed based upon kernel ethtool to migrate into user-space driver based DPDK libraries. Being said that, as you are aware the kernel version of ndo_open/ndo_close is more than just start and stop device. The initial implementation is to provide minimum functionality (strip off all the kernel related state management). In the future release (we need comments like yours), we will continue make improvement. So this new API can be another alternative for applications to run device management.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
On Sat, 30 May 2015 16:16:01 + "Wang, Liang-min" wrote: > >The design decision is to keep ethdev as THE interface for all the external > >API, so ethtool APIs are designed based upon ethdev API. At the meantime, > >the ethtool APIs are designed to enable users to migrate designs based upon > >kernel-space ethtool. The open/close/start are put in place to enable quick > >migration. > >But there is no open/close/start in ethtool in kernel. >Anyway ethtool is currently on the disfavored list from kernel developers. >What about netlink or something better? > >Remember each new API creates more long term compatiablity and ABI issues. >So I am against introducing any new API that does the same thing as existing >API's. Just to clarify APIs supported by this ethtool api: there are net_open and net_stop and no net_start. Both functions are put in place to support net_device_ops::ndo_open and net_device_ops::ndo_close as defined in linux/netdevice.h
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>On Fri, 29 May 2015 20:37:56 -0400 >Liang-Min Larry Wang wrote: > >> 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). >The API's for ethtool like things are valuable, but please contain it more. > >I think you should contain this to only those things which are in the original >Linux ethtool. Adding yet another layer that calls DPDK for open/close/start >stop just adds more layers with out providing any value. > >The ones I would keep: > get_driverinfo > get_link > >The ones I would add: > cmd - for speed/duplex negotiation > eeprom - for eeprom access > eee - for energy awareness > coalesce - for packet coalescing > ringparam - to allow changing ring parameters > (existing rx config model is limiting) > pauseparam - existing flow control in DPDK is mess > gstrings/stats - map to XSTATS > test - for offline tests > dump - for register dump > features - control offload features > perm_addr - base mac address The design decision is to keep ethdev as THE interface for all the external API, so ethtool APIs are designed based upon ethdev API. At the meantime, the ethtool APIs are designed to enable users to migrate designs based upon kernel-space ethtool. The open/close/start are put in place to enable quick migration. As for the suggestion, eeprom/ringparam/pauseparam/register-dump are on their way for latter release. For other ops, I will take the feedback for future planning.
[dpdk-dev] [PATCH 1/2] ethdev: add api to set default mac address
>On Fri, 29 May 2015 09:15:08 -0400
>Liang-Min Larry Wang wrote:
>
>> }
> >
>> int
>> +rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr
>> +*addr) {
>> +struct rte_eth_dev *dev;
>> +const int index = 0;
>> +const uint32_t pool = 0;
>> +
>> +if (!rte_eth_dev_is_valid_port(port_id)) {
>> +PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +return -ENODEV;
>> +}
>> +
>> +dev = _eth_devices[port_id];
>> +FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_remove, -ENOTSUP);
>> +FUNC_PTR_OR_ERR_RET(*dev->dev_ops->mac_addr_add, -ENOTSUP);
>> +
>> +/* Update NIC default MAC address*/
>> +(*dev->dev_ops->mac_addr_remove)(dev, index);
>> +(*dev->dev_ops->mac_addr_add)(dev, addr, index, pool);
>> +
>> +/* Update default address in NIC data structure */
>> +ether_addr_copy(addr, >data->mac_addrs[index]);
>> +
>> +return 0;
>> +}
>> +
>
>No. this won't work. for some devices.
>
>Please use mac_addr_set hook added in recent DPDK
I tested over ixgbe and igb, and both work. As for your concern, it's legit. I
will take your suggestion and make modification.
[dpdk-dev] [PATCH 2/2] ethtool: add new library to provide ethtool-alike APIs
>On Fri, 29 May 2015 09:15:09 -0400 >Liang-Min Larry Wang wrote: > >> +drvinfo->n_stats = sizeof(struct rte_eth_stats) / sizeof(uint64_t); > >+drvinfo->testinfo_len = 0; > > >Providing a hook to access more functionality is good and compatiablity with >old API's is helpful. Too bad ethtool is kind of a clunky old inflexible, and >easily broken with changes. But >that is not your fault. > >Duplicating eth_stats in ethtool is not helpful. >This is actively discouraged for Linux device drivers. (I have a typo of David's email, and I corrected it in this email) This new library is designed to support ethtool alike interface, some of the implementation will be just a placeholder. The rte_ethtool_net_get_stats64 is designed as a place holder to support net_device_ops::ndo_get_stats64. For the legacy support, it is required to provide # of statistic parameters are supported, so caller can allocate right size of buffer to get statistic from device driver.
