ethdev: get registers width

The ethtool app was allocating too little space for 64-bit
registers which resulted in memory corruption.

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 while 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.

Signed-off-by: Zyta Szpak <zyta.szpak@semihalf.com>
Acked-by: Remy Horton <remy.horton@intel.com>

diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c
index 0c12bec..9208a61 100644 (file)
--- a/drivers/net/cxgbe/cxgbe_ethdev.c
+++ b/drivers/net/cxgbe/cxgbe_ethdev.c
@@ -934,10 +934,17 @@ static int cxgbe_get_regs(struct rte_eth_dev *eth_dev,
        struct port_info *pi = (struct port_info *)(eth_dev->data->dev_private);
        struct adapter *adapter = pi->adapter;
 
-       regs->length = cxgbe_get_regs_len(eth_dev);
        regs->version = CHELSIO_CHIP_VERSION(adapter->params.chip) |
-                       (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) |
-                       (1 << 16);
+               (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) |
+               (1 << 16);
+
+       if (regs->data == NULL) {
+               regs->length = cxgbe_get_regs_len(eth_dev);
+               regs->width = sizeof(uint32_t);
+
+               return 0;
+       }
+
        t4_get_regs(adapter, regs->data, (regs->length * sizeof(uint32_t)));
 
        return 0;
@@ -971,7 +978,6 @@ static const struct eth_dev_ops cxgbe_eth_dev_ops = {
        .get_eeprom_length      = cxgbe_get_eeprom_length,
        .get_eeprom             = cxgbe_get_eeprom,
        .set_eeprom             = cxgbe_set_eeprom,
-       .get_reg_length         = cxgbe_get_regs_len,
        .get_reg                = cxgbe_get_regs,
 };
 

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 76d9f53..fbf4d09 100644 (file)
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -386,7 +386,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,
@@ -426,7 +425,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,
 };
 
@@ -4945,6 +4943,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))) {
@@ -4969,6 +4973,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 47b9b6b..06bc0b6 100644 (file)
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -440,8 +440,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);
 
@@ -524,7 +522,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,
@@ -9343,12 +9340,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)
 {
@@ -9357,6 +9348,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 3daa070..d478a15 100644 (file)
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -538,7 +538,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,
@@ -589,7 +588,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,
@@ -6316,6 +6314,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))) {
@@ -6340,6 +6344,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/drivers/net/thunderx/nicvf_ethdev.c b/drivers/net/thunderx/nicvf_ethdev.c
index 6e58d82..68a72e4 100644 (file)
--- a/drivers/net/thunderx/nicvf_ethdev.c
+++ b/drivers/net/thunderx/nicvf_ethdev.c
@@ -188,20 +188,17 @@ nicvf_dev_set_mtu(struct rte_eth_dev *dev, uint16_t mtu)
        return 0;
 }
 
-static int
-nicvf_dev_get_reg_length(struct rte_eth_dev *dev  __rte_unused)
-{
-       return nicvf_reg_get_count();
-}
-
 static int
 nicvf_dev_get_regs(struct rte_eth_dev *dev, struct rte_dev_reg_info *regs)
 {
        uint64_t *data = regs->data;
        struct nicvf *nic = nicvf_pmd_priv(dev);
 
-       if (data == NULL)
-               return -EINVAL;
+       if (data == NULL) {
+               regs->length = nicvf_reg_get_count();
+               regs->width = THUNDERX_REG_BYTES;
+               return 0;
+       }
 
        /* Support only full register dump */
        if ((regs->length == 0) ||
@@ -1623,7 +1620,6 @@ static const struct eth_dev_ops nicvf_eth_dev_ops = {
        .rx_queue_count           = nicvf_dev_rx_queue_count,
        .tx_queue_setup           = nicvf_dev_tx_queue_setup,
        .tx_queue_release         = nicvf_dev_tx_queue_release,
-       .get_reg_length           = nicvf_dev_get_reg_length,
        .get_reg                  = nicvf_dev_get_regs,
 };
 

diff --git a/drivers/net/thunderx/nicvf_ethdev.h b/drivers/net/thunderx/nicvf_ethdev.h
index 59fa19c..34447e0 100644 (file)
--- a/drivers/net/thunderx/nicvf_ethdev.h
+++ b/drivers/net/thunderx/nicvf_ethdev.h
@@ -36,6 +36,7 @@
 #include <rte_ethdev.h>
 
 #define THUNDERX_NICVF_PMD_VERSION      "1.0"
+#define THUNDERX_REG_BYTES             8
 
 #define NICVF_INTR_POLL_INTERVAL_MS    50
 #define NICVF_HALF_DUPLEX              0x00

diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c
index 54391f2..a1f91d4 100644 (file)
--- a/examples/ethtool/lib/rte_ethtool.c
+++ b/examples/ethtool/lib/rte_ethtool.c
@@ -46,6 +46,7 @@ int
 rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
 {
        struct rte_eth_dev_info dev_info;
+       struct rte_dev_reg_info reg_info;
        int n;
 
        if (drvinfo == NULL)
@@ -65,7 +66,9 @@ rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
                dev_info.pci_dev->addr.domain, dev_info.pci_dev->addr.bus,
                dev_info.pci_dev->addr.devid, dev_info.pci_dev->addr.function);
 
-       n = rte_eth_dev_get_reg_length(port_id);
+       memset(&reg_info, 0, sizeof(reg_info));
+       rte_eth_dev_get_reg_info(port_id, &reg_info);
+       n = reg_info.length;
        if (n > 0)
                drvinfo->regdump_len = n;
        else
@@ -86,12 +89,16 @@ rte_ethtool_get_drvinfo(uint8_t port_id, struct ethtool_drvinfo *drvinfo)
 int
 rte_ethtool_get_regs_len(uint8_t port_id)
 {
-       int count_regs;
+       struct rte_dev_reg_info reg_info;
+       int ret;
+
+       memset(&reg_info, 0, sizeof(reg_info));
+
+       ret = rte_eth_dev_get_reg_info(port_id, &reg_info);
+       if (ret)
+               return ret;
 
-       count_regs = rte_eth_dev_get_reg_length(port_id);
-       if (count_regs > 0)
-               return count_regs * sizeof(uint32_t);
-       return count_regs;
+       return reg_info.length * reg_info.width;
 }
 
 int

diff --git a/lib/librte_ether/rte_dev_info.h b/lib/librte_ether/rte_dev_info.h
index 291bd4d..574683d 100644 (file)
--- 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 eac260f..3cc667a 100644 (file)
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -3304,18 +3304,6 @@ rte_eth_timesync_write_time(uint8_t port_id, const struct timespec *timestamp)
        return (*dev->dev_ops->timesync_write_time)(dev, 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)
 {

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0f17323..4dac364 100644 (file)
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1321,9 +1321,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  */
@@ -1487,8 +1484,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;
@@ -4061,25 +4056,15 @@ 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 (number of registers and
+ * register size)
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param info
- *   The template includes buffer for register data and attribute to be filled.
+ *   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.
  * @return
  *   - (0) if successful.
  *   - (-ENOTSUP) if hardware doesn't support.

diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index e1ccebe..45ddf44 100644 (file)
--- 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;