[dpdk-dev] [PATCH v15 1/4] ethdev: add apis to support access device info

[Wang, Liang-min]

> -----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?