[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Zyta Szpak]

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 = _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

[Zyta Szpak]

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 1/2] ethdev: add callback to get register size in bytes

[Thomas Monjalon]

2016-05-27 13:28, Panu Matilainen:
> On 05/25/2016 09:36 AM, zr at semihalf.com wrote:
> > @@ -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?

Yes dev_ops are for drivers, not for applications.
Thus it should not be impacted by the ABI policy.

[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Panu Matilainen]

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 = _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.

- Panu -
>

[dpdk-dev] [PATCH 1/2] ethdev: add callback to get register size in bytes

[Remy Horton]

'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 1/2] ethdev: add callback to get register size in bytes

[z...@semihalf.com]

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 = _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
  *
-- 
1.9.1