[dpdk-dev,v4,1/2] ethdev: remove get_reg_length callback

Message ID 1466688410-13826-1-git-send-email-zr@semihalf.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Zyta Szpak June 23, 2016, 1:26 p.m. UTC
  From: Zyta Szpak <zr@semihalf.com>

Version 4 of fixing the assumption of that device registers
are always 32 bits long. rte_eth_dev_get_reg_length and
rte_eth_dev_get_reg_info callbacks did not provide register size
to the app in any way. It is needed to allocate proper number
of bytes before retrieving registers content with
rte_eth_dev_get_reg. This commit remove rte_eth_dev_get_reg_length
callback and adds width parameter to reg_info struct which makes
it possible to call rte_eth_dev_get_reg_info to get attributes
first. The drivers using this callback fill width and length
when call to function made with data=NULL.

Signed-off-by: Zyta Szpak <zr@semihalf.com>
---
 drivers/net/e1000/igb_ethdev.c         | 14 ++++++++++++--
 drivers/net/i40e/i40e_ethdev.c         | 15 ++++++---------
 drivers/net/ixgbe/ixgbe_ethdev.c       | 14 ++++++++++++--
 lib/librte_ether/rte_dev_info.h        |  1 +
 lib/librte_ether/rte_ethdev.c          | 12 ------------
 lib/librte_ether/rte_ethdev.h          | 20 +-------------------
 lib/librte_ether/rte_ether_version.map |  1 -
 7 files changed, 32 insertions(+), 45 deletions(-)
  

Comments

Remy Horton June 27, 2016, 10:46 a.m. UTC | #1
Morning,


On 23/06/2016 14:26, zr@semihalf.com wrote:
> From: Zyta Szpak <zr@semihalf.com>
>
> Version 4 of fixing the assumption of that device registers
> are always 32 bits long. rte_eth_dev_get_reg_length and
> rte_eth_dev_get_reg_info callbacks did not provide register size
> to the app in any way. It is needed to allocate proper number
> of bytes before retrieving registers content with
> rte_eth_dev_get_reg. This commit remove rte_eth_dev_get_reg_length
> callback and adds width parameter to reg_info struct which makes
> it possible to call rte_eth_dev_get_reg_info to get attributes
> first. The drivers using this callback fill width and length
> when call to function made with data=NULL.

I think this would read better as a commit message:

Removes hard-coded assumption that device registers are always 32 bits 
wide. The rte_eth_dev_get_reg_length and rte_eth_dev_get_reg_info 
callbacks did not provide register size to the app in any way, which is 
needed to allocate correct number of bytes before retrieving registers 
using rte_eth_dev_get_reg.

This commit changes rte_eth_dev_get_reg_info so that it can be used to 
retrieve both the number of registers and their width, and removes the 
now-redundant rte_eth_dev_get_reg_length.


> -/**
> - * Retrieve device registers and register attributes
> + * Retrieve device registers and register attributes (nb of regs and reg size)
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.

Need detail regarding how *info->data affects function behaviour. 
Something along the lines of:

/**
  * Retrieve device registers and register attributes (number of
  * registers and register size)
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param info
  *   Pointer to rte_dev_reg_info structure to fill in. If info->data is
  *   NULL the function fills in the width and length fields. If non-NULL
  *   the registers are put into the buffer pointed at by the data field.

Program code itself looks good to me.

..Remy
  
Thomas Monjalon June 27, 2016, 3:19 p.m. UTC | #2
2016-06-23 15:26, zr@semihalf.com:
> From: Zyta Szpak <zr@semihalf.com>
> 
> Version 4 of fixing the assumption of that device registers
> are always 32 bits long. rte_eth_dev_get_reg_length and
> rte_eth_dev_get_reg_info callbacks did not provide register size
> to the app in any way. It is needed to allocate proper number
> of bytes before retrieving registers content with
> rte_eth_dev_get_reg. This commit remove rte_eth_dev_get_reg_length
> callback and adds width parameter to reg_info struct which makes
> it possible to call rte_eth_dev_get_reg_info to get attributes
> first. The drivers using this callback fill width and length
> when call to function made with data=NULL.
> 
> Signed-off-by: Zyta Szpak <zr@semihalf.com>

Please do not mention patch revision in the commit log.
However you can add a changelog below the three dashes (with --annotate).
And please use --in-reply-to to keep revisions in the same thread.
Thanks
  
Zyta Szpak June 28, 2016, 4:05 p.m. UTC | #3
OK

On 27.06.2016 12:46, Remy Horton wrote:
> Morning,
>
>
> On 23/06/2016 14:26, zr@semihalf.com wrote:
>> From: Zyta Szpak <zr@semihalf.com>
>>
>> Version 4 of fixing the assumption of that device registers
>> are always 32 bits long. rte_eth_dev_get_reg_length and
>> rte_eth_dev_get_reg_info callbacks did not provide register size
>> to the app in any way. It is needed to allocate proper number
>> of bytes before retrieving registers content with
>> rte_eth_dev_get_reg. This commit remove rte_eth_dev_get_reg_length
>> callback and adds width parameter to reg_info struct which makes
>> it possible to call rte_eth_dev_get_reg_info to get attributes
>> first. The drivers using this callback fill width and length
>> when call to function made with data=NULL.
>
> I think this would read better as a commit message:
>
> Removes hard-coded assumption that device registers are always 32 bits 
> wide. The rte_eth_dev_get_reg_length and rte_eth_dev_get_reg_info 
> callbacks did not provide register size to the app in any way, which 
> is needed to allocate correct number of bytes before retrieving 
> registers using rte_eth_dev_get_reg.
>
> This commit changes rte_eth_dev_get_reg_info so that it can be used to 
> retrieve both the number of registers and their width, and removes the 
> now-redundant rte_eth_dev_get_reg_length.
>
>
>> -/**
>> - * Retrieve device registers and register attributes
>> + * Retrieve device registers and register attributes (nb of regs and 
>> reg size)
>>   *
>>   * @param port_id
>>   *   The port identifier of the Ethernet device.
>
> Need detail regarding how *info->data affects function behaviour. 
> Something along the lines of:
>
> /**
>  * Retrieve device registers and register attributes (number of
>  * registers and register size)
>  *
>  * @param port_id
>  *   The port identifier of the Ethernet device.
>  * @param info
>  *   Pointer to rte_dev_reg_info structure to fill in. If info->data is
>  *   NULL the function fills in the width and length fields. If non-NULL
>  *   the registers are put into the buffer pointed at by the data field.
>
> Program code itself looks good to me.
>
> ..Remy
  
Zyta Szpak June 28, 2016, 4:05 p.m. UTC | #4
OK

On 27.06.2016 17:19, Thomas Monjalon wrote:
> 2016-06-23 15:26, zr@semihalf.com:
>> From: Zyta Szpak <zr@semihalf.com>
>>
>> Version 4 of fixing the assumption of that device registers
>> are always 32 bits long. rte_eth_dev_get_reg_length and
>> rte_eth_dev_get_reg_info callbacks did not provide register size
>> to the app in any way. It is needed to allocate proper number
>> of bytes before retrieving registers content with
>> rte_eth_dev_get_reg. This commit remove rte_eth_dev_get_reg_length
>> callback and adds width parameter to reg_info struct which makes
>> it possible to call rte_eth_dev_get_reg_info to get attributes
>> first. The drivers using this callback fill width and length
>> when call to function made with data=NULL.
>>
>> Signed-off-by: Zyta Szpak <zr@semihalf.com>
> Please do not mention patch revision in the commit log.
> However you can add a changelog below the three dashes (with --annotate).
> And please use --in-reply-to to keep revisions in the same thread.
> Thanks
  

Patch

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index b822992..312a2f4 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -366,7 +366,6 @@  static const struct eth_dev_ops eth_igb_ops = {
 	.timesync_disable     = igb_timesync_disable,
 	.timesync_read_rx_timestamp = igb_timesync_read_rx_timestamp,
 	.timesync_read_tx_timestamp = igb_timesync_read_tx_timestamp,
-	.get_reg_length       = eth_igb_get_reg_length,
 	.get_reg              = eth_igb_get_regs,
 	.get_eeprom_length    = eth_igb_get_eeprom_length,
 	.get_eeprom           = eth_igb_get_eeprom,
@@ -406,7 +405,6 @@  static const struct eth_dev_ops igbvf_eth_dev_ops = {
 	.rxq_info_get         = igb_rxq_info_get,
 	.txq_info_get         = igb_txq_info_get,
 	.mac_addr_set         = igbvf_default_mac_addr_set,
-	.get_reg_length       = igbvf_get_reg_length,
 	.get_reg              = igbvf_get_regs,
 };
 
@@ -4750,6 +4748,12 @@  eth_igb_get_regs(struct rte_eth_dev *dev,
 	int count = 0;
 	const struct reg_info *reg_group;
 
+	if (data == NULL) {
+		regs->length = eth_igb_get_reg_length(dev);
+		regs->width = sizeof(uint32_t);
+		return 0;
+	}
+
 	/* Support only full register dump */
 	if ((regs->length == 0) ||
 	    (regs->length == (uint32_t)eth_igb_get_reg_length(dev))) {
@@ -4774,6 +4778,12 @@  igbvf_get_regs(struct rte_eth_dev *dev,
 	int count = 0;
 	const struct reg_info *reg_group;
 
+	if (data == NULL) {
+		regs->length = igbvf_get_reg_length(dev);
+		regs->width = sizeof(uint32_t);
+		return 0;
+	}
+
 	/* Support only full register dump */
 	if ((regs->length == 0) ||
 	    (regs->length == (uint32_t)igbvf_get_reg_length(dev))) {
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index f94ad87..9173e24 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -437,8 +437,6 @@  static int i40e_dev_rx_queue_intr_enable(struct rte_eth_dev *dev,
 static int i40e_dev_rx_queue_intr_disable(struct rte_eth_dev *dev,
 					  uint16_t queue_id);
 
-static int i40e_get_reg_length(struct rte_eth_dev *dev);
-
 static int i40e_get_regs(struct rte_eth_dev *dev,
 			 struct rte_dev_reg_info *regs);
 
@@ -519,7 +517,6 @@  static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.timesync_adjust_time         = i40e_timesync_adjust_time,
 	.timesync_read_time           = i40e_timesync_read_time,
 	.timesync_write_time          = i40e_timesync_write_time,
-	.get_reg_length	              = i40e_get_reg_length,
 	.get_reg                      = i40e_get_regs,
 	.get_eeprom_length            = i40e_get_eeprom_length,
 	.get_eeprom                   = i40e_get_eeprom,
@@ -9059,12 +9056,6 @@  i40e_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
 	return 0;
 }
 
-static int i40e_get_reg_length(__rte_unused struct rte_eth_dev *dev)
-{
-	/* Highest base addr + 32-bit word */
-	return I40E_GLGEN_STAT_CLEAR + 4;
-}
-
 static int i40e_get_regs(struct rte_eth_dev *dev,
 			 struct rte_dev_reg_info *regs)
 {
@@ -9073,6 +9064,12 @@  static int i40e_get_regs(struct rte_eth_dev *dev,
 	uint32_t reg_idx, arr_idx, arr_idx2, reg_offset;
 	const struct i40e_reg_info *reg_info;
 
+	if (ptr_data == NULL) {
+		regs->length = I40E_GLGEN_STAT_CLEAR + 4;
+		regs->width = sizeof(uint32_t);
+		return 0;
+	}
+
 	/* The first few registers have to be read using AQ operations */
 	reg_idx = 0;
 	while (i40e_regs_adminq[reg_idx].name) {
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index e11a431..a946509 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -532,7 +532,6 @@  static const struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.timesync_disable     = ixgbe_timesync_disable,
 	.timesync_read_rx_timestamp = ixgbe_timesync_read_rx_timestamp,
 	.timesync_read_tx_timestamp = ixgbe_timesync_read_tx_timestamp,
-	.get_reg_length       = ixgbe_get_reg_length,
 	.get_reg              = ixgbe_get_regs,
 	.get_eeprom_length    = ixgbe_get_eeprom_length,
 	.get_eeprom           = ixgbe_get_eeprom,
@@ -583,7 +582,6 @@  static const struct eth_dev_ops ixgbevf_eth_dev_ops = {
 	.rxq_info_get         = ixgbe_rxq_info_get,
 	.txq_info_get         = ixgbe_txq_info_get,
 	.mac_addr_set         = ixgbevf_set_default_mac_addr,
-	.get_reg_length       = ixgbevf_get_reg_length,
 	.get_reg              = ixgbevf_get_regs,
 	.reta_update          = ixgbe_dev_rss_reta_update,
 	.reta_query           = ixgbe_dev_rss_reta_query,
@@ -6268,6 +6266,12 @@  ixgbe_get_regs(struct rte_eth_dev *dev,
 	const struct reg_info **reg_set = (hw->mac.type == ixgbe_mac_82598EB) ?
 				    ixgbe_regs_mac_82598EB : ixgbe_regs_others;
 
+	if (data == NULL) {
+		regs->length = ixgbe_get_reg_length(dev);
+		regs->width = sizeof(uint32_t);
+		return 0;
+	}
+
 	/* Support only full register dump */
 	if ((regs->length == 0) ||
 	    (regs->length == (uint32_t)ixgbe_get_reg_length(dev))) {
@@ -6292,6 +6296,12 @@  ixgbevf_get_regs(struct rte_eth_dev *dev,
 	int count = 0;
 	const struct reg_info *reg_group;
 
+	if (data == NULL) {
+		regs->length = ixgbevf_get_reg_length(dev);
+		regs->width = sizeof(uint32_t);
+		return 0;
+	}
+
 	/* Support only full register dump */
 	if ((regs->length == 0) ||
 	    (regs->length == (uint32_t)ixgbevf_get_reg_length(dev))) {
diff --git a/lib/librte_ether/rte_dev_info.h b/lib/librte_ether/rte_dev_info.h
index 291bd4d..574683d 100644
--- a/lib/librte_ether/rte_dev_info.h
+++ b/lib/librte_ether/rte_dev_info.h
@@ -41,6 +41,7 @@  struct rte_dev_reg_info {
 	void *data; /**< Buffer for return registers */
 	uint32_t offset; /**< Start register table location for access */
 	uint32_t length; /**< Number of registers to fetch */
+	uint32_t width; /**< Size of device register */
 	uint32_t version; /**< Device version */
 };
 
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 42aaef7..2d73758 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3308,18 +3308,6 @@  rte_eth_timesync_write_time(uint8_t port_id, const struct timespec *timestamp)
 }
 
 int
-rte_eth_dev_get_reg_length(uint8_t port_id)
-{
-	struct rte_eth_dev *dev;
-
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-
-	dev = &rte_eth_devices[port_id];
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->get_reg_length, -ENOTSUP);
-	return (*dev->dev_ops->get_reg_length)(dev);
-}
-
-int
 rte_eth_dev_get_reg_info(uint8_t port_id, struct rte_dev_reg_info *info)
 {
 	struct rte_eth_dev *dev;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 45482f1..d4848a0 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1322,9 +1322,6 @@  typedef int (*eth_timesync_write_time)(struct rte_eth_dev *dev,
 				       const struct timespec *timestamp);
 /**< @internal Function used to get time from the device clock */
 
-typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev);
-/**< @internal Retrieve device register count  */
-
 typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev,
 				struct rte_dev_reg_info *info);
 /**< @internal Retrieve registers  */
@@ -1488,8 +1485,6 @@  struct eth_dev_ops {
 	/** Query redirection table. */
 	reta_query_t reta_query;
 
-	eth_get_reg_length_t get_reg_length;
-	/**< Get # of registers */
 	eth_get_reg_t get_reg;
 	/**< Get registers */
 	eth_get_eeprom_length_t get_eeprom_length;
@@ -4046,20 +4041,7 @@  int rte_eth_tx_queue_info_get(uint8_t port_id, uint16_t queue_id,
 	struct rte_eth_txq_info *qinfo);
 
 /**
- * Retrieve number of available registers for access
- *
- * @param port_id
- *   The port identifier of the Ethernet device.
- * @return
- *   - (>=0) number of registers if successful.
- *   - (-ENOTSUP) if hardware doesn't support.
- *   - (-ENODEV) if *port_id* invalid.
- *   - others depends on the specific operations implementation.
- */
-int rte_eth_dev_get_reg_length(uint8_t port_id);
-
-/**
- * Retrieve device registers and register attributes
+ * Retrieve device registers and register attributes (nb of regs and reg size)
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 97ed0b0..bbc50d1 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -36,7 +36,6 @@  DPDK_2.2 {
 	rte_eth_dev_get_eeprom_length;
 	rte_eth_dev_get_mtu;
 	rte_eth_dev_get_reg_info;
-	rte_eth_dev_get_reg_length;
 	rte_eth_dev_get_vlan_offload;
 	rte_eth_devices;
 	rte_eth_dev_info_get;