Re: [dpdk-dev] [PATCH v8 07/14] ethdev: Add functions that will be used by port hotplug functions

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/02/17 10:04, Thomas Monjalon wrote:
> 2015-02-16 13:14, Tetsuya Mukawa:
>> The patch adds following functions.
>>
>> - rte_eth_dev_save()
>>   The function is used for saving current rte_eth_dev structures.
>> - rte_eth_dev_get_changed_port()
>>   The function receives the rte_eth_dev structures, then compare
>>   these with current values to know which port is actually
>>   attached or detached.
>> - rte_eth_dev_get_addr_by_port()
>>   The function returns a pci address of an ethdev specified by port
>>   identifier.
>> - rte_eth_dev_get_port_by_addr()
>>   The function returns a port identifier of an ethdev specified by
>>   pci address.
>> - rte_eth_dev_get_name_by_port()
>>   The function returns a unique identifier name of an ethdev
>>   specified by port identifier.
>> - Add rte_eth_dev_check_detachable()
>>   The function returns whether a PMD supports detach function.
>>
>> Also, the patch changes scope of rte_eth_dev_allocated() to global.
>> This function will be called by virtual PMDs to support port hotplug.
>> So change scope of the function to global.
>>
>> v8:
>> - Add size parameter to rte_eth_dev_save().
>> - Add missing symbol in version map.
>>   (Thanks to Qiu, Michael and Iremonger, Bernard)
>> v7:
>> - Add pt_driver checking to rte_eth_dev_check_detachable().
>>   (Thanks to Qiu, Michael)
>> v5:
>> - Fix return value of below functions.
>>   rte_eth_dev_get_changed_port().
>>   rte_eth_dev_get_port_by_addr().
>> v4:
>> - Add parameter checking.
>> v3:
>> - Fix if-condition bug while comparing pci addresses.
>> - Add error checking codes.
>> Reported-by: Mark Enright <menrigh@brocade.com>
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_ether/rte_ethdev.c          | 99 +++++++++++++++++++++++++++++++++-
>>  lib/librte_ether/rte_ethdev.h          | 83 ++++++++++++++++++++++++++++
>>  lib/librte_ether/rte_ether_version.map |  6 +++
>>  3 files changed, 187 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>> index 58d8072..3869a96 100644
>> --- a/lib/librte_ether/rte_ethdev.c
>> +++ b/lib/librte_ether/rte_ethdev.c
>> @@ -206,7 +206,7 @@ rte_eth_dev_data_alloc(void)
>>  				RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data));
>>  }
>>  
>> -static struct rte_eth_dev *
>> +struct rte_eth_dev *
>>  rte_eth_dev_allocated(const char *name)
>>  {
>>  	unsigned i;
>> @@ -426,6 +426,103 @@ rte_eth_dev_count(void)
>>  	return (nb_ports);
>>  }
>>  
>> +int
>> +rte_eth_dev_save(struct rte_eth_dev *devs, size_t size)
>> +{
>> +	if ((devs == NULL) ||
>> +	    (size != sizeof(struct rte_eth_dev) * RTE_MAX_ETHPORTS))
>> +		return -EINVAL;
>> +
>> +	/* save current rte_eth_devices */
>> +	memcpy(devs, rte_eth_devices, size);
>> +	return 0;
>> +}
> Why creating a function for a memcpy?

When PCI layer initializes or un-initializes an eth device, a port id
that is actually plugged cannot be know out of eth layer.
But hotplug function needs to return the port id to DPDK application.

This function is used like below.
1. Hotplug function calls this function to save port status.
2. Attach or detach.
3. Hotplug function calls rte_eth_dev_get_changed_port() to know port id
actually plugged.

Above steps are done in rte_dev_attach() and rte_dev_detach().
And these 2 functions are not thread safe, so DPDK application needs to
handle it correctly.

>> +
>> +int
>> +rte_eth_dev_get_changed_port(struct rte_eth_dev *devs, uint8_t *port_id)
>> +{
>> +	if ((devs == NULL) || (port_id == NULL))
>> +		return -EINVAL;
>> +
>> +	/* check which port was attached or detached */
>> +	for (*port_id = 0; *port_id < RTE_MAX_ETHPORTS; (*port_id)++, devs++) {
>> +		if (rte_eth_devices[*port_id].attached ^ devs->attached)
>> +			return 0;
>> +	}
>> +	return -ENODEV;
>> +}
> It seems weird to require this function.
> Functions which are attaching a new port should return the port_id.

Please check above comment.

>> +
>> +int
>> +rte_eth_dev_get_addr_by_port(uint8_t port_id, struct rte_pci_addr *addr)
>> +{
>> +	if (rte_eth_dev_validate_port(port_id, TRACE) == DEV_INVALID)
>> +		return -EINVAL;
>> +
>> +	if (addr == NULL) {
>> +		PMD_DEBUG_TRACE("Null pointer is specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	*addr = rte_eth_devices[port_id].pci_dev->addr;
>> +	return 0;
>> +}
> Again, I'm not sure this function is needed.

PCI layer doesn't know relation between port id and pci address.
Eth layer only knows it. So the function is needed.

>> +
>> +int
>> +rte_eth_dev_get_port_by_addr(struct rte_pci_addr *addr, uint8_t *port_id)
>> +{
>> +	struct rte_pci_addr *tmp;
>> +
>> +	if ((addr == NULL) || (port_id == NULL)) {
>> +		PMD_DEBUG_TRACE("Null pointer is specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (*port_id = 0; *port_id < RTE_MAX_ETHPORTS; (*port_id)++) {
>> +		if (!rte_eth_devices[*port_id].attached)
>> +			continue;
>> +		if (!rte_eth_devices[*port_id].pci_dev)
>> +			continue;
>> +		tmp = &rte_eth_devices[*port_id].pci_dev->addr;
>> +		if (eal_compare_pci_addr(tmp, addr) == 0)
>> +			return 0;
>> +	}
>> +	return -ENODEV;
>> +}
>> +
>> +int
>> +rte_eth_dev_get_name_by_port(uint8_t port_id, char *name)
>> +{
>> +	char *tmp;
>> +
>> +	if (rte_eth_dev_validate_port(port_id, TRACE) == DEV_INVALID)
>> +		return -EINVAL;
>> +
>> +	if (name == NULL) {
>> +		PMD_DEBUG_TRACE("Null pointer is specified\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* shouldn't check 'rte_eth_devices[i].data',
>> +	 * because it might be overwritten by VDEV PMD */
> I don't understand the comment.

Sorry for my English.

rte_eth_devices[i].data is over written by some PMDs like pcap PMD.
Please check following code.

static int
rte_pmd_init_internals()
{
    struct rte_eth_dev_data *data = NULL;

    .....snip.....

    data = rte_zmalloc_socket(name, sizeof(*data), 0, numa_node);

    .....snip.....

    (*eth_dev)->data = data;

    .....snip.....
}

'data' is over written like above, but name is kept. So use it for
comparing.

>> +	tmp = rte_eth_dev_data[port_id].name;
>> +	strncpy(name, tmp, strlen(tmp) + 1);
> tmp seems useless.
> strncpy with strlen should be equivalent to strcpy.

Sure, I will fix it.

>
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_eth_dev_check_detachable(uint8_t port_id)
> Why not "is_detachable" instead of "check_detachable"?

I will change it.

>> +{
>> +	uint32_t drv_flags;
>> +
>> +	if (port_id >= RTE_MAX_ETHPORTS) {
>> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	drv_flags = rte_eth_devices[port_id].driver->pci_drv.drv_flags;
>> +	return !(drv_flags & RTE_PCI_DRV_DETACHABLE);
>> +}
>> +
>>  static int
>>  rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
>>  {
>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
>> index 91d9e86..6651890 100644
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -1616,6 +1616,89 @@ extern struct rte_eth_dev rte_eth_devices[];
>>  extern uint8_t rte_eth_dev_count(void);
>>  
>>  /**
>> + * Function for internal use by port hotplug functions.
>> + * Copies current ethdev structures to the specified pointer.
>> + *
>> + * @param	devs	The pointer to the ethdev structures
>> + * @param	size	The size of ethdev structures
>> + * @return
>> + *   - 0 on success, negative on error
>> + */
>> +extern int rte_eth_dev_save(struct rte_eth_dev *devs, size_t size);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Compare the specified ethdev structures with currents. Then
>> + * if there is a port which status is changed, fill the specified pointer
>> + * with the port id of that port.
>> + * @param	devs	The pointer to the ethdev structures
>> + * @param	port_id	The pointer to the port id
>> + * @return
>> + *   - 0 on success, negative on error
>> + */
>> +extern int rte_eth_dev_get_changed_port(
>> +		struct rte_eth_dev *devs, uint8_t *port_id);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Returns a pci address of a ethdev specified by port identifier.
>> + * @param	port_id
>> + *   The port identifier of the Ethernet device
>> + * @param	addr
>> + *   The pointer to the pci address
>> + * @return
>> + *   - 0 on success, negative on error
>> + */
>> +extern int rte_eth_dev_get_addr_by_port(
>> +		uint8_t port_id, struct rte_pci_addr *addr);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Returns a port identifier of a ethdev specified by pci address.
>> + * @param	addr
>> + *   The pointer to the pci address of the Ethernet device.
>> + * @param	port_id
>> + *   The pointer to the port identifier
>> + * @return
>> + *   - 0 on success, negative on error
>> + */
>> +extern int rte_eth_dev_get_port_by_addr(
>> +		struct rte_pci_addr *addr, uint8_t *port_id);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Returns a unique identifier name of a ethdev specified by port identifier.
>> + * @param	port_id
>> + *   The port identifier.
>> + * @param	name
>> + *  The pointer to the Unique identifier name for each Ethernet device
>> + * @return
>> + *   - 0 on success, negative on error
>> + */
>> +extern int rte_eth_dev_get_name_by_port(uint8_t port_id, char *name);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Check whether or not, a PMD that is handling the ethdev specified by port
>> + * identifier can support detach function.
>> + * @param	port_id
>> + *   The port identifier
>> + * @return
>> + *   - 0 on supporting detach function, negative on not supporting
>> + */
>> +extern int rte_eth_dev_check_detachable(uint8_t port_id);
>> +
>> +/**
>> + * Function for internal use by port hotplug functions.
>> + * Returns a ethdev slot specified by the unique identifier name.
>> + * @param	name
>> + *  The pointer to the Unique identifier name for each Ethernet device
>> + * @return
>> + *   - The pointer to the ethdev slot, on success. NULL on error
>> + */
>> +extern struct rte_eth_dev *rte_eth_dev_allocated(const char *name);
>> +
>> +/**
>>   * Function for internal use by dummy drivers primarily, e.g. ring-based
>>   * driver.
>>   * Allocates a new ethdev slot for an ethernet device and returns the pointer
>> diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
>> index 7316530..fc5dc27 100644
>> --- a/lib/librte_ether/rte_ether_version.map
>> +++ b/lib/librte_ether/rte_ether_version.map
>> @@ -109,6 +109,12 @@ DPDK_2.0 {
>>  	rte_eth_tx_queue_setup;
>>  	rte_eth_xstats_get;
>>  	rte_eth_xstats_reset;
>> +	rte_eth_dev_check_detachable;
>> +	rte_eth_dev_get_name_by_port;
>> +	rte_eth_dev_get_addr_by_port;
>> +	rte_eth_dev_get_port_by_addr;
>> +	rte_eth_dev_get_changed_port;
>> +	rte_eth_dev_save;
>>  
>>  	local: *;
>>  };
>>
>