[dpdk-dev,09/11] ethdev: fix port probing notification

  The new device was notified as soon as it was allocated.
It leads to use a device which is not yet initialized.

The notification must be published after the initialization is done
by the PMD, but before the state is changed, in order to let
notified entities taking ownership before general availability.

Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ethdev/rte_ethdev.c        | 5 ++---
 lib/librte_ethdev/rte_ethdev_driver.h | 2 ++
 2 files changed, 4 insertions(+), 3 deletions(-)
  

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The new device was notified as soon as it was allocated.
> It leads to use a device which is not yet initialized.
> 
> The notification must be published after the initialization is done
> by the PMD, but before the state is changed, in order to let
> notified entities taking ownership before general availability.
> 
> Fixes: 29aa41e36de7 ("ethdev: add notifications for probing and removal")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c        | 5 ++---
>  lib/librte_ethdev/rte_ethdev_driver.h | 2 ++
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 91cd0db11..e1209bb2a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -315,9 +315,6 @@ rte_eth_dev_allocate(const char *name)
>  unlock:
>  	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	if (eth_dev != NULL)
> -		_rte_eth_dev_callback_process(eth_dev, RTE_ETH_EVENT_NEW, NULL);
> -
>  	return eth_dev;
>  }
>  
> @@ -3386,6 +3383,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>  	if (dev == NULL)
>  		return;
>  
> +	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
> +

Technically we can have as many ethdev created as we want in probe() right?
Doesn't have to be a one to one mapping there, having user event in
rte_eth_dev_allocate() guaranties each ethdev created sends the event.

But when you moved this into probe() now one event sent for event, same comment
for previous one, I don't think it is good idea to tie ethdev allocation with
probe()

>  	dev->state = RTE_ETH_DEV_ATTACHED;
>  }
>  
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 3640dff68..c9c825e3f 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -106,6 +106,8 @@ int _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
>   * This is the last step of device probing.
>   * It must be called after a port is allocated and initialized successfully.
>   *
> + * The notification RTE_ETH_EVENT_NEW is sent to other entities
> + * (libraries and applications).
>   * The state is set as RTE_ETH_DEV_ATTACHED.
>   *
>   * @param dev
>
  

09/05/2018 20:07, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > @@ -3386,6 +3383,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
> >  	if (dev == NULL)
> >  		return;
> >  
> > +	_rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_NEW, NULL);
> > +
> 
> Technically we can have as many ethdev created as we want in probe() right?

Yes probing can create several ports.

> Doesn't have to be a one to one mapping there, having user event in
> rte_eth_dev_allocate() guaranties each ethdev created sends the event.

Allocation is too early to notify a new port.
We need to wait it is initialized before using it.

> But when you moved this into probe() now one event sent for event, same comment
> for previous one, I don't think it is good idea to tie ethdev allocation with
> probe()

The PMD sends one event per port by calling the appropriate ethdev function.
Event and allocation are not tied. I don't see the issue.

Note the definition of this event has always been about probing,
not allocation:
	RTE_ETH_EVENT_NEW,      /**< port is probed */