[dpdk-dev,v4,1/3] ethdev: refine API to query supported packet types

Message ID 1458866867-39582-2-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Commit Message

Jianfeng Tan March 25, 2016, 12:47 a.m. UTC
  Return 0 instead of -ENOTSUP for those which do not fill any packet types,
with some note and doc updated.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 doc/guides/nics/overview.rst  | 2 +-
 lib/librte_ether/rte_ethdev.c | 3 +--
 lib/librte_ether/rte_ethdev.h | 9 ++++++---
 3 files changed, 8 insertions(+), 6 deletions(-)
  

Comments

Jianfeng Tan March 25, 2016, 3:15 a.m. UTC | #1
patch 0: return 0 instead of -ENOTSUP.
patch 1: update doc/guides/nics/overview.rst.

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Jianfeng Tan (2):
  ethdev: refine new API to query supported ptypes
  doc: update which PMDs can parse packet type

 doc/guides/nics/overview.rst  | 2 +-
 lib/librte_ether/rte_ethdev.c | 3 +--
 lib/librte_ether/rte_ethdev.h | 9 ++++++---
 3 files changed, 8 insertions(+), 6 deletions(-)
  
Jianfeng Tan March 25, 2016, 10:01 a.m. UTC | #2
NACK.

I'll send an independent patchset for this.

Thanks,
Jianfeng

> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Friday, March 25, 2016 8:48 AM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng; Ananyev, Konstantin; Zhang, Helin; Richardson, Bruce
> Subject: [PATCH v4 1/3] ethdev: refine API to query supported packet types
> 
> Return 0 instead of -ENOTSUP for those which do not fill any packet types,
> with some note and doc updated.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  doc/guides/nics/overview.rst  | 2 +-
>  lib/librte_ether/rte_ethdev.c | 3 +--
>  lib/librte_ether/rte_ethdev.h | 9 ++++++---
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/guides/nics/overview.rst b/doc/guides/nics/overview.rst
> index 542479a..e7504da 100644
> --- a/doc/guides/nics/overview.rst
> +++ b/doc/guides/nics/overview.rst
> @@ -124,7 +124,7 @@ Most of these differences are summarized below.
>     L4 checksum offload          X   X   X   X
>     inner L3 checksum                X   X   X
>     inner L4 checksum                X   X   X
> -   packet type parsing          X       X   X
> +   packet type parsing  X X X X     X     X   X                     X   X X X X X X   X
>     timesync                             X X
>     basic stats                  X   X   X X X X                               X X
>     extended stats                   X   X X X X
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a328027..1ee79d2 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1636,8 +1636,7 @@ rte_eth_dev_get_supported_ptypes(uint8_t
> port_id, uint32_t ptype_mask,
> 
>  	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-
> >dev_supported_ptypes_get,
> -				-ENOTSUP);
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >dev_supported_ptypes_get, 0);
>  	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
> 
>  	if (!all_ptypes)
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index e7de34a..5167750 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2326,6 +2326,9 @@ void rte_eth_dev_info_get(uint8_t port_id, struct
> rte_eth_dev_info *dev_info);
>   * @note
>   *   Better to invoke this API after the device is already started or rx burst
>   *   function is decided, to obtain correct supported ptypes.
> + * @note
> + *   if a given PMD does not report what ptypes it supports, then the
> supported
> + *   ptype count is reported as 0.
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param ptype_mask
> @@ -2335,9 +2338,9 @@ void rte_eth_dev_info_get(uint8_t port_id, struct
> rte_eth_dev_info *dev_info);
>   * @param num
>   *  Size of the array pointed by param ptypes.
>   * @return
> - *   - (>0) Number of supported ptypes. If it exceeds param num, exceeding
> - *          packet types will not be filled in the given array.
> - *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
> + *   - (>=0) Number of supported ptypes. If the number of types exceeds
> num,
> +             only num entries will be filled into the ptypes array, but the full
> +             count of supported ptypes will be returned.
>   *   - (-ENODEV) if *port_id* invalid.
>   */
>  int rte_eth_dev_get_supported_ptypes(uint8_t port_id, uint32_t
> ptype_mask,
> --
> 2.1.4
  
Bruce Richardson March 25, 2016, 10:13 a.m. UTC | #3
On Fri, Mar 25, 2016 at 08:47:45AM +0800, Jianfeng Tan wrote:
> Return 0 instead of -ENOTSUP for those which do not fill any packet types,
> with some note and doc updated.
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Hi Jianfeng,

I think this is a good change to the API, as it should simplify app code - as
any driver which doesn't tell us what ptypes it supports should be counted as
not supporting any. It also eliminates the need for the vdevs to see about
exporting this function to say they don't support any types.

However, two comments:
1. I think the commit message for this change should include information as to
why we want to tweak the API of this new function i.e. put in the above reasons
plus any others.
2. Please separate out the doc change from the API change as they are unrelated.

Regards,
/Bruce
  
Ananyev, Konstantin March 25, 2016, 10:57 a.m. UTC | #4
> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Friday, March 25, 2016 3:16 AM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng; Ananyev, Konstantin; Zhang, Helin; Richardson, Bruce
> Subject: [PATCH 0/2] ethdev: refine new API to query supported ptypes
> 
> patch 0: return 0 instead of -ENOTSUP.
> patch 1: update doc/guides/nics/overview.rst.
> 
> Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Jianfeng Tan (2):
>   ethdev: refine new API to query supported ptypes
>   doc: update which PMDs can parse packet type
> 
>  doc/guides/nics/overview.rst  | 2 +-
>  lib/librte_ether/rte_ethdev.c | 3 +--
>  lib/librte_ether/rte_ethdev.h | 9 ++++++---
>  3 files changed, 8 insertions(+), 6 deletions(-)
> 

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 2.1.4
  

Patch

diff --git a/doc/guides/nics/overview.rst b/doc/guides/nics/overview.rst
index 542479a..e7504da 100644
--- a/doc/guides/nics/overview.rst
+++ b/doc/guides/nics/overview.rst
@@ -124,7 +124,7 @@  Most of these differences are summarized below.
    L4 checksum offload          X   X   X   X
    inner L3 checksum                X   X   X
    inner L4 checksum                X   X   X
-   packet type parsing          X       X   X
+   packet type parsing  X X X X     X     X   X                     X   X X X X X X   X
    timesync                             X X
    basic stats                  X   X   X X X X                               X X
    extended stats                   X   X X X X
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a328027..1ee79d2 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1636,8 +1636,7 @@  rte_eth_dev_get_supported_ptypes(uint8_t port_id, uint32_t ptype_mask,
 
 	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->dev_supported_ptypes_get,
-				-ENOTSUP);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0);
 	all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev);
 
 	if (!all_ptypes)
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index e7de34a..5167750 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2326,6 +2326,9 @@  void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info);
  * @note
  *   Better to invoke this API after the device is already started or rx burst
  *   function is decided, to obtain correct supported ptypes.
+ * @note
+ *   if a given PMD does not report what ptypes it supports, then the supported
+ *   ptype count is reported as 0.
  * @param port_id
  *   The port identifier of the Ethernet device.
  * @param ptype_mask
@@ -2335,9 +2338,9 @@  void rte_eth_dev_info_get(uint8_t port_id, struct rte_eth_dev_info *dev_info);
  * @param num
  *  Size of the array pointed by param ptypes.
  * @return
- *   - (>0) Number of supported ptypes. If it exceeds param num, exceeding
- *          packet types will not be filled in the given array.
- *   - (0 or -ENOTSUP) if PMD does not fill the specified ptype.
+ *   - (>=0) Number of supported ptypes. If the number of types exceeds num,
+             only num entries will be filled into the ptypes array, but the full
+             count of supported ptypes will be returned.
  *   - (-ENODEV) if *port_id* invalid.
  */
 int rte_eth_dev_get_supported_ptypes(uint8_t port_id, uint32_t ptype_mask,