diff mbox series

[v8,01/19] ethdev: add function to release port in local process

Message ID 20180702054450.29269-2-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series enable hotplug on multi-process |

Checks

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

Commit Message

Qi Zhang July 2, 2018, 5:44 a.m. UTC 
  Add driver API rte_eth_release_port_private to support the
requirement that an ethdev only be released on secondary process,
so only local state be set to unused, share data will not be
reset so primary process can still use it.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Remy Horton <remy.horton@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c        | 12 ++++++++++++
 lib/librte_ethdev/rte_ethdev_driver.h | 13 +++++++++++++
 lib/librte_ethdev/rte_ethdev_pci.h    |  3 +++
 3 files changed, 28 insertions(+)

Comments

Thomas Monjalon July 3, 2018, 9:21 a.m. UTC | #1 
Hi,

02/07/2018 07:44, Qi Zhang:
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -70,6 +70,19 @@ int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
>  
>  /**
>   * @internal
> + * Release the specified ethdev port in local process, only set to ethdev
> + * state to unused, but not reset share data since it assume other process
> + * is still using it, typically it is called by secondary process.

Please check grammar in doxygen comments, and do not make long sentences.

"only set to ethdev state to unused" -> "only set ethdev state to unused"
"share" -> "shared"
"it assume" -> "it assumes"

Please split sentences with a dot and an uppercase.
I expect all patches of the series must be reviewed for english wording.

> + *
> + * @param eth_dev
> + * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.

This comment is useless.
You can just say "Device to be detached".

> + * @return
> + *   - 0 on success, negative on error
> + */
> +int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);


[...]
> --- a/lib/librte_ethdev/rte_ethdev_pci.h
> +++ b/lib/librte_ethdev/rte_ethdev_pci.h
> @@ -197,6 +197,9 @@ rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return rte_eth_dev_release_port_private(eth_dev);

This is a behaviour change.
It means a PCI device cannot be globally detached directly by a secondary.
It must be justified by a comment in the code.

Please explain.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index a9977df97..52a97694c 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -359,6 +359,18 @@  rte_eth_dev_attach_secondary(const char *name)
 }
 
 int
+rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev)
+{
+	if (eth_dev == NULL)
+		return -EINVAL;
+
+	_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_DESTROY, NULL);
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
+
+	return 0;
+}
+
+int
 rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
 	if (eth_dev == NULL)
diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
index c9c825e3f..49c27223d 100644
--- a/lib/librte_ethdev/rte_ethdev_driver.h
+++ b/lib/librte_ethdev/rte_ethdev_driver.h
@@ -70,6 +70,19 @@  int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
 
 /**
  * @internal
+ * Release the specified ethdev port in local process, only set to ethdev
+ * state to unused, but not reset share data since it assume other process
+ * is still using it, typically it is called by secondary process.
+ *
+ * @param eth_dev
+ * The *eth_dev* pointer is the address of the *rte_eth_dev* structure.
+ * @return
+ *   - 0 on success, negative on error
+ */
+int rte_eth_dev_release_port_private(struct rte_eth_dev *eth_dev);
+
+/**
+ * @internal
  * Release device queues and clear its configuration to force the user
  * application to reconfigure it. It is for internal use only.
  *
diff --git a/lib/librte_ethdev/rte_ethdev_pci.h b/lib/librte_ethdev/rte_ethdev_pci.h
index 2cfd37274..eeb944146 100644
--- a/lib/librte_ethdev/rte_ethdev_pci.h
+++ b/lib/librte_ethdev/rte_ethdev_pci.h
@@ -197,6 +197,9 @@  rte_eth_dev_pci_generic_remove(struct rte_pci_device *pci_dev,
 	if (!eth_dev)
 		return -ENODEV;
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return rte_eth_dev_release_port_private(eth_dev);
+
 	if (dev_uninit) {
 		ret = dev_uninit(eth_dev);
 		if (ret)