[dpdk-dev,v6,16/17] ethdev: convert to eal hotplug

  Remove bus logic from ethdev hotplug by using eal for this.

Current api is preserved:
- the last port that has been created is tracked to return it to the
  application when attaching,
- the internal device name is reused when detaching.

We can not get rid of ethdev hotplug yet since we still need some mechanism
to inform applications of port creation/removal to substitute for ethdev
hotplug api.

dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as
is, but this information is not needed anymore and is removed in the following
commit.

Signed-off-by: David Marchand <david.marchand@6wind.com>
Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 lib/librte_ether/rte_ethdev.c | 207 +++++++-----------------------------------
 1 file changed, 33 insertions(+), 174 deletions(-)
  

On Tue, 12 Jul 2016 11:31:21 +0530
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> Remove bus logic from ethdev hotplug by using eal for this.
> 
> Current api is preserved:
> - the last port that has been created is tracked to return it to the
>   application when attaching,
> - the internal device name is reused when detaching.
> 
> We can not get rid of ethdev hotplug yet since we still need some mechanism
> to inform applications of port creation/removal to substitute for ethdev
> hotplug api.
> 
> dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as
> is, but this information is not needed anymore and is removed in the following
> commit.
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 207 +++++++-----------------------------------
>  1 file changed, 33 insertions(+), 174 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a667012..8d14fd7 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -72,6 +72,7 @@
>  static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
>  struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>  static struct rte_eth_dev_data *rte_eth_dev_data;
> +static uint8_t eth_dev_last_created_port;
>  static uint8_t nb_ports;
>  

[...]

> -
>  /* attach the new device, then store port_id of the device */
>  int
>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
>  {
> -	struct rte_pci_addr addr;
>  	int ret = -1;
> +	int current = eth_dev_last_created_port;
> +	char *name = NULL;
> +	char *args = NULL;
>  
>  	if ((devargs == NULL) || (port_id == NULL)) {
>  		ret = -EINVAL;
>  		goto err;
>  	}
>  
> -	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
> -		ret = rte_eth_dev_attach_pdev(&addr, port_id);
> -		if (ret < 0)
> -			goto err;
> -	} else {
> -		ret = rte_eth_dev_attach_vdev(devargs, port_id);
> -		if (ret < 0)
> -			goto err;
> +	/* parse devargs, then retrieve device name and args */
> +	if (rte_eal_parse_devargs_str(devargs, &name, &args))
> +		goto err;
> +
> +	ret = rte_eal_dev_attach(name, args);
> +	if (ret < 0)
> +		goto err;
> +
> +	/* no point looking at eth_dev_last_created_port if no port exists */

I am not sure about this comment. What is "no point"?

Isn't this also a potential bug? (Like the one below.) How could it
happen there is no port after a successful attach?

> +	if (!nb_ports) {
> +		ret = -1;
> +		goto err;
> +	}
> +	/* if nothing happened, there is a bug here, since some driver told us
> +	 * it did attach a device, but did not create a port */
> +	if (current == eth_dev_last_created_port) {
> +		ret = -1;
> +		goto err;

Should we log this? Or call some kind panic?

>  	}
> +	*port_id = eth_dev_last_created_port;
> +	ret = 0;
>  
> -	return 0;
>  err:
> -	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
> +	free(name);
> +	free(args);
>  	return ret;
>  }
>  
> @@ -590,7 +464,6 @@ err:
>  int
>  rte_eth_dev_detach(uint8_t port_id, char *name)
>  {
> -	struct rte_pci_addr addr;
>  	int ret = -1;
>  
>  	if (name == NULL) {
> @@ -598,33 +471,19 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>  		goto err;
>  	}
>  
> -	/* check whether the driver supports detach feature, or not */
> +	/* FIXME: move this to eal, once device flags are relocated there */
>  	if (rte_eth_dev_is_detachable(port_id))
>  		goto err;
>  
> -	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
> -		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
> -		if (ret < 0)
> -			goto err;
> -
> -		ret = rte_eth_dev_detach_pdev(port_id, &addr);
> -		if (ret < 0)
> -			goto err;
> -
> -		snprintf(name, RTE_ETH_NAME_MAX_LEN,
> -			"%04x:%02x:%02x.%d",
> -			addr.domain, addr.bus,
> -			addr.devid, addr.function);
> -	} else {
> -		ret = rte_eth_dev_detach_vdev(port_id, name);
> -		if (ret < 0)
> -			goto err;
> -	}
> +	snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
> +		 "%s", rte_eth_devices[port_id].data->name);
> +	ret = rte_eal_dev_detach(name);
> +	if (ret < 0)
> +		goto err;
>  
>  	return 0;
>  
>  err:
> -	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");

I'd be more specific about the failing device, we have its name.

>  	return ret;
>  }
>
  

On Thursday 14 July 2016 10:21 PM, Jan Viktorin wrote:
> On Tue, 12 Jul 2016 11:31:21 +0530
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
>> Remove bus logic from ethdev hotplug by using eal for this.
>>
>> Current api is preserved:
>> - the last port that has been created is tracked to return it to the
>>   application when attaching,
>> - the internal device name is reused when detaching.
>>
>> We can not get rid of ethdev hotplug yet since we still need some mechanism
>> to inform applications of port creation/removal to substitute for ethdev
>> hotplug api.
>>
>> dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as
>> is, but this information is not needed anymore and is removed in the following
>> commit.
>>
>> Signed-off-by: David Marchand <david.marchand@6wind.com>
>> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
>> ---
>>  lib/librte_ether/rte_ethdev.c | 207 +++++++-----------------------------------
>>  1 file changed, 33 insertions(+), 174 deletions(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index a667012..8d14fd7 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -72,6 +72,7 @@
>>  static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
>>  struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
>>  static struct rte_eth_dev_data *rte_eth_dev_data;
>> +static uint8_t eth_dev_last_created_port;
>>  static uint8_t nb_ports;
>>  
> 
> [...]
> 
>> -
>>  /* attach the new device, then store port_id of the device */
>>  int
>>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
>>  {
>> -	struct rte_pci_addr addr;
>>  	int ret = -1;
>> +	int current = eth_dev_last_created_port;
>> +	char *name = NULL;
>> +	char *args = NULL;
>>  
>>  	if ((devargs == NULL) || (port_id == NULL)) {
>>  		ret = -EINVAL;
>>  		goto err;
>>  	}
>>  
>> -	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
>> -		ret = rte_eth_dev_attach_pdev(&addr, port_id);
>> -		if (ret < 0)
>> -			goto err;
>> -	} else {
>> -		ret = rte_eth_dev_attach_vdev(devargs, port_id);
>> -		if (ret < 0)
>> -			goto err;
>> +	/* parse devargs, then retrieve device name and args */
>> +	if (rte_eal_parse_devargs_str(devargs, &name, &args))
>> +		goto err;
>> +
>> +	ret = rte_eal_dev_attach(name, args);
>> +	if (ret < 0)
>> +		goto err;
>> +
>> +	/* no point looking at eth_dev_last_created_port if no port exists */
> 
> I am not sure about this comment. What is "no point"?
> 
> Isn't this also a potential bug? (Like the one below.) How could it
> happen there is no port after a successful attach?

Yes, even searching through code path I couldn't find a positive case where control would reach here without nb_ports>0.
Though, i am not sure if some rough application attempts to mix-up calls - and that, in my opinion, is not worth checking.
Should I remove it?

> 
>> +	if (!nb_ports) {
>> +		ret = -1;
>> +		goto err;
>> +	}
>> +	/* if nothing happened, there is a bug here, since some driver told us
>> +	 * it did attach a device, but did not create a port */
>> +	if (current == eth_dev_last_created_port) {
>> +		ret = -1;
>> +		goto err;
> 
> Should we log this? Or call some kind panic?

I will place a log because applications should have a chance to ignore a device which cannot be attached for whatever reason.

> 
>>  	}
>> +	*port_id = eth_dev_last_created_port;
>> +	ret = 0;
>>  
>> -	return 0;
>>  err:
>> -	RTE_LOG(ERR, EAL, "Driver, cannot attach the device\n");
>> +	free(name);
>> +	free(args);
>>  	return ret;
>>  }
>>  
>> @@ -590,7 +464,6 @@ err:
>>  int
>>  rte_eth_dev_detach(uint8_t port_id, char *name)
>>  {
>> -	struct rte_pci_addr addr;
>>  	int ret = -1;
>>  
>>  	if (name == NULL) {
>> @@ -598,33 +471,19 @@ rte_eth_dev_detach(uint8_t port_id, char *name)
>>  		goto err;
>>  	}
>>  
>> -	/* check whether the driver supports detach feature, or not */
>> +	/* FIXME: move this to eal, once device flags are relocated there */
>>  	if (rte_eth_dev_is_detachable(port_id))
>>  		goto err;
>>  
>> -	if (rte_eth_dev_get_device_type(port_id) == RTE_ETH_DEV_PCI) {
>> -		ret = rte_eth_dev_get_addr_by_port(port_id, &addr);
>> -		if (ret < 0)
>> -			goto err;
>> -
>> -		ret = rte_eth_dev_detach_pdev(port_id, &addr);
>> -		if (ret < 0)
>> -			goto err;
>> -
>> -		snprintf(name, RTE_ETH_NAME_MAX_LEN,
>> -			"%04x:%02x:%02x.%d",
>> -			addr.domain, addr.bus,
>> -			addr.devid, addr.function);
>> -	} else {
>> -		ret = rte_eth_dev_detach_vdev(port_id, name);
>> -		if (ret < 0)
>> -			goto err;
>> -	}
>> +	snprintf(name, sizeof(rte_eth_devices[port_id].data->name),
>> +		 "%s", rte_eth_devices[port_id].data->name);
>> +	ret = rte_eal_dev_detach(name);
>> +	if (ret < 0)
>> +		goto err;
>>  
>>  	return 0;
>>  
>>  err:
>> -	RTE_LOG(ERR, EAL, "Driver, cannot detach the device\n");
> 
> I'd be more specific about the failing device, we have its name.

Agree. I will add 'name' to this.

>>  	return ret;
>>  }
>>
>
  

On Fri, 15 Jul 2016 16:06:25 +0530
Shreyansh jain <shreyansh.jain@nxp.com> wrote:

> On Thursday 14 July 2016 10:21 PM, Jan Viktorin wrote:
> > On Tue, 12 Jul 2016 11:31:21 +0530
> > Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> >   
> >> Remove bus logic from ethdev hotplug by using eal for this.
> >>
> >> Current api is preserved:
> >> - the last port that has been created is tracked to return it to the
> >>   application when attaching,
> >> - the internal device name is reused when detaching.
> >>
> >> We can not get rid of ethdev hotplug yet since we still need some mechanism
> >> to inform applications of port creation/removal to substitute for ethdev
> >> hotplug api.
> >>
> >> dev_type field in struct rte_eth_dev and rte_eth_dev_allocate are kept as
> >> is, but this information is not needed anymore and is removed in the following
> >> commit.
> >>
> >> Signed-off-by: David Marchand <david.marchand@6wind.com>
> >> Signed-off-by: Shreyansh Jain <shreyansh.jain@nxp.com>
> >> ---
> >>  lib/librte_ether/rte_ethdev.c | 207 +++++++-----------------------------------
> >>  1 file changed, 33 insertions(+), 174 deletions(-)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >> index a667012..8d14fd7 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -72,6 +72,7 @@
> >>  static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
> >>  struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
> >>  static struct rte_eth_dev_data *rte_eth_dev_data;
> >> +static uint8_t eth_dev_last_created_port;
> >>  static uint8_t nb_ports;
> >>    
> > 
> > [...]
> >   
> >> -
> >>  /* attach the new device, then store port_id of the device */
> >>  int
> >>  rte_eth_dev_attach(const char *devargs, uint8_t *port_id)
> >>  {
> >> -	struct rte_pci_addr addr;
> >>  	int ret = -1;
> >> +	int current = eth_dev_last_created_port;
> >> +	char *name = NULL;
> >> +	char *args = NULL;
> >>  
> >>  	if ((devargs == NULL) || (port_id == NULL)) {
> >>  		ret = -EINVAL;
> >>  		goto err;
> >>  	}
> >>  
> >> -	if (eal_parse_pci_DomBDF(devargs, &addr) == 0) {
> >> -		ret = rte_eth_dev_attach_pdev(&addr, port_id);
> >> -		if (ret < 0)
> >> -			goto err;
> >> -	} else {
> >> -		ret = rte_eth_dev_attach_vdev(devargs, port_id);
> >> -		if (ret < 0)
> >> -			goto err;
> >> +	/* parse devargs, then retrieve device name and args */
> >> +	if (rte_eal_parse_devargs_str(devargs, &name, &args))
> >> +		goto err;
> >> +
> >> +	ret = rte_eal_dev_attach(name, args);
> >> +	if (ret < 0)
> >> +		goto err;
> >> +
> >> +	/* no point looking at eth_dev_last_created_port if no port exists */  
> > 
> > I am not sure about this comment. What is "no point"?
> > 
> > Isn't this also a potential bug? (Like the one below.) How could it
> > happen there is no port after a successful attach?  
> 
> Yes, even searching through code path I couldn't find a positive case where control would reach here without nb_ports>0.
> Though, i am not sure if some rough application attempts to mix-up calls - and that, in my opinion, is not worth checking.
> Should I remove it?

At least a loud log might be helpful. If there is really no path to reach this point, I'd put RTE_VERIFY here.

> 
> >   
> >> +	if (!nb_ports) {
> >> +		ret = -1;
> >> +		goto err;
> >> +	}
> >> +	/* if nothing happened, there is a bug here, since some driver told us
> >> +	 * it did attach a device, but did not create a port */
> >> +	if (current == eth_dev_last_created_port) {
> >> +		ret = -1;
> >> +		goto err;  
> > 
> > Should we log this? Or call some kind panic?  
> 
> I will place a log because applications should have a chance to ignore a device which cannot be attached for whatever reason.

OK, we just should shout loudly if it means a bug...

[...]