From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wu, Jingjing" Subject: Re: [PATCH V17 4/4] app/testpmd: enable device hotplug monitoring Date: Mon, 2 Apr 2018 05:49:18 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F810FCF3B8@SHSMSX103.ccr.corp.intel.com> References: <1522063256-3997-5-git-send-email-jia.guo@intel.com> <1522339205-27698-1-git-send-email-jia.guo@intel.com> <1522339205-27698-5-git-send-email-jia.guo@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" To: "Guo, Jia" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "gaetan.rivet@6wind.com" , "thomas@monjalon.net" , "motih@mellanox.com" , "Van Haaren, Harry" , "Tan, Jianfeng" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id B2227727F for ; Mon, 2 Apr 2018 07:49:23 +0200 (CEST) In-Reply-To: <1522339205-27698-5-git-send-email-jia.guo@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" >=20 > +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 =3D=3D (portid_t)HOT_PLUG_FOR_ALL_DEVICE) { > + device_name =3D NULL; > + } else { > + device_name =3D 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 =3D 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 =3D=3D (portid_t)HOT_PLUG_FOR_ALL_DEVICE) { > + device_name =3D NULL; > + } else { > + device_name =3D 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 =3D 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; >=20 > + eth_dev_event_callback_register(pi); > + > socket_id =3D (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) >=20 > ports[pi].port_status =3D RTE_PORT_STOPPED; >=20 > + 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) >=20 > nb_ports =3D rte_eth_dev_count(); >=20 > + 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; >=20 > if (test_done =3D=3D 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 =3D rte_dev_event_monitor_stop(); > + if (ret) { > + RTE_LOG(ERR, EAL, "fail to stop device event monitor."); > + return; > + } > + > + ret =3D 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 =3D (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[] =3D { > + [RTE_DEV_EVENT_ADD] =3D "add", > + [RTE_DEV_EVENT_REMOVE] =3D "remove", > + }; > + char *dev_name =3D malloc(strlen(device_name) + 1); > + > + strcpy(dev_name, device_name); Why not use strdup as above? > + if (type >=3D 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