diff mbox series

[4/5] app/testpmd: move ethdev events registration

Message ID 20181024134111.26771-5-thomas@monjalon.net (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series app/testpmd: improve attach/detach support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Thomas Monjalon Oct. 24, 2018, 1:41 p.m. UTC 
  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(-)

Comments

Iremonger, Bernard Oct. 24, 2018, 3:55 p.m. UTC | #1 
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
Thomas Monjalon Oct. 24, 2018, 7:55 p.m. UTC | #2 
24/10/2018 17:55, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > +/* 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"

When it is printed, "event" is appended.
So I think "interrupt" is a bit too much.
OK for "link state 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"

For same reason, I think "device removal" is enough.
It will be printed as "device removal event".
Iremonger, Bernard Oct. 25, 2018, 8:54 a.m. UTC | #3 
Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 8:55 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org; ophirmu@mellanox.com;
> wisamm@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com
> Subject: Re: [PATCH 4/5] app/testpmd: move ethdev events registration
> 
> 24/10/2018 17:55, Iremonger, Bernard:
> > Hi Thomas,
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > +/* 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"
> 
> When it is printed, "event" is appended.
> So I think "interrupt" is a bit too much.
> OK for "link state change"?

Yes,
 
> > > +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",

Should "interrupt" be dropped from "interrupt reset" too for consistency? 

> > > +	[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"
> 
> For same reason, I think "device removal" is enough.
> It will be printed as "device removal event".
> 
> 
Regards,

Bernard.
Thomas Monjalon Oct. 25, 2018, 8:58 a.m. UTC | #4 
25/10/2018 10:54, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 24/10/2018 17:55, Iremonger, Bernard:
> > > Hi Thomas,
> > >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > +/* 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"
> > 
> > When it is printed, "event" is appended.
> > So I think "interrupt" is a bit too much.
> > OK for "link state change"?
> 
> Yes,
>  
> > > > +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > > +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> 
> Should "interrupt" be dropped from "interrupt reset" too for consistency? 

Yes, you're right.

> > > > +	[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"
> > 
> > For same reason, I think "device removal" is enough.
> > It will be printed as "device removal event".

I will send a v2 today.
Thanks for the review.
diff mbox series

Patch

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",
+	[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,
+};
+
 /*
  * 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);