diff mbox

[dpdk-dev,v3,1/6] ethdev: fix port id storage

Message ID 20180117215802.90809-1-ferruh.yigit@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Ferruh Yigit Jan. 17, 2018, 9:57 p.m. UTC 
  port_id is now 16bits, update function parameter according.

Fixes: 4c270218aa26 ("ethdev: support security APIs")
Cc: stable@dpdk.org
Cc: declan.doherty@intel.com

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
Cc: Boris Pismenny <borisp@mellanox.com>
Cc: Aviad Yehezkel <aviadye@mellanox.com>
Cc: Radu Nicolau <radu.nicolau@intel.com>
Cc: Declan Doherty <declan.doherty@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 2 +-
 lib/librte_ether/rte_ethdev.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Thomas Monjalon Jan. 17, 2018, 10:09 p.m. UTC | #1 
17/01/2018 22:57, Ferruh Yigit:
> port_id is now 16bits, update function parameter according.
> 
> Fixes: 4c270218aa26 ("ethdev: support security APIs")
> Cc: stable@dpdk.org
> Cc: declan.doherty@intel.com
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>

Obviously,
Acked-by: Thomas Monjalon <thomas@monjalon.net>
Thomas Monjalon Jan. 17, 2018, 10:19 p.m. UTC | #2 
17/01/2018 23:09, Thomas Monjalon:
> 17/01/2018 22:57, Ferruh Yigit:
> > port_id is now 16bits, update function parameter according.
> > 
> > Fixes: 4c270218aa26 ("ethdev: support security APIs")
> > Cc: stable@dpdk.org
> > Cc: declan.doherty@intel.com
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> 
> Obviously,
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Not so obvious actually.
It is a good fix, but an API change.
This function was not declared experimental.
It must wait 18.05.

And the function has no doxygen!
And the function was placed randomly in the middle of struct declarations!
One more proof of the poor quality of rte_security stuff.
Ferruh Yigit Jan. 18, 2018, 10:15 a.m. UTC | #3 
On 1/17/2018 10:19 PM, Thomas Monjalon wrote:
> 17/01/2018 23:09, Thomas Monjalon:
>> 17/01/2018 22:57, Ferruh Yigit:
>>> port_id is now 16bits, update function parameter according.
>>>
>>> Fixes: 4c270218aa26 ("ethdev: support security APIs")
>>> Cc: stable@dpdk.org
>>> Cc: declan.doherty@intel.com
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>
>> Obviously,
>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Not so obvious actually.
> It is a good fix, but an API change.

It is more like API fix.
While whole library is using 16bits for port_id, I think we shouldn't deliver
release with this specific API uses 8bits.

I am for getting this because what it does is wrong.

> This function was not declared experimental.
> It must wait 18.05.
> 
> And the function has no doxygen!

There was a request to add it [1] but not received the patch yet.

[1]
https://dpdk.org/ml/archives/dev/2017-December/082824.html

> And the function was placed randomly in the middle of struct declarations!
> One more proof of the poor quality of rte_security stuff.
>
Thomas Monjalon Jan. 18, 2018, 11:29 a.m. UTC | #4 
18/01/2018 11:15, Ferruh Yigit:
> On 1/17/2018 10:19 PM, Thomas Monjalon wrote:
> > 17/01/2018 23:09, Thomas Monjalon:
> >> 17/01/2018 22:57, Ferruh Yigit:
> >>> port_id is now 16bits, update function parameter according.
> >>>
> >>> Fixes: 4c270218aa26 ("ethdev: support security APIs")
> >>> Cc: stable@dpdk.org
> >>> Cc: declan.doherty@intel.com
> >>>
> >>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> >>
> >> Obviously,
> >> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> > 
> > Not so obvious actually.
> > It is a good fix, but an API change.
> 
> It is more like API fix.
> While whole library is using 16bits for port_id, I think we shouldn't deliver
> release with this specific API uses 8bits.
> 
> I am for getting this because what it does is wrong.
> 
> > This function was not declared experimental.
> > It must wait 18.05.

I really want to keep API stable in 18.02.
We have to keep this mistake for one more release.
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index b3495992d..779853d02 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -277,7 +277,7 @@  rte_eth_dev_socket_id(uint16_t port_id)
 }
 
 void *
-rte_eth_dev_get_sec_ctx(uint8_t port_id)
+rte_eth_dev_get_sec_ctx(uint16_t port_id)
 {
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, NULL);
 	return rte_eth_devices[port_id].security_ctx;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index a5ba56412..e4927029d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1722,7 +1722,7 @@  struct rte_eth_dev {
 } __rte_cache_aligned;
 
 void *
-rte_eth_dev_get_sec_ctx(uint8_t port_id);
+rte_eth_dev_get_sec_ctx(uint16_t port_id);
 
 struct rte_eth_dev_sriov {
 	uint8_t active;               /**< SRIOV is active with 16, 32 or 64 pools */