Re: [dpdk-dev] [PATCH v2 2/4] eal: modify device event callback process func

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/2/18 7:45 AM, Jeff Guo wrote:
> andrew,
>
> On 10/1/2018 5:46 PM, Andrew Rybchenko wrote:
>> On 9/30/18 5:16 PM, Jeff Guo wrote:
>>> This patch modify the device event callback process function name to be
>>> more explicit, change the variable to be const and exposure the API out
>>> from private eal. The bus drivers and eal device would directly use 
>>> this
>>> API to process device event callback.
>>
>> Sorry, but from the description it is unclear what has changed to
>> make it necessary.
>>
>
> the reason is that not only eal device help will use the callback, but 
> also vfio bus will use
>
> the callback to handle hot-unplug. So exposure these API to let vfio 
> call it. I will add detail
>
> more at next version if need.

Yes, it would be good. Thanks.

>
>
>>> Signed-off-by: Jeff Guo<jia.guo@intel.com>
>>> ---
>>> v2->v1:
>>> change the rte_dev_event_callback_prcess from internal to external api
>>> for bus or app usage.
>>> ---
>>>   app/test-pmd/testpmd.c                  |  4 ++--
>>>   lib/librte_eal/bsdapp/eal/eal_dev.c     |  8 ++++++++
>>>   lib/librte_eal/common/eal_common_dev.c  |  5 +++--
>>>   lib/librte_eal/common/eal_private.h     | 12 ------------
>>>   lib/librte_eal/common/include/rte_dev.h | 18 +++++++++++++++++-
>>>   lib/librte_eal/linuxapp/eal/eal_dev.c   |  2 +-
>>>   lib/librte_eal/rte_eal_version.map      |  1 +
>>>   7 files changed, 32 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>>> index bfef483..1313100 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -431,7 +431,7 @@ static void check_all_ports_link_status(uint32_t 
>>> port_mask);
>>>   static int eth_event_callback(portid_t port_id,
>>>                     enum rte_eth_event_type type,
>>>                     void *param, void *ret_param);
>>> -static void eth_dev_event_callback(char *device_name,
>>> +static void eth_dev_event_callback(const char *device_name,
>>>                   enum rte_dev_event_type type,
>>>                   void *param);
>>>   static int eth_dev_event_callback_register(void);
>>> @@ -2249,7 +2249,7 @@ eth_event_callback(portid_t port_id, enum 
>>> rte_eth_event_type type, void *param,
>>>     /* This function is used by the interrupt thread */
>>>   static void
>>> -eth_dev_event_callback(char *device_name, enum rte_dev_event_type 
>>> type,
>>> +eth_dev_event_callback(const char *device_name, enum 
>>> rte_dev_event_type type,
>>>                    __rte_unused void *arg)
>>>   {
>>>       uint16_t port_id;
>>> diff --git a/lib/librte_eal/bsdapp/eal/eal_dev.c 
>>> b/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> index ae1c558..374e737 100644
>>> --- a/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c
>>> @@ -33,3 +33,11 @@ rte_dev_hotplug_handle_disable(void)
>>>       RTE_LOG(ERR, EAL, "Device event is not supported for FreeBSD\n");
>>>       return -1;
>>>   }
>>> +
>>> +void  __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event)
>>> +{
>>> +    RTE_LOG(ERR, EAL, "Device event callback process is not 
>>> supported "
>>> +        "for FreeBSD\n");
>>
>> Consider to avoid split of log message to simplify search using grep.
>>
>
> i think i got your point, but the 80 limitation a  line and the 
> clearly meaning of the log should be the 1 prior to be considerate,
>
> right? It should be considerate but it is inevitable.

It is OK to have line longer than 80 symbols in this case. As far as I 
know checkpatches.sh accepts it and does not generate any errors/warnings.

>>> +}
>>> diff --git a/lib/librte_eal/common/eal_common_dev.c 
>>> b/lib/librte_eal/common/eal_common_dev.c
>>> index 678dbca..2d610a4 100644
>>> --- a/lib/librte_eal/common/eal_common_dev.c
>>> +++ b/lib/librte_eal/common/eal_common_dev.c
>>> @@ -342,8 +342,9 @@ rte_dev_event_callback_unregister(const char 
>>> *device_name,
>>>       return ret;
>>>   }
>>>   -void
>>> -dev_callback_process(char *device_name, enum rte_dev_event_type event)
>>> +void __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event)
>>>   {
>>>       struct dev_event_callback *cb_lst;
>>>   diff --git a/lib/librte_eal/common/eal_private.h 
>>> b/lib/librte_eal/common/eal_private.h
>>> index 637f20d..47e8a33 100644
>>> --- a/lib/librte_eal/common/eal_private.h
>>> +++ b/lib/librte_eal/common/eal_private.h
>>> @@ -259,18 +259,6 @@ struct rte_bus 
>>> *rte_bus_find_by_device_name(const char *str);
>>>   int rte_mp_channel_init(void);
>>>     /**
>>> - * Internal Executes all the user application registered callbacks for
>>> - * the specific device. It is for DPDK internal user only. User
>>> - * application should not call it directly.
>>> - *
>>> - * @param device_name
>>> - *  The device name.
>>> - * @param event
>>> - *  the device event type.
>>> - */
>>> -void dev_callback_process(char *device_name, enum 
>>> rte_dev_event_type event);
>>> -
>>> -/**
>>>    * @internal
>>>    * Parse a device string and store its information in an
>>>    * rte_devargs structure.
>>> diff --git a/lib/librte_eal/common/include/rte_dev.h 
>>> b/lib/librte_eal/common/include/rte_dev.h
>>> index ff580a0..58fab43 100644
>>> --- a/lib/librte_eal/common/include/rte_dev.h
>>> +++ b/lib/librte_eal/common/include/rte_dev.h
>>> @@ -39,7 +39,7 @@ struct rte_dev_event {
>>>       char *devname;            /**< device name */
>>>   };
>>>   -typedef void (*rte_dev_event_cb_fn)(char *device_name,
>>> +typedef void (*rte_dev_event_cb_fn)(const char *device_name,
>>>                       enum rte_dev_event_type event,
>>>                       void *cb_arg);
>>>   @@ -438,6 +438,22 @@ rte_dev_event_callback_unregister(const char 
>>> *device_name,
>>>    * @warning
>>>    * @b EXPERIMENTAL: this API may change without prior notice
>>>    *
>>> + * Executes all the user application registered callbacks for
>>> + * the specific device.
>>> + *
>>> + * @param device_name
>>> + *  The device name.
>>> + * @param event
>>> + *  the device event type.
>>> + */
>>> +void  __rte_experimental
>>> +rte_dev_event_callback_process(const char *device_name,
>>> +                   enum rte_dev_event_type event);
>>> +
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>>> + *
>>>    * Start the device event monitoring.
>>>    *
>>>    * @return
>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c 
>>> b/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> index 9f9e1cf..01e3a04 100644
>>> --- a/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c
>>> @@ -271,7 +271,7 @@ dev_uev_handler(__rte_unused void *param)
>>>                   return;
>>>               }
>>>           }
>>> -        dev_callback_process(uevent.devname, uevent.type);
>>> +        rte_dev_event_callback_process(uevent.devname, uevent.type);
>>>       }
>>>   }
>>>   diff --git a/lib/librte_eal/rte_eal_version.map 
>>> b/lib/librte_eal/rte_eal_version.map
>>> index a3255aa..b96da50 100644
>>> --- a/lib/librte_eal/rte_eal_version.map
>>> +++ b/lib/librte_eal/rte_eal_version.map
>>> @@ -277,6 +277,7 @@ EXPERIMENTAL {
>>>       rte_class_register;
>>>       rte_class_unregister;
>>>       rte_ctrl_thread_create;
>>> +    rte_dev_event_callback_process;
>>>       rte_dev_event_callback_register;
>>>       rte_dev_event_callback_unregister;
>>>       rte_dev_event_monitor_start;
>>