From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f47.google.com (mail-pa0-f47.google.com [209.85.220.47]) by dpdk.org (Postfix) with ESMTP id 030ACB6D3 for ; Tue, 17 Feb 2015 09:51:01 +0100 (CET) Received: by padfb1 with SMTP id fb1so4805485pad.8 for ; Tue, 17 Feb 2015 00:50:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=+oy2uZU35IJkclZIHvP3cbkF8HhTroeh85NcBwK19pM=; b=Fz3kGcvKDnrFOMw8szL9p4XAA7jhWPw8Mfl7k4NrImNQg9wabUiAHLAi6ThAXmg7uh FpydHjcZgskKLTCndm60lRchBJUXVpyN+YLq2FrCesDr+F1UDRIdRRNFMXAh7R0nblye D1GGkU52YbYHzoGpmjwYe9bZY2NaOQYmOIR42axFbaTBenzUk+bJuWMNdwVL4K8lYLXP 0mdqUSr8mJPISwsHd7k+U0bA12g4v654UsCh+LfxJPoD7zSp+VOMDJXBzkN9F0I9+pZr P9Kn3YrBRhaxVUQ/zlNUPMN9f8oUTrxF4KiAa8yQsqZq/yKT9Ugs/abvELyFKOFrpfrJ wAjQ== X-Gm-Message-State: ALoCoQlXWmtXVqAqOJ2PNe0e+MCuYKbLpMpV8+5zZKSSawJHcUz3c37lA5OxbH9u5ZjiEGl9QCb4 X-Received: by 10.66.141.42 with SMTP id rl10mr47649652pab.124.1424163059361; Tue, 17 Feb 2015 00:50:59 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id di5sm16887810pbc.36.2015.02.17.00.50.57 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 17 Feb 2015 00:50:58 -0800 (PST) Message-ID: <54E300F0.5090208@igel.co.jp> Date: Tue, 17 Feb 2015 17:50:56 +0900 From: Tetsuya Mukawa User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Thomas Monjalon 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> <5503545.UueaRkuWFh@xps13> In-Reply-To: <5503545.UueaRkuWFh@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 08:51:01 -0000 On 2015/02/17 10:04, Thomas Monjalon wrote: > 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_ethd= ev.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)); >> } >> =20 >> -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); >> } >> =20 >> +int >> +rte_eth_dev_save(struct rte_eth_dev *devs, size_t size) >> +{ >> + if ((devs =3D=3D NULL) || >> + (size !=3D 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? When PCI layer initializes or un-initializes an eth device, a port id that is actually plugged cannot be know out of eth layer. But hotplug function needs to return the port id to DPDK application. This function is used like below. 1. Hotplug function calls this function to save port status. 2. Attach or detach. 3. Hotplug function calls rte_eth_dev_get_changed_port() to know port id actually plugged. Above steps are done in rte_dev_attach() and rte_dev_detach(). And these 2 functions are not thread safe, so DPDK application needs to handle it correctly. >> + >> +int >> +rte_eth_dev_get_changed_port(struct rte_eth_dev *devs, uint8_t *port_= id) >> +{ >> + if ((devs =3D=3D NULL) || (port_id =3D=3D NULL)) >> + return -EINVAL; >> + >> + /* check which port was attached or detached */ >> + for (*port_id =3D 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. Please check above comment. >> + >> +int >> +rte_eth_dev_get_addr_by_port(uint8_t port_id, struct rte_pci_addr *ad= dr) >> +{ >> + if (rte_eth_dev_validate_port(port_id, TRACE) =3D=3D DEV_INVALID) >> + return -EINVAL; >> + >> + if (addr =3D=3D NULL) { >> + PMD_DEBUG_TRACE("Null pointer is specified\n"); >> + return -EINVAL; >> + } >> + >> + *addr =3D rte_eth_devices[port_id].pci_dev->addr; >> + return 0; >> +} > Again, I'm not sure this function is needed. PCI layer doesn't know relation between port id and pci address. Eth layer only knows it. So the 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 =3D=3D NULL) || (port_id =3D=3D NULL)) { >> + PMD_DEBUG_TRACE("Null pointer is specified\n"); >> + return -EINVAL; >> + } >> + >> + for (*port_id =3D 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 =3D &rte_eth_devices[*port_id].pci_dev->addr; >> + if (eal_compare_pci_addr(tmp, addr) =3D=3D 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) =3D=3D DEV_INVALID) >> + return -EINVAL; >> + >> + if (name =3D=3D 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. Sorry for my English. rte_eth_devices[i].data is over written by some PMDs like pcap PMD. Please check following code. static int rte_pmd_init_internals() { struct rte_eth_dev_data *data =3D NULL; .....snip..... data =3D rte_zmalloc_socket(name, sizeof(*data), 0, numa_node); .....snip..... (*eth_dev)->data =3D data; .....snip..... } 'data' is over written like above, but name is kept. So use it for comparing. >> + tmp =3D rte_eth_dev_data[port_id].name; >> + strncpy(name, tmp, strlen(tmp) + 1); > tmp seems useless. > strncpy with strlen should be equivalent to strcpy. Sure, I will fix it. > >> + return 0; >> +} >> + >> +int >> +rte_eth_dev_check_detachable(uint8_t port_id) > Why not "is_detachable" instead of "check_detachable"? I will change it. >> +{ >> + uint32_t drv_flags; >> + >> + if (port_id >=3D RTE_MAX_ETHPORTS) { >> + PMD_DEBUG_TRACE("Invalid port_id=3D%d\n", port_id); >> + return -EINVAL; >> + } >> + >> + drv_flags =3D 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_queu= es) >> { >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethd= ev.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); >> =20 >> /** >> + * 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 poi= nter >> + * 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 ide= ntifier. >> + * @param port_id >> + * The port identifier. >> + * @param name >> + * The pointer to the Unique identifier name for each Ethernet devic= e >> + * @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 devic= e >> + * @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-ba= sed >> * 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; >> =20 >> local: *; >> }; >> >