[dpdk-dev] [PATCH v6 2/2] examples/ethtool: use rte_eth_dev_get_reg_info for reg params
From: Zyta Szpak This change deals with hard-coded register width. The app was allocating too little space for 64-bit registers which resulted in memory corruption. This commit resolves this by getting the number of registers and size of register by rte_eth_dev_get_reg_info function called first time with data=NULL. Signed-off-by: Zyta Szpak --- examples/ethtool/lib/rte_ethtool.c | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c index 54391f2..a1f91d4 100644 --- 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(®_info, 0, sizeof(reg_info)); + rte_eth_dev_get_reg_info(port_id, ®_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(®_info, 0, sizeof(reg_info)); + + ret = rte_eth_dev_get_reg_info(port_id, ®_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 -- 2.7.4
[dpdk-dev] [PATCH v6 1/2] ethdev: remove get_reg_length callback
From: Zyta Szpak 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 --- drivers/net/cxgbe/cxgbe_ethdev.c | 14 ++ drivers/net/e1000/igb_ethdev.c | 14 -- drivers/net/i40e/i40e_ethdev.c | 15 ++- drivers/net/ixgbe/ixgbe_ethdev.c | 14 -- drivers/net/thunderx/nicvf_ethdev.c| 14 +- drivers/net/thunderx/nicvf_ethdev.h| 1 + lib/librte_ether/rte_dev_info.h| 1 + lib/librte_ether/rte_ethdev.c | 12 lib/librte_ether/rte_ethdev.h | 25 + lib/librte_ether/rte_ether_version.map | 1 - 10 files changed, 52 insertions(+), 59 deletions(-) diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c index 6c130ed..f822d8f 100644 --- 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 dea50f3..594655d 100644 --- 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, }; @@ -4947,6 +4945,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))) { @@ -4971,6 +4975,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 c07da1b..8e3e27d 100644 --- 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_g
[dpdk-dev] [PATCH v5 1/2] ethdev: remove get_reg_length callback
Very good point.. this one and the other :) THere comes fixed version. 2016-07-04 12:38 GMT+02:00 Remy Horton : > > +++ b/drivers/net/cxgbe/cxgbe_ethdev.c >> @@ -934,7 +934,15 @@ 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); >> + if (regs->data == NULL) { >> + regs->length = cxgbe_get_regs_len(eth_dev); >> + regs->width = sizeof(uint32_t); >> + regs->version = >> CHELSIO_CHIP_VERSION(adapter->params.chip) | >> + (CHELSIO_CHIP_RELEASE(adapter->params.chip) << >> 10) | >> + (1 << 16); >> + return 0; >> + } >> + >> regs->version = CHELSIO_CHIP_VERSION(adapter->params.chip) | >> (CHELSIO_CHIP_RELEASE(adapter->params.chip) << >> 10) | >> (1 << 16); >> > > Code duplication.. > > Rest looks ok and passed a quick compile test. Might need to keep an eye > out for other driver changes. > -- Zyta Szpak Software Development Engineer Semihalf sp. z o.o. ul. Krakusa 11 30-535 Krak?w
[dpdk-dev] [PATCH v5 2/2] examples/ethtool: use rte_eth_dev_get_reg_info for reg params
From: Zyta Szpak Version 4 of fixing the fixed register width assumption. The app was allocating too little space for 64-bit registers which resulted in memory corruption. This commit resolves this by getting the number of registers and size of register by rte_eth_dev_get_reg_info function called first time with data=NULL. Signed-off-by: Zyta Szpak --- examples/ethtool/lib/rte_ethtool.c | 19 +-- 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/examples/ethtool/lib/rte_ethtool.c b/examples/ethtool/lib/rte_ethtool.c index 54391f2..a1f91d4 100644 --- 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(®_info, 0, sizeof(reg_info)); + rte_eth_dev_get_reg_info(port_id, ®_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(®_info, 0, sizeof(reg_info)); + + ret = rte_eth_dev_get_reg_info(port_id, ®_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 -- 2.7.4
[dpdk-dev] [PATCH v5 1/2] ethdev: remove get_reg_length callback
From: Zyta Szpak 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 --- drivers/net/cxgbe/cxgbe_ethdev.c | 11 +-- drivers/net/e1000/igb_ethdev.c | 14 -- drivers/net/i40e/i40e_ethdev.c | 15 ++- drivers/net/ixgbe/ixgbe_ethdev.c | 14 -- drivers/net/thunderx/nicvf_ethdev.c| 14 +- drivers/net/thunderx/nicvf_ethdev.h| 1 + lib/librte_ether/rte_dev_info.h| 1 + lib/librte_ether/rte_ethdev.c | 12 lib/librte_ether/rte_ethdev.h | 25 + lib/librte_ether/rte_ether_version.map | 1 - 10 files changed, 51 insertions(+), 57 deletions(-) diff --git a/drivers/net/cxgbe/cxgbe_ethdev.c b/drivers/net/cxgbe/cxgbe_ethdev.c index 6c130ed..90098f9 100644 --- a/drivers/net/cxgbe/cxgbe_ethdev.c +++ b/drivers/net/cxgbe/cxgbe_ethdev.c @@ -934,7 +934,15 @@ 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); + if (regs->data == NULL) { + regs->length = cxgbe_get_regs_len(eth_dev); + regs->width = sizeof(uint32_t); + regs->version = CHELSIO_CHIP_VERSION(adapter->params.chip) | + (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) | + (1 << 16); + return 0; + } + regs->version = CHELSIO_CHIP_VERSION(adapter->params.chip) | (CHELSIO_CHIP_RELEASE(adapter->params.chip) << 10) | (1 << 16); @@ -971,7 +979,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 dea50f3..594655d 100644 --- 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, }; @@ -4947,6 +4945,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))) { @@ -4971,6 +4975,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 c07da1b..8e3e27d 100644 --- 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(struc
[dpdk-dev] [PATCH v4 1/2] ethdev: remove get_reg_length callback
OK On 27.06.2016 17:19, Thomas Monjalon wrote: > 2016-06-23 15:26, zr at semihalf.com: >> From: Zyta Szpak >> >> 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 > 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
[dpdk-dev] [PATCH v4 1/2] ethdev: remove get_reg_length callback
OK On 27.06.2016 12:46, Remy Horton wrote: > Morning, > > > On 23/06/2016 14:26, zr at semihalf.com wrote: >> From: Zyta Szpak >> >> 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
[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
Note that if we remove rte_eth_dev_get_reg_length() then it will break all of the drivers that implement it. Shall I remove it all leave it and modify only ethtool to use rte_eth_dev_get_regs() to get reg size? In the end the drivers will have to implement the part of setting the size in reg_info struct. rte_eth_dev_get_regs() itself wouldn't change at all. Or do you have different opinion? On 21.06.2016 11:55, Zyta Szpak wrote: > OK, I will do the v4. > > On 17.06.2016 12:20, Thomas Monjalon wrote: >> 2016-06-13 16:51, Remy Horton: >>> On 12/06/2016 15:51, Zyta Szpak wrote: >>>> I would prefer having only one function rte_eth_dev_get_regs() >>>> which returns length and width if data is NULL. >>>> The first call is a parameter request before buffer allocation, >>>> and the second call fills the buffer. >>>> >>>> We can deprecate the old API and introduce this new one. >>>> >>>> Opinions? >>>> >>>> In my opinion as it is now it works fine. Gathering all parameters in >>>> one callback might be a good idea if the maintainer also agrees to >>>> that >>>> because as I mentioned, it interferes. >>> From my perspective changing rte_eth_dev_get_regs() isn't a >>> problem, as >>> it isn't used directly rather than through rte_ethtool_get_regs().. >> Zyta, would you like to make a v4? >
[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
OK, I will do the v4. On 17.06.2016 12:20, Thomas Monjalon wrote: > 2016-06-13 16:51, Remy Horton: >> On 12/06/2016 15:51, Zyta Szpak wrote: >>> I would prefer having only one function rte_eth_dev_get_regs() >>> which returns length and width if data is NULL. >>> The first call is a parameter request before buffer allocation, >>> and the second call fills the buffer. >>> >>> We can deprecate the old API and introduce this new one. >>> >>> Opinions? >>> >>> In my opinion as it is now it works fine. Gathering all parameters in >>> one callback might be a good idea if the maintainer also agrees to that >>> because as I mentioned, it interferes. >> From my perspective changing rte_eth_dev_get_regs() isn't a problem, as >> it isn't used directly rather than through rte_ethtool_get_regs().. > Zyta, would you like to make a v4?
[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
Hi, please see inline 2016-06-08 10:53 GMT+02:00 Thomas Monjalon : > Hi Zyta, > > 2016-06-01 09:56, zr at semihalf.com: > > rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks > > do 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. > > Yes, register size is needed. > And I think it makes sense to register it in the struct rte_dev_reg_info. > We already have a length field, so we could just add a width field. > That was my first thought to add reg_size to reg_info struct but get_reg_length doesn' take reg_info as parameter so it would require modification of this callback as well. This would interfere with the author's vision. I think that adding a new one is clear and readable. > > > @@ -1455,6 +1458,8 @@ struct eth_dev_ops { > > > > eth_get_reg_length_t get_reg_length; > > /**< Get # of registers */ > > + eth_get_reg_width_t get_reg_width; > > + /**< Get # of bytes in register */ > > eth_get_reg_t get_reg; > > /**< Get registers */ > > I am not sure it is a good practice to add a new function for each > parameter of a request. > I would prefer having only one function rte_eth_dev_get_regs() > which returns length and width if data is NULL. > The first call is a parameter request before buffer allocation, > and the second call fills the buffer. > > We can deprecate the old API and introduce this new one. > > Opinions? > In my opinion as it is now it works fine. Gathering all parameters in one callback might be a good idea if the maintainer also agrees to that because as I mentioned, it interferes. Any other opinions?\ Best regards, Zyta Szpak
[dpdk-dev] [PATCH v2 1/2] ethdev: add callback to get register size in bytes
On 30.05.2016 12:58, Panu Matilainen wrote: > On 05/30/2016 12:39 PM, zr at semihalf.com wrote: >> From: Zyta Szpak >> >> Version 2 of fixing the fixed register width assumption. >> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks >> do 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. >> >> Signed-off-by: Zyta Szpak > [...] >> diff --git a/lib/librte_ether/rte_ether_version.map >> b/lib/librte_ether/rte_ether_version.map >> index 214ecc7..288bc63 100644 >> --- a/lib/librte_ether/rte_ether_version.map >> +++ b/lib/librte_ether/rte_ether_version.map >> @@ -130,5 +130,6 @@ DPDK_16.04 { >> rte_eth_tx_buffer_drop_callback; >> rte_eth_tx_buffer_init; >> rte_eth_tx_buffer_set_err_callback; >> +rte_eth_dev_get_reg_width; >> >> } DPDK_2.2; > > This symbol did not exist in DPDK 16.04 so it must not be added there. > Add a new section for 16.07 which inherits from DPDK_16.04. > > - Panu - > Right.
[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes
On 27.05.2016 12:28, Panu Matilainen wrote: > On 05/25/2016 09:36 AM, zr at semihalf.com wrote: >> From: Zyta Szpak >> >> Version 2 of fixing the fixed register width assumption. >> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks >> do 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. >> >> Signed-off-by: Zyta Szpak >> --- >> lib/librte_ether/rte_ethdev.c | 12 >> lib/librte_ether/rte_ethdev.h | 18 ++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/lib/librte_ether/rte_ethdev.c >> b/lib/librte_ether/rte_ethdev.c >> index a31018e..e0765f8 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -3231,6 +3231,18 @@ rte_eth_dev_get_reg_length(uint8_t port_id) >> } >> >> int >> +rte_eth_dev_get_reg_width(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_width, -ENOTSUP); >> +return (*dev->dev_ops->get_reg_width)(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 2757510..552eaed 100644 >> --- a/lib/librte_ether/rte_ethdev.h >> +++ b/lib/librte_ether/rte_ethdev.h >> @@ -1292,6 +1292,9 @@ typedef int (*eth_timesync_write_time)(struct >> rte_eth_dev *dev, >> typedef int (*eth_get_reg_length_t)(struct rte_eth_dev *dev); >> /**< @internal Retrieve device register count */ >> >> +typedef int (*eth_get_reg_width_t)(struct rte_eth_dev *dev); >> +/**< @internal Retrieve device register byte number */ >> + >> typedef int (*eth_get_reg_t)(struct rte_eth_dev *dev, >> struct rte_dev_reg_info *info); >> /**< @internal Retrieve registers */ >> @@ -1455,6 +1458,8 @@ struct eth_dev_ops { >> >> eth_get_reg_length_t get_reg_length; >> /**< Get # of registers */ >> +eth_get_reg_width_t get_reg_width; >> +/**< Get # of bytes in register */ >> eth_get_reg_t get_reg; >> /**< Get registers */ >> eth_get_eeprom_length_t get_eeprom_length; > > This is an ABI break, but maybe it is part of that "driver > implementation API" which is exempt from the ABI/API policies. Thomas? > >> @@ -3971,6 +3976,19 @@ int rte_eth_tx_queue_info_get(uint8_t port_id, >> uint16_t queue_id, >> */ >> int rte_eth_dev_get_reg_length(uint8_t port_id); >> >> +/* >> + * Retrieve the number of bytes in register for a specific device >> + * >> + * @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_width(uint8_t port_id); >> + >> /** >> * Retrieve device registers and register attributes >> * > > The function needs to be exported via rte_ether_version.map as well. OK, right! > > - Panu - >> >
[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes
Hi, It is the standard DPDK return value -ENOTSUP when the function is not supported by Ethernet device. I think it is safer to keep it this way rather than default implicitly to sizeof(uint32_t) and more generic. Regards, Zyta On 25.05.2016 15:14, Remy Horton wrote: > 'noon, > > Was expecting rte_eth_dev_get_reg_width() itself to default to > sizeof(uint32_t) rather than -ENOTSUP, but that is purely personal > taste which others might disagree with. You'll also need a > documentation update & Fixes: line. > > > On 25/05/2016 07:36, zr at semihalf.com wrote: >> From: Zyta Szpak > [..] >> Signed-off-by: Zyta Szpak > > Acked-by: Remy Horton
[dpdk-dev] [PATCH] examples/ethtool: include case for 64-bit registers
Hi, sorry on my late reply I was on sick leave. Sure I can do that. This fix was the fastest possible without interfering with DPDK API. I will add the callback then. Regards, Zyta Szpak On 20.05.2016 10:25, Remy Horton wrote: > Morning, > > On 11/05/2016 11:48, zr at semihalf.com wrote: >> From: Zyta Szpak >> >> rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks >> do not provide register size to the app in any way. Example assuming >> they are 32-bit wide always allocates not enough memory if the >> registers are 64-bit wide. It results in memory corruption. >> This commit is a quick fix to make enough room for 64-bit >> register values when this returned value is given to malloc. > [..] > > This is a loose end that needs to be fixed but my feeling is that it > ought to be done via querying the driver rather than overstating > register bank size. My suggestion would be to add something like > get_reg_wordsize to struct eth_dev_ops and then to use sizeof(uint32) > as fallback for drivers that don't implement the callback. > > Regards, > > ..R?my