diff mbox series

[04/20] net/atlantic: release port upon close

Message ID 20200913220711.3768597-5-thomas@monjalon.net (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series cleanup ethdev close operation |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Thomas Monjalon Sept. 13, 2020, 10:06 p.m. UTC 
  The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
can be freed by rte_eth_dev_close().

Freeing of private port resources is moved
from the ".remove(device)" to the ".dev_close(port)" operation.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/atlantic/atl_ethdev.c | 62 ++++++++++++-------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

Comments

Igor Russkikh Sept. 16, 2020, 3:14 p.m. UTC | #1 
On 14/09/2020 1:06 am, Thomas Monjalon wrote:
> The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> can be freed by rte_eth_dev_close().
> 
> Freeing of private port resources is moved
> from the ".remove(device)" to the ".dev_close(port)" operation.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Thanks,

Acked-by: Igor Russkikh <irusskikh@marvell.com>

I'll verify this soon on HW.

Igor
Ferruh Yigit Sept. 23, 2020, 4:42 p.m. UTC | #2 
On 9/13/2020 11:06 PM, Thomas Monjalon wrote:
> The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> can be freed by rte_eth_dev_close().
> 
> Freeing of private port resources is moved
> from the ".remove(device)" to the ".dev_close(port)" operation.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

<...>

> @@ -721,12 +687,32 @@ atl_dev_set_link_down(struct rte_eth_dev *dev)
>   static int
>   atl_dev_close(struct rte_eth_dev *dev)
>   {
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
> +	struct aq_hw_s *hw;
> +
>   	PMD_INIT_FUNC_TRACE();
>   
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -EPERM;

Is this case an error, or should it return 0.

> +
> +	hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
>   	atl_dev_stop(dev);
>   
>   	atl_free_queues(dev);
>   
> +	dev->dev_ops = NULL;
> +	dev->rx_pkt_burst = NULL;
> +	dev->tx_pkt_burst = NULL;

What do you think moving above cleanup to 'rte_eth_dev_release_port()'?
Thomas Monjalon Sept. 23, 2020, 8:50 p.m. UTC | #3 
23/09/2020 18:42, Ferruh Yigit:
> On 9/13/2020 11:06 PM, Thomas Monjalon wrote:
> > The flag RTE_ETH_DEV_CLOSE_REMOVE is set so all port resources
> > can be freed by rte_eth_dev_close().
> > 
> > Freeing of private port resources is moved
> > from the ".remove(device)" to the ".dev_close(port)" operation.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> 
> <...>
> 
> > @@ -721,12 +687,32 @@ atl_dev_set_link_down(struct rte_eth_dev *dev)
> >   static int
> >   atl_dev_close(struct rte_eth_dev *dev)
> >   {
> > +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> > +	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
> > +	struct aq_hw_s *hw;
> > +
> >   	PMD_INIT_FUNC_TRACE();
> >   
> > +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> > +		return -EPERM;
> 
> Is this case an error, or should it return 0.

In other drivers, I think I chose 0.

> > +	dev->dev_ops = NULL;
> > +	dev->rx_pkt_burst = NULL;
> > +	dev->tx_pkt_burst = NULL;
> 
> What do you think moving above cleanup to 'rte_eth_dev_release_port()'?

I asked myself the same question.
I think we should but I propose to do this change in a different series.
diff mbox series

Patch

diff --git a/drivers/net/atlantic/atl_ethdev.c b/drivers/net/atlantic/atl_ethdev.c
index 1d76883c52..16721a011f 100644
--- a/drivers/net/atlantic/atl_ethdev.c
+++ b/drivers/net/atlantic/atl_ethdev.c
@@ -15,8 +15,6 @@ 
 #include "hw_atl/hw_atl_b0_internal.h"
 
 static int eth_atl_dev_init(struct rte_eth_dev *eth_dev);
-static int eth_atl_dev_uninit(struct rte_eth_dev *eth_dev);
-
 static int  atl_dev_configure(struct rte_eth_dev *dev);
 static int  atl_dev_start(struct rte_eth_dev *dev);
 static void atl_dev_stop(struct rte_eth_dev *dev);
@@ -381,6 +379,8 @@  eth_atl_dev_init(struct rte_eth_dev *eth_dev)
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	eth_dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
+
 	/* Vendor and Device ID need to be set before init of shared code */
 	hw->device_id = pci_dev->id.device_id;
 	hw->vendor_id = pci_dev->id.vendor_id;
@@ -441,40 +441,6 @@  eth_atl_dev_init(struct rte_eth_dev *eth_dev)
 	return err;
 }
 
-static int
-eth_atl_dev_uninit(struct rte_eth_dev *eth_dev)
-{
-	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev);
-	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
-	struct aq_hw_s *hw;
-
-	PMD_INIT_FUNC_TRACE();
-
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return -EPERM;
-
-	hw = ATL_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private);
-
-	if (hw->adapter_stopped == 0)
-		atl_dev_close(eth_dev);
-
-	eth_dev->dev_ops = NULL;
-	eth_dev->rx_pkt_burst = NULL;
-	eth_dev->tx_pkt_burst = NULL;
-
-	/* disable uio intr before callback unregister */
-	rte_intr_disable(intr_handle);
-	rte_intr_callback_unregister(intr_handle,
-				     atl_dev_interrupt_handler, eth_dev);
-
-	rte_free(eth_dev->data->mac_addrs);
-	eth_dev->data->mac_addrs = NULL;
-
-	pthread_mutex_destroy(&hw->mbox_mutex);
-
-	return 0;
-}
-
 static int
 eth_atl_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 	struct rte_pci_device *pci_dev)
@@ -486,7 +452,7 @@  eth_atl_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 static int
 eth_atl_pci_remove(struct rte_pci_device *pci_dev)
 {
-	return rte_eth_dev_pci_generic_remove(pci_dev, eth_atl_dev_uninit);
+	return rte_eth_dev_pci_generic_remove(pci_dev, atl_dev_close);
 }
 
 static int
@@ -721,12 +687,32 @@  atl_dev_set_link_down(struct rte_eth_dev *dev)
 static int
 atl_dev_close(struct rte_eth_dev *dev)
 {
+	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
+	struct rte_intr_handle *intr_handle = &pci_dev->intr_handle;
+	struct aq_hw_s *hw;
+
 	PMD_INIT_FUNC_TRACE();
 
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+		return -EPERM;
+
+	hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
 	atl_dev_stop(dev);
 
 	atl_free_queues(dev);
 
+	dev->dev_ops = NULL;
+	dev->rx_pkt_burst = NULL;
+	dev->tx_pkt_burst = NULL;
+
+	/* disable uio intr before callback unregister */
+	rte_intr_disable(intr_handle);
+	rte_intr_callback_unregister(intr_handle,
+				     atl_dev_interrupt_handler, dev);
+
+	pthread_mutex_destroy(&hw->mbox_mutex);
+
 	return 0;
 }
 
@@ -735,7 +721,7 @@  atl_dev_reset(struct rte_eth_dev *dev)
 {
 	int ret;
 
-	ret = eth_atl_dev_uninit(dev);
+	ret = atl_dev_close(dev);
 	if (ret)
 		return ret;