[dpdk-dev] [PATCH V17 4/4] app/testpmd: enable device hotplug monitoring

Wu, Jingjing jingjing.wu at intel.com
Mon Apr 2 07:49:18 CEST 2018


> 
> +static int
> +eth_dev_event_callback_register(portid_t port_id)
> +{
> +	int diag;
> +	char *device_name;
> +
> +	/* if port id equal -1, unregister all device event callbacks */
> +	if (port_id == (portid_t)HOT_PLUG_FOR_ALL_DEVICE) {
> +		device_name = NULL;
> +	} else {
> +		device_name = strdup(rte_eth_devices[port_id].device->name);
> +		if (!device_name) {
> +			free(device_name);
If device_name is NULL, no memory allocated, why free?

> +			return -1;
> +		}
> +	}
> +	/* register the dev_event callback */
> +	diag = rte_dev_event_callback_register(device_name,
> +		eth_dev_event_callback, (void *)(intptr_t)port_id);
> +	if (diag) {
> +		printf("Failed to setup dev_event callback\n");
> +		return -1;
> +	}
> +
> +	free(device_name);
> +	return 0;
> +}
> +
> +
> +static int
> +eth_dev_event_callback_unregister(portid_t port_id)
> +{
> +	int diag;
> +	char *device_name;
> +
> +	/* if port id equal -1, unregister all device event callbacks */
> +	if (port_id == (portid_t)HOT_PLUG_FOR_ALL_DEVICE) {
> +		device_name = NULL;
> +	} else {
> +		device_name = strdup(rte_eth_devices[port_id].device->name);
> +		if (!device_name) {
> +			free(device_name);
Same as above.

> +			return -1;
> +		}
> +	}
> +
> +	/* unregister the dev_event callback */
> +	diag = rte_dev_event_callback_unregister(device_name,
> +		eth_dev_event_callback, (void *)(intptr_t)port_id);
> +	if (diag) {
> +		printf("Failed to setup dev_event callback\n");
> +		return -1;
> +	}
> +
> +	free(device_name);
> +	return 0;
> +}
> +
>  void
>  attach_port(char *identifier)
>  {
> @@ -1869,6 +1941,8 @@ attach_port(char *identifier)
>  	if (rte_eth_dev_attach(identifier, &pi))
>  		return;
> 
> +	eth_dev_event_callback_register(pi);
> +
>  	socket_id = (unsigned)rte_eth_dev_socket_id(pi);
>  	/* if socket_id is invalid, set to 0 */
>  	if (check_socket_id(socket_id) < 0)
> @@ -1880,6 +1954,12 @@ attach_port(char *identifier)
> 
>  	ports[pi].port_status = RTE_PORT_STOPPED;
> 
> +	if (hot_plug) {
> +		hotplug_list_add(rte_eth_devices[pi].device,
> +				 rte_eth_devices[pi].data->kdrv);
> +		eth_dev_event_callback_register(pi);
> +	}
> +
>  	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
>  	printf("Done\n");
>  }
> @@ -1906,6 +1986,12 @@ detach_port(portid_t port_id)
> 
>  	nb_ports = rte_eth_dev_count();
> 
> +	if (hot_plug) {
> +		hotplug_list_add(rte_eth_devices[port_id].device,
> +				 rte_eth_devices[port_id].data->kdrv);
> +		eth_dev_event_callback_register(port_id);
> +	}
> +
>  	printf("Port '%s' is detached. Now total ports is %d\n",
>  			name, nb_ports);
>  	printf("Done\n");
> @@ -1916,6 +2002,7 @@ void
>  pmd_test_exit(void)
>  {
>  	portid_t pt_id;
> +	int ret;
> 
>  	if (test_done == 0)
>  		stop_packet_forwarding();
> @@ -1929,6 +2016,19 @@ pmd_test_exit(void)
>  			close_port(pt_id);
>  		}
>  	}
> +

Need to check if hot_plug is enabled?

> +	ret = rte_dev_event_monitor_stop();
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "fail to stop device event monitor.");
> +		return;
> +	}
> +
> +	ret = eth_dev_event_callback_unregister(HOT_PLUG_FOR_ALL_DEVICE);
> +	if (ret) {
> +		RTE_LOG(ERR, EAL, "fail to unregister all event callbacks.");
> +		return;
> +	}


<...>

> +static void
> +add_dev_event_callback(void *arg)
> +{
> +	char *dev_name = (char *)arg;
> +
> +	rte_eal_alarm_cancel(add_dev_event_callback, arg);
> +
> +	if (!in_hotplug_list(dev_name))

Remove "!" in the check

> +		return;
> +
> +	RTE_LOG(ERR, EAL, "add device: %s\n", dev_name);
It is not ERR, please make the log aligned with remove device.

> +	attach_port(dev_name);
> +}
> +

<...>
> +
> +/* This function is used by the interrupt thread */
> +static int
> +eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
> +			     void *arg)
> +{
> +	static const char * const event_desc[] = {
> +		[RTE_DEV_EVENT_ADD] = "add",
> +		[RTE_DEV_EVENT_REMOVE] = "remove",
> +	};
> +	char *dev_name = malloc(strlen(device_name) + 1);
> +
> +	strcpy(dev_name, device_name);
Why not use strdup as above?

> +	if (type >= RTE_DEV_EVENT_MAX) {
> +		fprintf(stderr, "%s called upon invalid event %d\n",
> +			__func__, type);
> +		fflush(stderr);
> +	} else if (event_print_mask & (UINT32_C(1) << type)) {
> +		printf("%s event\n",
> +			event_desc[type]);
> +		fflush(stdout);
> +	}
> +
> +	switch (type) {
> +	case RTE_DEV_EVENT_REMOVE:
> +		if (rte_eal_alarm_set(100000,
> +			rmv_dev_event_callback, arg))
> +			fprintf(stderr,
> +				"Could not set up deferred device removal\n");
> +		break;
> +	case RTE_DEV_EVENT_ADD:
> +		if (rte_eal_alarm_set(100000,
> +			add_dev_event_callback, dev_name))
> +			fprintf(stderr,
> +				"Could not set up deferred device add\n");
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
Always 0, even alarm set fails?


Thanks
Jingjing


More information about the dev mailing list