diff mbox

[dpdk-dev,v5,2/4] net/ixgbe: add support of reset

Message ID 1498817556-64379-3-git-send-email-wei.dai@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Checks

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

Commit Message

Wei Dai June 30, 2017, 10:12 a.m. UTC 
  Reset a NIC by calling dev_uninit and then dev_init.
Go through same way in NIC PCI remove without release of
ethdev resource and then NIC PCI probe function without
ethdev resource allocation.

Signed-off-by: Wei Dai <wei.dai@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 42 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

Comments

Thomas Monjalon July 7, 2017, 8:25 a.m. UTC | #1 
Hi,

30/06/2017 12:12, Wei Dai:
> +/*
> + * Reest PF device.
> + */
> +static int
> +ixgbe_dev_reset(struct rte_eth_dev *dev)
> +{
> +       int ret;
> +
> +       /* To avoid unexpected behavior in VF, disable PF reset */
> +       if (dev->data->sriov.active)
> +               return -ENOTSUP;
> +
> +       ret = eth_ixgbe_dev_uninit(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = eth_ixgbe_dev_init(dev);
> +
> +       return ret;
> +}

rte_eth_dev_reset() just do
+       rte_eth_dev_stop(port_id);
+       ret = dev->dev_ops->dev_reset(dev);

and dev_reset() just do
+       ret = eth_ixgbe_dev_uninit(dev);
+       ret = eth_ixgbe_dev_init(dev);

It is doing one more thing, the check of SR-IOV.
Unfortunately, this restriction is not documented.

This is the documentation of the new API:

 /**
+ * Reset a Ethernet device.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ */
+int rte_eth_dev_reset(uint8_t port_id);

It is really really too short.
From the beginning of this proposal we are asking you to better explain
why this API is needed. It still does not appear in the doc.
Are you adding it to offer a new service to DPDK application developpers?
Or is it just a secret sauce that you will explain only to your customers?

This is what is expected to be documented:
- why/when this API must be used
- what the API will do
- what is needed to do after
Thomas Monjalon July 7, 2017, 8:36 a.m. UTC | #2 
07/07/2017 10:25, Thomas Monjalon:
> Hi,
> 
> 30/06/2017 12:12, Wei Dai:
> > +/*
> > + * Reest PF device.
> > + */
> > +static int
> > +ixgbe_dev_reset(struct rte_eth_dev *dev)
> > +{
> > +       int ret;
> > +
> > +       /* To avoid unexpected behavior in VF, disable PF reset */
> > +       if (dev->data->sriov.active)
> > +               return -ENOTSUP;
> > +
> > +       ret = eth_ixgbe_dev_uninit(dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = eth_ixgbe_dev_init(dev);
> > +
> > +       return ret;
> > +}
> 
> rte_eth_dev_reset() just do
> +       rte_eth_dev_stop(port_id);
> +       ret = dev->dev_ops->dev_reset(dev);
> 
> and dev_reset() just do
> +       ret = eth_ixgbe_dev_uninit(dev);
> +       ret = eth_ixgbe_dev_init(dev);
> 
> It is doing one more thing, the check of SR-IOV.
> Unfortunately, this restriction is not documented.
> 
> This is the documentation of the new API:
> 
>  /**
> + * Reset a Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + */
> +int rte_eth_dev_reset(uint8_t port_id);
> 
> It is really really too short.
> From the beginning of this proposal we are asking you to better explain
> why this API is needed. It still does not appear in the doc.
> Are you adding it to offer a new service to DPDK application developpers?
> Or is it just a secret sauce that you will explain only to your customers?
> 
> This is what is expected to be documented:
> - why/when this API must be used
> - what the API will do
> - what is needed to do after

I would like to add that the description of the API must also help
other PMD maintainers to implement it.
Adding a new op means more work for PMD maintainers, that's why
they should understand the benefit and acknowledge it.
Ferruh, as the maintainer of next-net, please could you ask for feedbacks
from other PMD maintainers?
Wei Dai July 10, 2017, 10:19 a.m. UTC | #3 
Thanks, Thomas
I have just sent out my v6 patch set which includes more details to explain why/what/when.

-----Original Message-----
From: Thomas Monjalon [mailto:thomas@monjalon.net] 
Sent: Friday, July 7, 2017 4:36 PM
To: Dai, Wei <wei.dai@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>; Peng, Yuan <yuan.peng@intel.com>
Subject: Re: [dpdk-dev] [PATCH v5 2/4] net/ixgbe: add support of reset

07/07/2017 10:25, Thomas Monjalon:
> Hi,
> 
> 30/06/2017 12:12, Wei Dai:
> > +/*
> > + * Reest PF device.
> > + */
> > +static int
> > +ixgbe_dev_reset(struct rte_eth_dev *dev) {
> > +       int ret;
> > +
> > +       /* To avoid unexpected behavior in VF, disable PF reset */
> > +       if (dev->data->sriov.active)
> > +               return -ENOTSUP;
> > +
> > +       ret = eth_ixgbe_dev_uninit(dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = eth_ixgbe_dev_init(dev);
> > +
> > +       return ret;
> > +}
> 
> rte_eth_dev_reset() just do
> +       rte_eth_dev_stop(port_id);
> +       ret = dev->dev_ops->dev_reset(dev);
> 
> and dev_reset() just do
> +       ret = eth_ixgbe_dev_uninit(dev);
> +       ret = eth_ixgbe_dev_init(dev);
> 
> It is doing one more thing, the check of SR-IOV.
> Unfortunately, this restriction is not documented.
> 
> This is the documentation of the new API:
> 
>  /**
> + * Reset a Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + */
> +int rte_eth_dev_reset(uint8_t port_id);
> 
> It is really really too short.
> From the beginning of this proposal we are asking you to better 
> explain why this API is needed. It still does not appear in the doc.
> Are you adding it to offer a new service to DPDK application developpers?
> Or is it just a secret sauce that you will explain only to your customers?
> 
> This is what is expected to be documented:
> - why/when this API must be used
> - what the API will do
> - what is needed to do after

I would like to add that the description of the API must also help other PMD maintainers to implement it.
Adding a new op means more work for PMD maintainers, that's why they should understand the benefit and acknowledge it.
Ferruh, as the maintainer of next-net, please could you ask for feedbacks from other PMD maintainers?
diff mbox

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index ebc5467..e2f1da0 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -170,6 +170,7 @@  static void ixgbe_dev_stop(struct rte_eth_dev *dev);
 static int  ixgbe_dev_set_link_up(struct rte_eth_dev *dev);
 static int  ixgbe_dev_set_link_down(struct rte_eth_dev *dev);
 static void ixgbe_dev_close(struct rte_eth_dev *dev);
+static int  ixgbe_dev_reset(struct rte_eth_dev *dev);
 static void ixgbe_dev_promiscuous_enable(struct rte_eth_dev *dev);
 static void ixgbe_dev_promiscuous_disable(struct rte_eth_dev *dev);
 static void ixgbe_dev_allmulticast_enable(struct rte_eth_dev *dev);
@@ -264,6 +265,7 @@  static int  ixgbevf_dev_configure(struct rte_eth_dev *dev);
 static int  ixgbevf_dev_start(struct rte_eth_dev *dev);
 static void ixgbevf_dev_stop(struct rte_eth_dev *dev);
 static void ixgbevf_dev_close(struct rte_eth_dev *dev);
+static int  ixgbevf_dev_reset(struct rte_eth_dev *dev);
 static void ixgbevf_intr_disable(struct ixgbe_hw *hw);
 static void ixgbevf_intr_enable(struct ixgbe_hw *hw);
 static void ixgbevf_dev_stats_get(struct rte_eth_dev *dev,
@@ -525,6 +527,7 @@  static const struct eth_dev_ops ixgbe_eth_dev_ops = {
 	.dev_set_link_up    = ixgbe_dev_set_link_up,
 	.dev_set_link_down  = ixgbe_dev_set_link_down,
 	.dev_close            = ixgbe_dev_close,
+	.dev_reset            = ixgbe_dev_reset,
 	.promiscuous_enable   = ixgbe_dev_promiscuous_enable,
 	.promiscuous_disable  = ixgbe_dev_promiscuous_disable,
 	.allmulticast_enable  = ixgbe_dev_allmulticast_enable,
@@ -614,6 +617,7 @@  static const struct eth_dev_ops ixgbevf_eth_dev_ops = {
 	.xstats_reset         = ixgbevf_dev_stats_reset,
 	.xstats_get_names     = ixgbevf_dev_xstats_get_names,
 	.dev_close            = ixgbevf_dev_close,
+	.dev_reset            = ixgbevf_dev_reset,
 	.allmulticast_enable  = ixgbevf_dev_allmulticast_enable,
 	.allmulticast_disable = ixgbevf_dev_allmulticast_disable,
 	.dev_infos_get        = ixgbevf_dev_info_get,
@@ -2838,6 +2842,27 @@  ixgbe_dev_close(struct rte_eth_dev *dev)
 	ixgbe_set_rar(hw, 0, hw->mac.addr, 0, IXGBE_RAH_AV);
 }
 
+/*
+ * Reest PF device.
+ */
+static int
+ixgbe_dev_reset(struct rte_eth_dev *dev)
+{
+	int ret;
+
+	/* To avoid unexpected behavior in VF, disable PF reset */
+	if (dev->data->sriov.active)
+		return -ENOTSUP;
+
+	ret = eth_ixgbe_dev_uninit(dev);
+	if (ret)
+		return ret;
+
+	ret = eth_ixgbe_dev_init(dev);
+
+	return ret;
+}
+
 static void
 ixgbe_read_stats_registers(struct ixgbe_hw *hw,
 			   struct ixgbe_hw_stats *hw_stats,
@@ -4902,6 +4927,23 @@  ixgbevf_dev_close(struct rte_eth_dev *dev)
 	ixgbevf_remove_mac_addr(dev, 0);
 }
 
+/*
+ * Reset VF device
+ */
+static int
+ixgbevf_dev_reset(struct rte_eth_dev *dev)
+{
+	int ret;
+
+	ret = eth_ixgbevf_dev_uninit(dev);
+	if (ret)
+		return ret;
+
+	ret = eth_ixgbevf_dev_init(dev);
+
+	return ret;
+}
+
 static void ixgbevf_set_vfta_all(struct rte_eth_dev *dev, bool on)
 {
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);