Re: [dpdk-dev] [PATCH v4 1/4] ethdev: Add API to enable device event handler

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

On 7/11/2018 4:49 PM, Andrew Rybchenko wrote:
> On 10.07.2018 15:51, Jeff Guo wrote:
>> Implement a couple of eal device event handler install/uninstall APIs
>> in ethdev, it could let PMDs have chance to manage the eal device event,
>> such as register device event callback, then monitor and process the
>> hotplug event.
>
> I think it is moving in right direction, but my problem with the patch is
> that I don't understand what prevents us to make it even more generic.
> Right now it looks like that PCI driver flag RTE_PCI_DRV_INTR_RMV is
> sufficient and everything else could be done on ethdev layer.
> RTE_PCI_DRV_INTR_RMV  is mapped to RTE_ETH_DEV_INTR_RMV eth
> device flag. The flag may be used as an indication in 
> rte_eth_dev_create()
> to register the callback.
> May be I'm completely wrong above, but if so, I'd like to understand why
> and prefer to see explanations in the patch description.
>

Your acknowledgement is right, and it is make sense to check the reason 
why is the most generic but not other.
i think that let driver to decide if it support the RTE_PCI_DRV_INTR_RMV 
should be fine, that is first.
And second, the place of installer in driver is also fine, each ethdev 
driver install specific callback for each port, and could let
driver have change to manage the status of hotplug for further. So if 
there are no better place in ethdev here for more generic to install it,
that should be fine.

>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v4->v3:
>> change to use eal device event handler install api
>> ---
>>   doc/guides/rel_notes/release_18_08.rst | 12 +++++++
>>   lib/librte_ethdev/rte_ethdev.c         | 59 
>> ++++++++++++++++++++++++++++++++++
>>   lib/librte_ethdev/rte_ethdev_driver.h  | 32 ++++++++++++++++++
>>   3 files changed, 103 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_18_08.rst 
>> b/doc/guides/rel_notes/release_18_08.rst
>> index bc01242..b6482ce 100644
>> --- a/doc/guides/rel_notes/release_18_08.rst
>> +++ b/doc/guides/rel_notes/release_18_08.rst
>> @@ -46,6 +46,18 @@ New Features
>>     Flow API support has been added to CXGBE Poll Mode Driver to offload
>>     flows to Chelsio T5/T6 NICs.
>>   +* **Added eal device event process helper in ethdev.**
>> +
>> +  Implement a couple of eal device event handler install/uninstall 
>> APIs in
>> +  ethdev, these helper could let PMDs have chance to manage the eal 
>> device
>> +  event, such as register device event callback, then monitor and 
>> process
>> +  hotplug event.
>> +
>> +  * ``rte_eth_dev_event_handler_install`` for PMDs use to install 
>> the device
>> +    event handler.
>> +  * ``rte_eth_dev_event_handler_uninstall`` for PMDs use to 
>> uninstall the device
>> +    event handler.
>> +
>>     API Changes
>>   -----------
>> diff --git a/lib/librte_ethdev/rte_ethdev.c 
>> b/lib/librte_ethdev/rte_ethdev.c
>> index a9977df..09ea02d 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -4518,6 +4518,65 @@ rte_eth_devargs_parse(const char *dargs, 
>> struct rte_eth_devargs *eth_da)
>>       return result;
>>   }
>>   +static void __rte_experimental
>> +eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
>> +                 void *arg)
>> +{
>> +    struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg;
>> +
>> +    switch (type) {
>> +    case RTE_DEV_EVENT_REMOVE:
>> +        ethdev_log(INFO, "The device: %s has been removed!\n",
>
> Colon after 'device' above looks strange for me. I'd suggest to remove 
> it.
> If so, similar below for ADD.
>

ok, i am fine.

>> +                device_name);
>> +
>> +        if (!device_name || !eth_dev)
>> +            return;
>> +
>> +        if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV))
>
> It looks like a typo above. Shouldn't it be RTE_ETH_DEV_INTR_RMV?
>

you are right here, it is a typo.

>> +            return;
>> +
>> +        if (!strcmp(device_name, eth_dev->device->name))
>> +            _rte_eth_dev_callback_process(eth_dev,
>> +                              RTE_ETH_EVENT_INTR_RMV,
>> +                              NULL);
>> +        break;
>> +    case RTE_DEV_EVENT_ADD:
>> +        ethdev_log(INFO, "The device: %s has been added!\n",
>> +            device_name);
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +int __rte_experimental
>> +rte_eth_dev_event_handler_install(struct rte_eth_dev *eth_dev)
>> +{
>> +    int result = 0;
>> +
>> +    result = rte_dev_event_callback_register(eth_dev->device->name,
>> +                    eth_dev_event_callback, eth_dev);
>> +    if (result)
>> +        RTE_LOG(ERR, EAL, "device event callback register failed for "
>> +            "device:%s!\n", eth_dev->device->name);
>
> Why ethdev_log() is used above, but here is RTE_LOG with EAL?
> Similar question below.
>

should be align to use ethdev_log.

>> +
>> +    return result;
>> +}
>> +
>> +int __rte_experimental
>> +rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *eth_dev)
>> +{
>> +    int result = 0;
>> +
>> +    result = rte_dev_event_callback_unregister(eth_dev->device->name,
>> +                    eth_dev_event_callback, eth_dev);
>> +    if (result)
>> +        RTE_LOG(ERR, EAL, "device event callback unregister failed for"
>> +            " device:%s!\n", eth_dev->device->name);
>> +
>> +    return result;
>> +}
>> +
>>   RTE_INIT(ethdev_init_log);
>>   static void
>>   ethdev_init_log(void)
>
> <...>