[dpdk-dev] [PATCH 20.05] ethdev: clear struct on releasing port

[Ferruh Yigit]

On 2/13/2020 2:33 PM, Thomas Monjalon wrote:
> The function rte_eth_dev_release_port() is called by drivers
> when closing or removing a device.
> Its main usage should be via rte_eth_dev_close() by up-to-date
> drivers which are compliant with RTE_ETH_DEV_CLOSE_REMOVE flag.
> 
> When a port is released, the data (rte_eth_dev_data) are cleared,
> but the pointers in rte_eth_dev were not cleared.
> It may cause issues with code paths trying to use dangling pointers
> (e.g. the .device pointer which may reference a removed rte_device).
> Everything is now cleared to 0 when releasing.
> The state is explicitly set to RTE_ETH_DEV_UNUSED which is 0 anyway.
> 
> Using this patch may reveal bugs in some code paths:
> 	- device pointer must be saved before closing a port if needed

Is this saving should be done in application code or will be done by ethdev?

> 	- drivers must close ports on device remove for consistent cleanup
> 
> Signed-off-by: Thomas Monjalon <thomas at monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 774c721b34..2a43a9abe9 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -553,8 +553,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>  
>  	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	eth_dev->state = RTE_ETH_DEV_UNUSED;
> -
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		rte_free(eth_dev->data->rx_queues);
>  		rte_free(eth_dev->data->tx_queues);
> @@ -563,6 +561,8 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>  		rte_free(eth_dev->data->dev_private);
>  		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
>  	}
> +	memset(eth_dev, 0, sizeof(struct rte_eth_dev));
> +	eth_dev->state = RTE_ETH_DEV_UNUSED;
>  
>  	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
>  
>