[dpdk-dev,08/11] ethdev: fix port visibility before initialization

  The port was set to the state ATTACHED during allocation.
The consequence was to iterate over ports which are not initialized.

The state ATTACHED is now set as the last step of probing.

The uniqueness of port name is now checked before the availability
of a port id for allocation (order reversed).

As the state is not set on allocation anymore, it is also not checked
in the function telling whether a port is allocated or not.
The name of the port is set on allocation, so it is enough as a check.

Fixes: 5588909af21b ("ethdev: add device iterator")
Cc: stable@dpdk.org

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: Matan Azrad <matan@mellanox.com>
---
 lib/librte_ethdev/rte_ethdev.c        | 18 +++++++++---------
 lib/librte_ethdev/rte_ethdev_driver.h |  2 ++
 2 files changed, 11 insertions(+), 9 deletions(-)
  

On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> The port was set to the state ATTACHED during allocation.
> The consequence was to iterate over ports which are not initialized.
> 
> The state ATTACHED is now set as the last step of probing.
> 
> The uniqueness of port name is now checked before the availability
> of a port id for allocation (order reversed).
> 
> As the state is not set on allocation anymore, it is also not checked
> in the function telling whether a port is allocated or not.
> The name of the port is set on allocation, so it is enough as a check.
> 
> Fixes: 5588909af21b ("ethdev: add device iterator")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c        | 18 +++++++++---------
>  lib/librte_ethdev/rte_ethdev_driver.h |  2 ++
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 357be2dca..91cd0db11 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -233,7 +233,7 @@ rte_eth_dev_allocated_lock_free(const char *name)
>  	unsigned i;
>  
>  	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> -		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
> +		if (rte_eth_devices[i].data != NULL &&
>  		    strcmp(rte_eth_devices[i].data->name, name) == 0)
>  			return &rte_eth_devices[i];
>  	}
> @@ -278,7 +278,6 @@ eth_dev_get(uint16_t port_id)
>  	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
>  
>  	eth_dev->data = &rte_eth_dev_shared_data->data[port_id];
> -	eth_dev->state = RTE_ETH_DEV_ATTACHED;
>  
>  	eth_dev_last_created_port = port_id;
>  
> @@ -296,16 +295,15 @@ rte_eth_dev_allocate(const char *name)
>  	/* Synchronize port creation between primary and secondary threads. */
>  	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
>  
> -	port_id = rte_eth_dev_find_free_port();
> -	if (port_id == RTE_MAX_ETHPORTS) {
> -		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
> +	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
> +		ethdev_log(ERR, "Ethernet device with name %s already allocated",
> +				name);
>  		goto unlock;
>  	}
>  
> -	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
> -		ethdev_log(ERR,
> -			"Ethernet Device with name %s already allocated!",
> -			name);
> +	port_id = rte_eth_dev_find_free_port();
> +	if (port_id == RTE_MAX_ETHPORTS) {
> +		ethdev_log(ERR, "Reached maximum number of Ethernet ports");
>  		goto unlock;
>  	}
>  
> @@ -3387,6 +3385,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
>  {
>  	if (dev == NULL)
>  		return;
> +
> +	dev->state = RTE_ETH_DEV_ATTACHED;

Aren't ethdev allocation and probe in different levels? Why not make ethdev
allocation self sufficient but tie it to the device probe() ?

>  }
>  
>  int
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index 3821f0b1d..3640dff68 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 state is set as RTE_ETH_DEV_ATTACHED.
> + *
>   * @param dev
>   *  New ethdev port.
>   */
>
  

09/05/2018 20:03, Ferruh Yigit:
> On 5/9/2018 10:43 AM, Thomas Monjalon wrote:
> > The port was set to the state ATTACHED during allocation.
> > The consequence was to iterate over ports which are not initialized.
> > 
> > The state ATTACHED is now set as the last step of probing.
> > 
> > The uniqueness of port name is now checked before the availability
> > of a port id for allocation (order reversed).
> > 
> > As the state is not set on allocation anymore, it is also not checked
> > in the function telling whether a port is allocated or not.
> > The name of the port is set on allocation, so it is enough as a check.
> > 
> > Fixes: 5588909af21b ("ethdev: add device iterator")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> > @@ -3387,6 +3385,8 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
> >  {
> >  	if (dev == NULL)
> >  		return;
> > +
> > +	dev->state = RTE_ETH_DEV_ATTACHED;
> 
> Aren't ethdev allocation and probe in different levels? Why not make ethdev
> allocation self sufficient but tie it to the device probe() ?

Ethdev allocation is self sufficient.
Probing is done by the PMD with these steps:
	1/ allocate ethdev port
	2/ initialize device
	3/ notify NEW event and set ethdev status
  

On Wed,  9 May 2018 11:43:34 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> The port was set to the state ATTACHED during allocation.
> The consequence was to iterate over ports which are not initialized.
> 
> The state ATTACHED is now set as the last step of probing.
> 
> The uniqueness of port name is now checked before the availability
> of a port id for allocation (order reversed).
> 
> As the state is not set on allocation anymore, it is also not checked
> in the function telling whether a port is allocated or not.
> The name of the port is set on allocation, so it is enough as a check.
> 
> Fixes: 5588909af21b ("ethdev: add device iterator")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> Signed-off-by: Matan Azrad <matan@mellanox.com>

Could these states be described somewhere in the documentation.
Maybe some doc/guides? Maybe even exposed in API under rte_eth_dev_info.

Ideally, the device states could be aligned to the SNMP MIB for
interfaces (RFC2863) which defines complex state model appropriate for devices
and tunnels.
  

10/05/2018 22:40, Stephen Hemminger:
> On Wed,  9 May 2018 11:43:34 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > The port was set to the state ATTACHED during allocation.
> > The consequence was to iterate over ports which are not initialized.
> > 
> > The state ATTACHED is now set as the last step of probing.
> > 
> > The uniqueness of port name is now checked before the availability
> > of a port id for allocation (order reversed).
> > 
> > As the state is not set on allocation anymore, it is also not checked
> > in the function telling whether a port is allocated or not.
> > The name of the port is set on allocation, so it is enough as a check.
> > 
> > Fixes: 5588909af21b ("ethdev: add device iterator")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> 
> Could these states be described somewhere in the documentation.

It is in this patchset:
	ethdev: add doxygen comments for each state

> Maybe some doc/guides? Maybe even exposed in API under rte_eth_dev_info.

It is an internal state. It should not be used by the application.

> Ideally, the device states could be aligned to the SNMP MIB for
> interfaces (RFC2863) which defines complex state model appropriate for devices
> and tunnels.

Something to be discussed, MIB compliance would be a totally new feature
I guess. I think it is a huge work with not a lot of interest.