From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 6A2EE5F6E for ; Wed, 21 Mar 2018 15:20:08 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Mar 2018 07:20:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,340,1517904000"; d="scan'208";a="184742722" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.255.27.148]) ([10.255.27.148]) by orsmga004.jf.intel.com with ESMTP; 21 Mar 2018 07:20:03 -0700 To: Jeff Guo , stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, konstantin.ananyev@intel.com, gaetan.rivet@6wind.com, jingjing.wu@intel.com, thomas@monjalon.net, motih@mellanox.com, harry.van.haaren@intel.com References: <1517314860-8097-3-git-send-email-jia.guo@intel.com> <1521610066-12966-1-git-send-email-jia.guo@intel.com> <1521610066-12966-2-git-send-email-jia.guo@intel.com> Cc: jblunck@infradead.org, shreyansh.jain@nxp.com, dev@dpdk.org, helin.zhang@intel.com From: "Tan, Jianfeng" Message-ID: <6065d4b8-8069-e0ea-b948-cc1e794c5020@intel.com> Date: Wed, 21 Mar 2018 22:20:02 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1521610066-12966-2-git-send-email-jia.guo@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V15 2/5] eal: add uevent pass and process function X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Mar 2018 14:20:09 -0000 On 3/21/2018 1:27 PM, Jeff Guo wrote: > In order to handle the uevent which have been detected from the kernel > side, add uevent process function, let hot plug event to be example to > show uevent mechanism how to pass the uevent and process the uevent. In fact, how to pass the uevent to eal/linux for processing, is already done by last patch, by registering a callback into interrupt thread. In this patch, we are actually showing how to process the uevent, and translate it into RTE_DEV_EVENT_ADD, RTE_DEV_EVENT_DEL, etc. So the title would be something like: eal/linux: translate uevent to dev event > > About uevent passing and processing, add below functions in linux eal > dev layer. FreeBSD not support uevent ,so let it to be void and do not > implement in function. > a.dev_uev_parse > b.dev_uev_receive > c.dev_uev_process We already have dummy rte_dev_event_monitor_start and rte_dev_event_monitor_stop, we don't need to have those dummy internal functions any more. Actually, you did not implement those dummy functions. > > Signed-off-by: Jeff Guo > --- > v15->v14: > remove the uevent type check and any policy from eal, > let it check and management in user's callback. > --- > lib/librte_eal/common/include/rte_dev.h | 17 ++++++ And if you agree with me in the above, we shall not touch this file. Move the definition into the previous patch. > lib/librte_eal/linuxapp/eal/eal_dev.c | 95 ++++++++++++++++++++++++++++++++- > 2 files changed, 111 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h > index d2fcbc9..98ea12b 100644 > --- a/lib/librte_eal/common/include/rte_dev.h > +++ b/lib/librte_eal/common/include/rte_dev.h > @@ -24,6 +24,23 @@ extern "C" { > #include > #include > > +#define RTE_EAL_UEV_MSG_LEN 4096 > +#define RTE_EAL_UEV_MSG_ELEM_LEN 128 Such macro shall be linux uevent specific, so put them in linuxapp folder. > + > +enum rte_dev_state { > + RTE_DEV_UNDEFINED, /**< unknown device state */ > + RTE_DEV_FAULT, /**< device fault or error */ > + RTE_DEV_PARSED, /**< device has been scanned on bus*/ > + RTE_DEV_PROBED, /**< device has been probed driver */ > +}; This enum is not used in this patch series, I do see it's used in the other series. So put the definition there. > + > +enum rte_dev_event_subsystem { > + RTE_DEV_EVENT_SUBSYSTEM_UNKNOWN, I don't see where we use this macro. Seems that we now only implement UIO, so I suppose, we shall set the other cases to this UNKNOWN. > + RTE_DEV_EVENT_SUBSYSTEM_UIO, > + RTE_DEV_EVENT_SUBSYSTEM_VFIO, If we don't support VFIO now, I prefer not defining it now. > + RTE_DEV_EVENT_SUBSYSTEM_MAX > +}; > + > /** > * The device event type. > */ > diff --git a/lib/librte_eal/linuxapp/eal/eal_dev.c b/lib/librte_eal/linuxapp/eal/eal_dev.c > index 9d9e088..2b34e2c 100644 > --- a/lib/librte_eal/linuxapp/eal/eal_dev.c > +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c > @@ -78,9 +78,102 @@ dev_uev_monitor_create(int netlink_fd) > } > > static void > +dev_uev_parse(const char *buf, struct rte_dev_event *event) > +{ > + char action[RTE_EAL_UEV_MSG_ELEM_LEN]; > + char subsystem[RTE_EAL_UEV_MSG_ELEM_LEN]; > + char dev_path[RTE_EAL_UEV_MSG_ELEM_LEN]; > + char pci_slot_name[RTE_EAL_UEV_MSG_ELEM_LEN]; > + int i = 0; > + > + memset(action, 0, RTE_EAL_UEV_MSG_ELEM_LEN); > + memset(subsystem, 0, RTE_EAL_UEV_MSG_ELEM_LEN); > + memset(dev_path, 0, RTE_EAL_UEV_MSG_ELEM_LEN); > + memset(pci_slot_name, 0, RTE_EAL_UEV_MSG_ELEM_LEN); > + Maybe we can put an example here for better understanding. And if this buf can contain multiple events? If yes, the implementation is not correct, we will only record one event; if no, we can simplify it a little bit. > + while (i < RTE_EAL_UEV_MSG_LEN) { > + for (; i < RTE_EAL_UEV_MSG_LEN; i++) { > + if (*buf) > + break; > + buf++; > + } If we pass in the length of the buf, we don't have to skip "\0"? > + /** > + * check device uevent from kernel side, no need to check > + * uevent from udev. > + */ > + if (!strncmp(buf, "libudev", 7)) { Use strcmp is enough. And we actually need to check left length enough for strlen("libudev"). > + buf += 7; > + i += 7; > + return; > + } > + if (!strncmp(buf, "ACTION=", 7)) { > + buf += 7; > + i += 7; > + snprintf(action, sizeof(action), "%s", buf); > + } else if (!strncmp(buf, "DEVPATH=", 8)) { > + buf += 8; > + i += 8; > + snprintf(dev_path, sizeof(dev_path), "%s", buf); > + } else if (!strncmp(buf, "SUBSYSTEM=", 10)) { > + buf += 10; > + i += 10; > + snprintf(subsystem, sizeof(subsystem), "%s", buf); > + } else if (!strncmp(buf, "PCI_SLOT_NAME=", 14)) { > + buf += 14; > + i += 14; > + snprintf(pci_slot_name, sizeof(subsystem), "%s", buf); > + event->devname = pci_slot_name; > + } > + for (; i < RTE_EAL_UEV_MSG_LEN; i++) { > + if (*buf == '\0') > + break; > + buf++; > + } As we already check '\0' in the begin of the loop, we don't need it at the end any more. > + } > + > + if ((!strncmp(subsystem, "uio", 3)) || > + (!strncmp(subsystem, "pci", 3))) > + event->subsystem = RTE_DEV_EVENT_SUBSYSTEM_UIO; > + if (!strncmp(action, "add", 3)) > + event->type = RTE_DEV_EVENT_ADD; > + if (!strncmp(action, "remove", 6)) > + event->type = RTE_DEV_EVENT_REMOVE; > +} > + > +static int > +dev_uev_receive(int fd, struct rte_dev_event *uevent) > +{ > + int ret; > + char buf[RTE_EAL_UEV_MSG_LEN]; > + > + memset(uevent, 0, sizeof(struct rte_dev_event)); > + memset(buf, 0, RTE_EAL_UEV_MSG_LEN); > + > + ret = recv(fd, buf, RTE_EAL_UEV_MSG_LEN - 1, MSG_DONTWAIT); > + if (ret < 0) { > + RTE_LOG(ERR, EAL, > + "Socket read error(%d): %s\n", > + errno, strerror(errno)); > + return -1; > + } else if (ret == 0) > + /* connection closed */ > + return -1; So we are sure how many bytes shall be parsed, we can pass the length into dev_uev_parse(). > + > + dev_uev_parse(buf, uevent); > + > + return 0; > +} > + > +static void > dev_uev_process(__rte_unused void *param) > { > - /* TODO: device uevent processing */ > + struct rte_dev_event uevent; > + > + if (dev_uev_receive(intr_handle.fd, &uevent)) > + return; We don't use uevent->subsystem below, why we have to define it in first place? > + > + if (uevent.devname) > + _rte_dev_callback_process(uevent.devname, uevent.type, NULL); > } > > int __rte_experimental