RE: [PATCH v2 1/4] eventdev/timer: add periodic event timer support

["Carrillo, Erik G" <erik.g.carrillo@intel.com>]

Hi Harish,

> -----Original Message-----
> From: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Sent: Wednesday, August 10, 2022 2:07 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>; jerinj@marvell.com
> Cc: pbhagavatula@marvell.com; sthotton@marvell.com; dev@dpdk.org
> Subject: [PATCH v2 1/4] eventdev/timer: add periodic event timer support
> 
> This patch adds support to configure and use periodic event timers in
> software timer adapter.
> 
> The structure ``rte_event_timer_adapter_stats`` is extended by adding a
> new field, ``evtim_drop_count``. This stat represents the number of times an
> event_timer expiry event is dropped by the event timer adapter.
> 
> Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> ---
>  lib/eventdev/rte_event_timer_adapter.c | 86 ++++++++++++++++++-------
> -  lib/eventdev/rte_event_timer_adapter.h |  2 +
>  lib/eventdev/rte_eventdev.c            |  6 +-
>  3 files changed, 67 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/eventdev/rte_event_timer_adapter.c
> b/lib/eventdev/rte_event_timer_adapter.c
> index e0d978d641..0de88dfc0f 100644
> --- a/lib/eventdev/rte_event_timer_adapter.c
> +++ b/lib/eventdev/rte_event_timer_adapter.c
> @@ -53,6 +53,14 @@ static const struct event_timer_adapter_ops
> swtim_ops;  #define EVTIM_SVC_LOG_DBG(...) (void)0  #endif
> 
> +static inline enum rte_timer_type
> +get_event_timer_type(const struct rte_event_timer_adapter *adapter) {

Let's call this function "get_timer_type" since it is selecting a type for an rte_timer.

> +	return (adapter->data->conf.flags &
> +			RTE_EVENT_TIMER_ADAPTER_F_PERIODIC) ?
> +			PERIODICAL : SINGLE;
> +}
> +
>  static int
>  default_port_conf_cb(uint16_t id, uint8_t event_dev_id, uint8_t
> *event_port_id,
>  		     void *conf_arg)
> @@ -195,10 +203,11 @@ rte_event_timer_adapter_create_ext(
>  	adapter->data->conf = *conf;  /* copy conf structure */
> 
>  	/* Query eventdev PMD for timer adapter capabilities and ops */
> -	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> +	ret = dev->dev_ops->timer_adapter_caps_get ?
> +				dev->dev_ops-
> >timer_adapter_caps_get(dev,
>  						   adapter->data->conf.flags,
>  						   &adapter->data->caps,
> -						   &adapter->ops);
> +						   &adapter->ops) : 0;
>  	if (ret < 0) {
>  		rte_errno = -ret;
>  		goto free_memzone;

IMO, this hunk would read better as:

        if (dev->dev_ops->timer_adapter_caps_get) {
                ret = dev->dev_ops->timer_adapter_caps_get(dev,
                                adapter->data->conf.flags, &adapter->data->caps,
                                &adapter->ops);
                if (ret < 0) {
                        rte_errno = -ret;
                        goto free_memzone;
                }
        }

> @@ -348,10 +357,11 @@ rte_event_timer_adapter_lookup(uint16_t
> adapter_id)
>  	dev = &rte_eventdevs[adapter->data->event_dev_id];
> 
>  	/* Query eventdev PMD for timer adapter capabilities and ops */
> -	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> +	ret = dev->dev_ops->timer_adapter_caps_get ?
> +			dev->dev_ops->timer_adapter_caps_get(dev,
>  						   adapter->data->conf.flags,
>  						   &adapter->data->caps,
> -						   &adapter->ops);
> +						   &adapter->ops) : 0;
>  	if (ret < 0) {
>  		rte_errno = EINVAL;
>  		return NULL;

Same comment as above for this hunk... 

Thanks,
Erik