Re: [dpdk-dev] [PATCH v5 2/4] net/ixgbe: install ethdev hotplug handler in ixgbe

[Jeff Guo <jia.guo@intel.com>]

hi, ferruh


On 8/25/2018 12:22 AM, Ferruh Yigit wrote:
> On 7/11/2018 12:51 PM, Jeff Guo wrote:
>> This patch aim to enable hotplug detect in ixgbe PMD. Firstly it
>> set the flags RTE_PCI_DRV_INTR_RMV in drv_flags to announce the hotplug
>> ability, and then use rte_eth_dev_event_handler_install to install
>> the hotplug event handler for ethdev. When eal detect the hotplug event,
>> it will call the ethdev callback to process it. If the event is hotplug
>> removal, it will trigger the RTE_ETH_EVENT_INTR_RMV event into ethdev
>> callback to let app process the hotplug for this ethdev.
>>
>> This is an example for other driver, that if any driver support hotplug
>> feature could be use this way to install hotplug handler.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
>> ---
>> v5->v4:
>> no change.
>> ---
>>   drivers/net/ixgbe/ixgbe_ethdev.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>> index 87d2ad0..e7ae9bf 100644
>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>> @@ -1678,6 +1678,9 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
>>   	rte_intr_enable(intr_handle);
>>   	ixgbevf_intr_enable(eth_dev);
>>   
>> +	/* install the dev event handler for ethdev. */
>> +	rte_eth_dev_event_handler_install(eth_dev);
>> +
>>   	PMD_INIT_LOG(DEBUG, "port %d vendorID=0x%x deviceID=0x%x mac.type=%s",
>>   		     eth_dev->data->port_id, pci_dev->id.vendor_id,
>>   		     pci_dev->id.device_id, "ixgbe_mac_82599_vf");
>> @@ -1718,6 +1721,9 @@ eth_ixgbevf_dev_uninit(struct rte_eth_dev *eth_dev)
>>   	rte_intr_callback_unregister(intr_handle,
>>   				     ixgbevf_dev_interrupt_handler, eth_dev);
>>   
>> +	/* uninstall the dev event handler for ethdev. */
>> +	rte_eth_dev_event_handler_uninstall(eth_dev);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1801,7 +1807,7 @@ static int eth_ixgbe_pci_remove(struct rte_pci_device *pci_dev)
>>   static struct rte_pci_driver rte_ixgbe_pmd = {
>>   	.id_table = pci_id_ixgbe_map,
>>   	.drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC |
>> -		     RTE_PCI_DRV_IOVA_AS_VA,
>> +		     RTE_PCI_DRV_IOVA_AS_VA | RTE_PCI_DRV_INTR_RMV,
> Instead of each driver explicitly install/uninstall handler, can it be possible
> to do this in a common code for drivers that report RTE_PCI_DRV_INTR_RMV support?
> With this you may not need helper functions but implement them as static
> functions in common code.

make sense, I think offload the install/uninstall to step into the 
device class helper should be a better option, since we could check if 
the driver support
hotplug by RTE_PCI_DRV_INTR_RMV.

> Also should registered_callback remove eth_dev? (after calling user registered
> callbacks)

The eth_dev should be remove as any other device user space resources, 
the process should be include in the currently user registered callback.

> And what is the relation of RTE_ETH_DEV_REMOVED state, which is to say device
> removed and remove callback?

The state of RTE_ETH_DEV_REMOVED means that ether device have already 
been removed, it should be use to let to check and stop any request from 
the app.
that is device class interface layer event, just manage by the layer and 
app.

> Lastly, do you think can there be cases driver specific actions needs to be
> taken, so should driver provide a callback for removal?

so far, i think the callback should be the same functional for hotplug, 
so the answer is there can be but no needs to taken other specific 
actions, just let it handler by bus layer but not driver.