Re: [dpdk-dev] [PATCH v3 07/28] ethdev: Add functions to know which port is attached or detached

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

(2014/12/11 12:35), Qiu, Michael wrote:
> On 12/11/2014 11:12 AM, Tetsuya Mukawa wrote:
>> Hi Michael,
>>
>> (2014/12/09 23:39), Qiu, Michael wrote:
>>> On 2014/12/9 14:32, Tetsuya Mukawa wrote:
>>>> The patch adds rte_eth_dev_save() and rte_eth_dev_get_changed_port().
>>>> rte_eth_dev_save() is used for saving current rte_eth_dev structures.
>>>> rte_eth_dev_get_changed_port() receives the rte_eth_dev structures, then
>>>> compare these with current values to know which port is actually
>>>> attached or detached.
>>>>
>>>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>>>> ---
>>>>  lib/librte_ether/rte_ethdev.c | 21 +++++++++++++++++++++
>>>>  lib/librte_ether/rte_ethdev.h | 21 +++++++++++++++++++++
>>>>  2 files changed, 42 insertions(+)
>>>>
>>>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
>>>> index 51697e1..6a3700e 100644
>>>> --- a/lib/librte_ether/rte_ethdev.c
>>>> +++ b/lib/librte_ether/rte_ethdev.c
>>>> @@ -416,6 +416,27 @@ rte_eth_dev_count(void)
>>>>  	return (nb_ports);
>>>>  }
>>>>  
>>>> +void
>>>> +rte_eth_dev_save(struct rte_eth_dev *devs)
>>>> +{
>>>> +	if (devs == NULL)
>>>> +		return;
>>>> +
>>>> +	/* 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)
>>>> +{
>>>> +	/* 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;
>>> Can we have more than one port changed?
>>> If so, your logic should do little modify.
>> The port hotplug APIs cannot attach or detach multiple port at the same
>> time.
> What I mean is can we first detach one port, then another? If it legal
> to do this, here will always return the port with the min port_id.

It's not legal.
Above 2 functions are implemented to be called by rte_eal_dev_attach().
When rte_eal_dev_attach() is called, rte_eth_dev_save() and
rte_eth_dev_get_changed_port() will be invoked continuously in the function.
But while executing the 2 functions, only one port status will be
changed, because rte_eal_dev_attach() and rte_eal_dev_detach() should be
called one by one.

Thanks,
Tetsuya

>
> Thanks,
> Michael
>> And the APIs are not thread safe. DPDK application should have lock
>> properly if multiple threads call the APIs.
>> Because of this, we don't need to take care of such a case.
>>
>>> Thanks,
>>> Michael
>>>> +	return 1;
>>>> +}
>>>> +
>>>>  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 b329e11..03c8850 100644
>>>> --- a/lib/librte_ether/rte_ethdev.h
>>>> +++ b/lib/librte_ether/rte_ethdev.h
>>>> @@ -1642,6 +1642,27 @@ 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 dummy drivers primarily, e.g. ring-based
>>>>   * driver.
>>>>   * Allocates a new ethdev slot for an ethernet device and returns the pointer
>>