Re: [dpdk-dev] [PATCH 4/5] app/testpmd: move ethdev events registration

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 4/5] app/testpmd: move ethdev events registration
> 
> The callback for ethdev events was registered on port start, so it was missing
> some events.
> 
> It is now registered at the beginning of the main function.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 72b91adf5..6eb416e7a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
> 
>  uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
> +/* Pretty printing of ethdev events */
> +static const char * const eth_event_desc[] = {
> +	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> +	[RTE_ETH_EVENT_INTR_LSC] = "LSC",

How about replacing "LSC" with "interrupt link status change"

> +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> +	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> +	[RTE_ETH_EVENT_IPSEC] = "IPsec",
> +	[RTE_ETH_EVENT_MACSEC] = "MACsec",
> +	[RTE_ETH_EVENT_INTR_RMV] = "device removal",

How about replacing "device removal" with "interrupt device removal"

> +	[RTE_ETH_EVENT_NEW] = "device probed",
> +	[RTE_ETH_EVENT_DESTROY] = "device released",
> +	[RTE_ETH_EVENT_MAX] = NULL,
> +};
> +
>  /*
>   * Display or mask ether events
>   * Default to all events except VF_MBOX @@ -1934,7 +1949,6 @@
> start_port(portid_t pid)
>  	queueid_t qi;
>  	struct rte_port *port;
>  	struct ether_addr mac_addr;
> -	enum rte_eth_event_type event_type;
> 
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -2090,20 +2104,6 @@ start_port(portid_t pid)
>  		need_check_link_status = 1;
>  	}
> 
> -	for (event_type = RTE_ETH_EVENT_UNKNOWN;
> -	     event_type < RTE_ETH_EVENT_MAX;
> -	     event_type++) {
> -		diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
> -						event_type,
> -						eth_event_callback,
> -						NULL);
> -		if (diag) {
> -			printf("Failed to setup even callback for event %d\n",
> -				event_type);
> -			return -1;
> -		}
> -	}
> -
>  	if (need_check_link_status == 1 && !no_link_check)
>  		check_all_ports_link_status(RTE_PORT_ALL);
>  	else if (need_check_link_status == 0)
> @@ -2522,20 +2522,6 @@ static int
>  eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void
> *param,
>  		  void *ret_param)
>  {
> -	static const char * const event_desc[] = {
> -		[RTE_ETH_EVENT_UNKNOWN] = "Unknown",
> -		[RTE_ETH_EVENT_INTR_LSC] = "LSC",
> -		[RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
> -		[RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
> -		[RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
> -		[RTE_ETH_EVENT_IPSEC] = "IPsec",
> -		[RTE_ETH_EVENT_MACSEC] = "MACsec",
> -		[RTE_ETH_EVENT_INTR_RMV] = "device removal",
> -		[RTE_ETH_EVENT_NEW] = "device probed",
> -		[RTE_ETH_EVENT_DESTROY] = "device released",
> -		[RTE_ETH_EVENT_MAX] = NULL,
> -	};
> -
>  	RTE_SET_USED(param);
>  	RTE_SET_USED(ret_param);
> 
> @@ -2545,7 +2531,7 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
>  		fflush(stderr);
>  	} else if (event_print_mask & (UINT32_C(1) << type)) {
>  		printf("\nPort %" PRIu16 ": %s event\n", port_id,
> -			event_desc[type]);
> +			eth_event_desc[type]);
>  		fflush(stdout);
>  	}
> 
> @@ -2564,6 +2550,28 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
>  	return 0;
>  }
> 
> +static int
> +register_eth_event_callback(void)
> +{
> +	int ret;
> +	enum rte_eth_event_type event;
> +
> +	for (event = RTE_ETH_EVENT_UNKNOWN;
> +			event < RTE_ETH_EVENT_MAX; event++) {
> +		ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
> +				event,
> +				eth_event_callback,
> +				NULL);
> +		if (ret != 0) {
> +			TESTPMD_LOG(ERR, "Failed to register callback for "
> +					"%s event\n", eth_event_desc[event]);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* This function is used by the interrupt thread */  static void
> eth_dev_event_callback(const char *device_name, enum rte_dev_event_type
> type, @@ -3048,6 +3056,10 @@ main(int argc, char** argv)
>  		rte_panic("Cannot register log type");
>  	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
> 
> +	ret = register_eth_event_callback();
> +	if (ret != 0)
> +		rte_panic("Cannot register for ethdev events");
> +
>  #ifdef RTE_LIBRTE_PDUMP
>  	/* initialize packet capture framework */
>  	rte_pdump_init(NULL);
> --
> 2.19.0

Regards,

Bernard