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

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

On 2015/02/10 0:34, Iremonger, Bernard wrote:
>
>> -----Original Message-----
>> From: Tetsuya Mukawa [mailto:mukawa@igel.co.jp]
>> Sent: Monday, February 9, 2015 8:31 AM
>> To: dev@dpdk.org
>> Cc: Iremonger, Bernard; Qiu, Michael; Tetsuya Mukawa
>> Subject: [PATCH v7 07/14] ethdev: Add functions that will be used by port hotplug functions
>>
>> 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.
>>
>> 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 | 109 +++++++++++++++++++++++++++++++++++++++++-
>>  lib/librte_ether/rte_ethdev.h |  80 +++++++++++++++++++++++++++++++
>>  2 files changed, 188 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index 7bed901..14a040a
>> 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,113 @@ rte_eth_dev_count(void)
>>  	return (nb_ports);
>>  }
>>
>> +void
>> +rte_eth_dev_save(struct rte_eth_dev *devs) {
>> +	if (devs == NULL)
>> +		return;
>> +
> Hi Tetsuya,
>
> This function should probably have an input parameter for the size of the devs buffer.
> This input parameter should be used with the memcpy function to ensure that nothing is overwritten.

Hi Bernard,

I am sorry that I forgot to fix issues your suggested.
(I checked the patchwork to submit v7 patches, and I forgot that cover
letter wasn't involved in. So all suggestion to cover letter were not
involved in v7 patches. I am sorry for that.)
I will fix your all suggestions and submit again by this weekend.

Regards,
Tetsuya

> Regards,
>
> Bernard.
>
>> +	/* save current rte_eth_devices */
>> +	memcpy(devs, rte_eth_devices,
>> +			sizeof(struct rte_eth_dev) * RTE_MAX_ETHPORTS); }
>> +
>> +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;
>> +}
>> +
>> +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;
>> +}
>> +
>> +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 */
>> +	tmp = rte_eth_dev_data[port_id].name;
>> +	strncpy(name, tmp, strlen(tmp) + 1);
>> +	return 0;
>> +}
>> +
>> +int
>> +rte_eth_dev_check_detachable(uint8_t port_id) {
>> +	uint32_t drv_flags;
>> +
>> +	if (port_id >= RTE_MAX_ETHPORTS) {
>> +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rte_eth_devices[port_id].dev_type == RTE_ETH_DEV_PHYSICAL) {
>> +		switch (rte_eth_devices[port_id].pci_dev->pt_driver) {
>> +		case RTE_PT_IGB_UIO:
>> +		case RTE_PT_UIO_GENERIC:
>> +			break;
>> +		case RTE_PT_VFIO:
>> +		default:
>> +			return -ENOTSUP;
>> +		}
>> +	}
>> +
>> +	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..9919968 100644
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> @@ -1616,6 +1616,86 @@ 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
>> + */
>> +extern void rte_eth_dev_save(struct rte_eth_dev *devs);
>> +
>> +/**
>> + * 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
>> --
>> 1.9.1