[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
2016-06-22 10:19, Zyta Szpak: > 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? igb, ixgbe and i40e must be updated in the same patch to comply with the new behaviour of rte_eth_dev_get_regs. rte_eth_dev_get_reg_length can be deprecated and removed in the next release. > 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
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
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
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().. ..Remy
[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 v3 1/2] ethdev: add callback to get register size in bytes
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.
> @@ -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?
[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
On 01/06/2016 08:56, 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 Acked-by: Remy Horton
[dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes
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 ++
lib/librte_ether/rte_ether_version.map | 7 +++
3 files changed, 37 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;
@@ -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
*
diff --git a/lib/librte_ether/rte_ether_version.map
b/lib/librte_ether/rte_ether_version.map
index 214ecc7..568509c 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -132,3 +132,10 @@ DPDK_16.04 {
rte_eth_tx_buffer_set_err_callback;
} DPDK_2.2;
+
+DPDK_16.07 {
+ global:
+
+ rte_eth_dev_get_reg_width;
+
+} DPDK_16.04;
\ No newline at end of file
--
1.9.1
