From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id C07E9B643 for ; Tue, 17 Feb 2015 02:04:47 +0100 (CET) Received: by mail-wg0-f50.google.com with SMTP id l2so32310094wgh.9 for ; Mon, 16 Feb 2015 17:04:47 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=ZmJrUudV46XiD+MtQ+qvdPP/+3iT07XPVK4K3cvAeDU=; b=Ailk72sh8St+yIAKFReL16nk2W0x1G9aZZLEeqL2dssh1YeMc4ylsZ6S/rHSVuSy3v UkGmS9NPpaJ16+kT3UcQtl+hO+AlJl+ObgBB4Vlop1JnuVybUztew0hCXlGinwJtyDX5 fVvdgGJiIliPNdwJmJk1E9pcDgoGBi3MMi6wAbKT3NJdjigVqLZPwYEV0GnNxKTCXQBh B75SygO/p2kA0ob1w1pfAIBcVGe9J1mOBSHPgiGkTriycw1u5IG0NfNcmNEDi4R+OXm2 wsQmCqr29Szi0p4mNYy/j9r2a6Ca1cQZQ3yt2Z3XU2UoT9DKXXFYX5ABUK6tv7XWqSM+ Pdig== X-Gm-Message-State: ALoCoQl/OF/aYyHKS6inbwFMrQEIgYv2qZHEh2xzTQKj1qyDagRBpVe/Nm42XMQgnkGSNekiRvX+ X-Received: by 10.180.95.162 with SMTP id dl2mr51912208wib.31.1424135087413; Mon, 16 Feb 2015 17:04:47 -0800 (PST) Received: from xps13.localnet (guy78-1-82-235-116-147.fbx.proxad.net. [82.235.116.147]) by mx.google.com with ESMTPSA id eb10sm21911685wib.13.2015.02.16.17.04.46 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Feb 2015 17:04:46 -0800 (PST) From: Thomas Monjalon To: Tetsuya Mukawa Date: Tue, 17 Feb 2015 02:04:14 +0100 Message-ID: <5503545.UueaRkuWFh@xps13> Organization: 6WIND User-Agent: KMail/4.14.4 (Linux/3.18.4-1-ARCH; KDE/4.14.4; x86_64; ; ) In-Reply-To: <1424060073-23484-8-git-send-email-mukawa@igel.co.jp> References: <1423470639-15744-2-git-send-email-mukawa@igel.co.jp> <1424060073-23484-1-git-send-email-mukawa@igel.co.jp> <1424060073-23484-8-git-send-email-mukawa@igel.co.jp> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v8 07/14] ethdev: Add functions that will be used by port hotplug functions X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Feb 2015 01:04:48 -0000 2015-02-16 13:14, Tetsuya Mukawa: > The patch adds following functions. > > - rte_eth_dev_save() > The function is used for saving current rte_eth_dev structures. > - rte_eth_dev_get_changed_port() > The function receives the rte_eth_dev structures, then compare > these with current values to know which port is actually > attached or detached. > - rte_eth_dev_get_addr_by_port() > The function returns a pci address of an ethdev specified by port > identifier. > - rte_eth_dev_get_port_by_addr() > The function returns a port identifier of an ethdev specified by > pci address. > - rte_eth_dev_get_name_by_port() > The function returns a unique identifier name of an ethdev > specified by port identifier. > - Add rte_eth_dev_check_detachable() > The function returns whether a PMD supports detach function. > > Also, the patch changes scope of rte_eth_dev_allocated() to global. > This function will be called by virtual PMDs to support port hotplug. > So change scope of the function to global. > > v8: > - Add size parameter to rte_eth_dev_save(). > - Add missing symbol in version map. > (Thanks to Qiu, Michael and Iremonger, Bernard) > v7: > - Add pt_driver checking to rte_eth_dev_check_detachable(). > (Thanks to Qiu, Michael) > v5: > - Fix return value of below functions. > rte_eth_dev_get_changed_port(). > rte_eth_dev_get_port_by_addr(). > v4: > - Add parameter checking. > v3: > - Fix if-condition bug while comparing pci addresses. > - Add error checking codes. > Reported-by: Mark Enright > > Signed-off-by: Tetsuya Mukawa > --- > lib/librte_ether/rte_ethdev.c | 99 +++++++++++++++++++++++++++++++++- > lib/librte_ether/rte_ethdev.h | 83 ++++++++++++++++++++++++++++ > lib/librte_ether/rte_ether_version.map | 6 +++ > 3 files changed, 187 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c > index 58d8072..3869a96 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -206,7 +206,7 @@ rte_eth_dev_data_alloc(void) > RTE_MAX_ETHPORTS * sizeof(*rte_eth_dev_data)); > } > > -static struct rte_eth_dev * > +struct rte_eth_dev * > rte_eth_dev_allocated(const char *name) > { > unsigned i; > @@ -426,6 +426,103 @@ rte_eth_dev_count(void) > return (nb_ports); > } > > +int > +rte_eth_dev_save(struct rte_eth_dev *devs, size_t size) > +{ > + if ((devs == NULL) || > + (size != sizeof(struct rte_eth_dev) * RTE_MAX_ETHPORTS)) > + return -EINVAL; > + > + /* save current rte_eth_devices */ > + memcpy(devs, rte_eth_devices, size); > + return 0; > +} Why creating a function for a memcpy? > + > +int > +rte_eth_dev_get_changed_port(struct rte_eth_dev *devs, uint8_t *port_id) > +{ > + if ((devs == NULL) || (port_id == NULL)) > + return -EINVAL; > + > + /* check which port was attached or detached */ > + for (*port_id = 0; *port_id < RTE_MAX_ETHPORTS; (*port_id)++, devs++) { > + if (rte_eth_devices[*port_id].attached ^ devs->attached) > + return 0; > + } > + return -ENODEV; > +} It seems weird to require this function. Functions which are attaching a new port should return the port_id. > + > +int > +rte_eth_dev_get_addr_by_port(uint8_t port_id, struct rte_pci_addr *addr) > +{ > + if (rte_eth_dev_validate_port(port_id, TRACE) == DEV_INVALID) > + return -EINVAL; > + > + if (addr == NULL) { > + PMD_DEBUG_TRACE("Null pointer is specified\n"); > + return -EINVAL; > + } > + > + *addr = rte_eth_devices[port_id].pci_dev->addr; > + return 0; > +} Again, I'm not sure this function is needed. > + > +int > +rte_eth_dev_get_port_by_addr(struct rte_pci_addr *addr, uint8_t *port_id) > +{ > + struct rte_pci_addr *tmp; > + > + if ((addr == NULL) || (port_id == NULL)) { > + PMD_DEBUG_TRACE("Null pointer is specified\n"); > + return -EINVAL; > + } > + > + for (*port_id = 0; *port_id < RTE_MAX_ETHPORTS; (*port_id)++) { > + if (!rte_eth_devices[*port_id].attached) > + continue; > + if (!rte_eth_devices[*port_id].pci_dev) > + continue; > + tmp = &rte_eth_devices[*port_id].pci_dev->addr; > + if (eal_compare_pci_addr(tmp, addr) == 0) > + return 0; > + } > + return -ENODEV; > +} > + > +int > +rte_eth_dev_get_name_by_port(uint8_t port_id, char *name) > +{ > + char *tmp; > + > + if (rte_eth_dev_validate_port(port_id, TRACE) == DEV_INVALID) > + return -EINVAL; > + > + if (name == NULL) { > + PMD_DEBUG_TRACE("Null pointer is specified\n"); > + return -EINVAL; > + } > + > + /* shouldn't check 'rte_eth_devices[i].data', > + * because it might be overwritten by VDEV PMD */ I don't understand the comment. > + tmp = rte_eth_dev_data[port_id].name; > + strncpy(name, tmp, strlen(tmp) + 1); tmp seems useless. strncpy with strlen should be equivalent to strcpy. > + return 0; > +} > + > +int > +rte_eth_dev_check_detachable(uint8_t port_id) Why not "is_detachable" instead of "check_detachable"? > +{ > + uint32_t drv_flags; > + > + if (port_id >= RTE_MAX_ETHPORTS) { > + PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); > + return -EINVAL; > + } > + > + drv_flags = rte_eth_devices[port_id].driver->pci_drv.drv_flags; > + return !(drv_flags & RTE_PCI_DRV_DETACHABLE); > +} > + > static int > rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues) > { > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h > index 91d9e86..6651890 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -1616,6 +1616,89 @@ extern struct rte_eth_dev rte_eth_devices[]; > extern uint8_t rte_eth_dev_count(void); > > /** > + * Function for internal use by port hotplug functions. > + * Copies current ethdev structures to the specified pointer. > + * > + * @param devs The pointer to the ethdev structures > + * @param size The size of ethdev structures > + * @return > + * - 0 on success, negative on error > + */ > +extern int rte_eth_dev_save(struct rte_eth_dev *devs, size_t size); > + > +/** > + * Function for internal use by port hotplug functions. > + * Compare the specified ethdev structures with currents. Then > + * if there is a port which status is changed, fill the specified pointer > + * with the port id of that port. > + * @param devs The pointer to the ethdev structures > + * @param port_id The pointer to the port id > + * @return > + * - 0 on success, negative on error > + */ > +extern int rte_eth_dev_get_changed_port( > + struct rte_eth_dev *devs, uint8_t *port_id); > + > +/** > + * Function for internal use by port hotplug functions. > + * Returns a pci address of a ethdev specified by port identifier. > + * @param port_id > + * The port identifier of the Ethernet device > + * @param addr > + * The pointer to the pci address > + * @return > + * - 0 on success, negative on error > + */ > +extern int rte_eth_dev_get_addr_by_port( > + uint8_t port_id, struct rte_pci_addr *addr); > + > +/** > + * Function for internal use by port hotplug functions. > + * Returns a port identifier of a ethdev specified by pci address. > + * @param addr > + * The pointer to the pci address of the Ethernet device. > + * @param port_id > + * The pointer to the port identifier > + * @return > + * - 0 on success, negative on error > + */ > +extern int rte_eth_dev_get_port_by_addr( > + struct rte_pci_addr *addr, uint8_t *port_id); > + > +/** > + * Function for internal use by port hotplug functions. > + * Returns a unique identifier name of a ethdev specified by port identifier. > + * @param port_id > + * The port identifier. > + * @param name > + * The pointer to the Unique identifier name for each Ethernet device > + * @return > + * - 0 on success, negative on error > + */ > +extern int rte_eth_dev_get_name_by_port(uint8_t port_id, char *name); > + > +/** > + * Function for internal use by port hotplug functions. > + * Check whether or not, a PMD that is handling the ethdev specified by port > + * identifier can support detach function. > + * @param port_id > + * The port identifier > + * @return > + * - 0 on supporting detach function, negative on not supporting > + */ > +extern int rte_eth_dev_check_detachable(uint8_t port_id); > + > +/** > + * Function for internal use by port hotplug functions. > + * Returns a ethdev slot specified by the unique identifier name. > + * @param name > + * The pointer to the Unique identifier name for each Ethernet device > + * @return > + * - The pointer to the ethdev slot, on success. NULL on error > + */ > +extern struct rte_eth_dev *rte_eth_dev_allocated(const char *name); > + > +/** > * Function for internal use by dummy drivers primarily, e.g. ring-based > * driver. > * Allocates a new ethdev slot for an ethernet device and returns the pointer > diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map > index 7316530..fc5dc27 100644 > --- a/lib/librte_ether/rte_ether_version.map > +++ b/lib/librte_ether/rte_ether_version.map > @@ -109,6 +109,12 @@ DPDK_2.0 { > rte_eth_tx_queue_setup; > rte_eth_xstats_get; > rte_eth_xstats_reset; > + rte_eth_dev_check_detachable; > + rte_eth_dev_get_name_by_port; > + rte_eth_dev_get_addr_by_port; > + rte_eth_dev_get_port_by_addr; > + rte_eth_dev_get_changed_port; > + rte_eth_dev_save; > > local: *; > }; >