diff mbox series

[v2,1/6] rte_ethdev: add function to check if device is owned

Message ID 20200316160923.5335-2-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series check for owned ports in portmask |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger March 16, 2020, 4:09 p.m. UTC 
  This is a simple helper function to check if device is owned
(being used as a sub-device). It is more convienent than having
applications call rte_eth_dev_owner_get and check the result.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
v2 - rename the helper function and use rte_eth_dev_owner_get

 lib/librte_ethdev/rte_ethdev.c           | 12 ++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 13 +++++++++++++
 lib/librte_ethdev/rte_ethdev_version.map |  3 +++
 3 files changed, 28 insertions(+)

Comments

Thomas Monjalon April 1, 2020, 9:42 p.m. UTC | #1 
16/03/2020 17:09, Stephen Hemminger:
> This is a simple helper function to check if device is owned
> (being used as a sub-device).

I would suggest not restricting ownership to sub-device case.

> It is more convienent than having
> applications call rte_eth_dev_owner_get and check the result.

Yes it is more convenient but I don't like adding such simple wrapper.

I propose to extend rte_eth_dev_owner_get() behaviour:
if the owner pointer is NULL, the function returns 0 only
if an owner (not RTE_ETH_DEV_NO_OWNER) is found.

So instead of using your wrapper:
	if (rte_eth_dev_is_owned(port_id))
you can use:
	if (rte_eth_dev_owner_get(port_id, NULL) == 0)

[...]
> +int
> +rte_eth_dev_is_owned(uint16_t port_id)
> +{
> +	struct rte_eth_dev_owner owner;
> +	int ret;
> +
> +	ret = rte_eth_dev_owner_get(port_id, &owner);
> +	if (ret == 0)
> +		ret = (owner.id != RTE_ETH_DEV_NO_OWNER);
> +	return ret;
> +}
Stephen Hemminger April 1, 2020, 10:24 p.m. UTC | #2 
On Wed, 01 Apr 2020 23:42:44 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 16/03/2020 17:09, Stephen Hemminger:
> > This is a simple helper function to check if device is owned
> > (being used as a sub-device).  
> 
> I would suggest not restricting ownership to sub-device case.
> 
> > It is more convienent than having
> > applications call rte_eth_dev_owner_get and check the result.  
> 
> Yes it is more convenient but I don't like adding such simple wrapper.
> 
> I propose to extend rte_eth_dev_owner_get() behaviour:
> if the owner pointer is NULL, the function returns 0 only
> if an owner (not RTE_ETH_DEV_NO_OWNER) is found.
> 
> So instead of using your wrapper:
> 	if (rte_eth_dev_is_owned(port_id))
> you can use:
> 	if (rte_eth_dev_owner_get(port_id, NULL) == 0)

That is not how rte_eth_dev_owner_get works now.
Passing NULL to it would crash.

And if devices is not owned rte_eth_dev_owner_get() returns 0
and owner is set to a magic value. We could change the ABI for this 
since it is marked experimental. But that seems more risky.
Thomas Monjalon April 2, 2020, 8:04 a.m. UTC | #3 
02/04/2020 00:24, Stephen Hemminger:
> On Wed, 01 Apr 2020 23:42:44 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 16/03/2020 17:09, Stephen Hemminger:
> > > This is a simple helper function to check if device is owned
> > > (being used as a sub-device).  
> > 
> > I would suggest not restricting ownership to sub-device case.
> > 
> > > It is more convienent than having
> > > applications call rte_eth_dev_owner_get and check the result.  
> > 
> > Yes it is more convenient but I don't like adding such simple wrapper.
> > 
> > I propose to extend rte_eth_dev_owner_get() behaviour:
> > if the owner pointer is NULL, the function returns 0 only
> > if an owner (not RTE_ETH_DEV_NO_OWNER) is found.
> > 
> > So instead of using your wrapper:
> > 	if (rte_eth_dev_is_owned(port_id))
> > you can use:
> > 	if (rte_eth_dev_owner_get(port_id, NULL) == 0)
> 
> That is not how rte_eth_dev_owner_get works now.
> Passing NULL to it would crash.

Indeed this is why I am proposing to extend it
and fill this case.

> And if devices is not owned rte_eth_dev_owner_get() returns 0
> and owner is set to a magic value. We could change the ABI for this 
> since it is marked experimental. But that seems more risky.

This is not an ABI change.
Just manage a case which was crashing previously.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 774c721b3484..6e2d0110f65e 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -737,6 +737,18 @@  rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 	return ret;
 }
 
+int
+rte_eth_dev_is_owned(uint16_t port_id)
+{
+	struct rte_eth_dev_owner owner;
+	int ret;
+
+	ret = rte_eth_dev_owner_get(port_id, &owner);
+	if (ret == 0)
+		ret = (owner.id != RTE_ETH_DEV_NO_OWNER);
+	return ret;
+}
+
 int
 rte_eth_dev_socket_id(uint16_t port_id)
 {
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad112a..3a8a4bcde4b7 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1693,6 +1693,19 @@  __rte_experimental
 int rte_eth_dev_owner_get(const uint16_t port_id,
 		struct rte_eth_dev_owner *owner);
 
+/**
+ * Check if port_id is part of a sub-device
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device
+ * @return
+ *   - 1 if device is associated with an owner
+ *   - 0 if port is not owned
+ *   - negative errno value on error.
+ */
+__rte_experimental
+int rte_eth_dev_is_owned(uint16_t port_id);
+
 /**
  * Get the number of ports which are usable for the application.
  *
diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
index 3f32fdecf722..435df2dba149 100644
--- a/lib/librte_ethdev/rte_ethdev_version.map
+++ b/lib/librte_ethdev/rte_ethdev_version.map
@@ -230,4 +230,7 @@  EXPERIMENTAL {
 
 	# added in 20.02
 	rte_flow_dev_dump;
+
+	# added in 20.05
+	rte_eth_dev_is_owned;
 };