From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f175.google.com (mail-pd0-f175.google.com [209.85.192.175]) by dpdk.org (Postfix) with ESMTP id E34D6B674 for ; Tue, 17 Feb 2015 07:15:24 +0100 (CET) Received: by pdjp10 with SMTP id p10so41622458pdj.3 for ; Mon, 16 Feb 2015 22:15:24 -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=5rjCWlLvsRo4bWhKmf5qNDvbkKqaGoG3yp94v+VV0uE=; b=QRHZSlwk0J0Ylrfi380kVNPCY+40I+RQsoy6LT8+BKA2jLqYnpPgqtS8kGUORFq/yD yww7OaSAsH2F59aT4ovIQaKUA4uDoICIPrSp9zkP6BRNfSqO0a/dZWSr0xwwoHXpBrDc CvE54v3FzeZ6gl43fSavKrKPpsSXR5Ss9wixDT5LTkwLhcCxVLNC4S2evznDDmzCD/V7 dBMI7eCyyj7x9HW93tOS1Cn1kLNBb5ub8dfPLoFhkA2MDhmeFsVpHM7HNP6NRGmTTpjL jlq23rnSuT+kh24hSbG5f8ajsrsKY/1U3RM0HQS5olAiOCglTrRtSslBq2I3r599dsiC Ll+A== X-Gm-Message-State: ALoCoQk2egITDKZcqnOhk0LYjMgQePyEsAeZAI5gltEKMkWE667Bj7iqV3jDsk3J/iBfCRddCPJ3 X-Received: by 10.70.91.106 with SMTP id cd10mr47277716pdb.48.1424153724252; Mon, 16 Feb 2015 22:15:24 -0800 (PST) Received: from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132]) by mx.google.com with ESMTPSA id w8sm16501369pds.64.2015.02.16.22.15.22 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 16 Feb 2015 22:15:23 -0800 (PST) Message-ID: <54E2DC7A.1070907@igel.co.jp> Date: Tue, 17 Feb 2015 15:15:22 +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-7-git-send-email-mukawa@igel.co.jp> <22631413.IesiHpntxK@xps13> In-Reply-To: <22631413.IesiHpntxK@xps13> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v8 06/14] eal, ethdev: Add a function and function pointers to close ether device 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 06:15:25 -0000 On 2015/02/17 9:56, Thomas Monjalon wrote: > 2015-02-16 13:14, Tetsuya Mukawa: >> The patch adds function pointer to rte_pci_driver and eth_driver >> structure. These function pointers are used when ports are detached. >> Also, the patch adds rte_eth_dev_uninit(). So far, it's not called >> by anywhere, but it will be called when port hotplug function is >> implemented. >> >> v6: >> - Fix rte_eth_dev_uninit() to handle a return value of uninit >> function of PMD. >> v4: >> - Add parameter checking. >> - Change function names. >> >> Signed-off-by: Tetsuya Mukawa >> --- >> lib/librte_eal/common/include/rte_pci.h | 7 +++++ >> lib/librte_ether/rte_ethdev.c | 47 ++++++++++++++++++++++++= +++++++++ >> lib/librte_ether/rte_ethdev.h | 24 +++++++++++++++++ >> 3 files changed, 78 insertions(+) >> >> diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/= common/include/rte_pci.h >> index 4814cd7..87ca4cf 100644 >> --- a/lib/librte_eal/common/include/rte_pci.h >> +++ b/lib/librte_eal/common/include/rte_pci.h >> @@ -189,12 +189,19 @@ struct rte_pci_driver; >> typedef int (pci_devinit_t)(struct rte_pci_driver *, struct rte_pci_d= evice *); >> =20 >> /** >> + * Uninitialisation function for the driver called during hotplugging= =2E >> + */ >> +typedef int (pci_devuninit_t)( >> + struct rte_pci_driver *, struct rte_pci_device *); >> + >> +/** >> * A structure describing a PCI driver. >> */ >> struct rte_pci_driver { >> TAILQ_ENTRY(rte_pci_driver) next; /**< Next in list. */ >> const char *name; /**< Driver name. */ >> pci_devinit_t *devinit; /**< Device init. function. = */ >> + pci_devuninit_t *devuninit; /**< Device uninit function.= */ >> struct rte_pci_id *id_table; /**< ID table, NULL terminat= ed. */ >> uint32_t drv_flags; /**< Flags contolling handli= ng of device. */ >> }; >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethd= ev.c >> index 2463d18..58d8072 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -326,6 +326,52 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv, >> return diag; >> } >> =20 >> +static int >> +rte_eth_dev_uninit(struct rte_pci_driver *pci_drv, >> + struct rte_pci_device *pci_dev) > There's something strange here: this is an uninit of an ethdev but the = parameter > is a pci_dev. rte_eth_dev_init and rte_eth_dev_uninit are called by PCI layer. I guess PCI layer cannot handle eth device or eth driver directly. I guess receiving pci dev in eth layer may be fair. But as you said, pci_drv can be removed. =20 > The driver parameter shouldn't be needed. I will change the function like below. static int rte_eth_dev_uninit(struct rte_pci_device *pci_dev) >> +{ >> + struct eth_driver *eth_drv; >> + struct rte_eth_dev *eth_dev; >> + char ethdev_name[RTE_ETH_NAME_MAX_LEN]; >> + int ret; >> + >> + if ((pci_drv =3D=3D NULL) || (pci_dev =3D=3D NULL)) >> + return -EINVAL; >> + >> + /* Create unique Ethernet device name using PCI address */ >> + snprintf(ethdev_name, RTE_ETH_NAME_MAX_LEN, "%d:%d.%d", >> + pci_dev->addr.bus, pci_dev->addr.devid, >> + pci_dev->addr.function); > You should call a function common to init and uninit to generate > the same unique name. Sure, I will. >> + >> + eth_dev =3D rte_eth_dev_allocated(ethdev_name); >> + if (eth_dev =3D=3D NULL) >> + return -ENODEV; >> + >> + eth_drv =3D (struct eth_driver *)pci_drv; >> + >> + /* Invoke PMD device uninit function */ >> + if (*eth_drv->eth_dev_uninit) { >> + ret =3D (*eth_drv->eth_dev_uninit)(eth_drv, eth_dev); >> + if (ret) >> + return ret; >> + } >> + >> + /* free ether device */ >> + rte_eth_dev_free(eth_dev); >> + >> + /* init user callbacks */ >> + TAILQ_INIT(&(eth_dev->callbacks)); > Please comment more why you are resetting callbacks. > An init in an uninit function seems weird ;) I agree. This code can be removed. (Actually callbacks will be initialized when device is initialized.) >> + >> + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) >> + rte_free(eth_dev->data->dev_private); >> + >> + eth_dev->pci_dev =3D NULL; >> + eth_dev->driver =3D NULL; >> + eth_dev->data =3D NULL; >> + >> + return 0; >> +} >> + >> /** >> * Register an Ethernet [Poll Mode] driver. >> * >> @@ -344,6 +390,7 @@ void >> rte_eth_driver_register(struct eth_driver *eth_drv) >> { >> eth_drv->pci_drv.devinit =3D rte_eth_dev_init; >> + eth_drv->pci_drv.devuninit =3D rte_eth_dev_uninit; >> rte_eal_pci_register(ð_drv->pci_drv); >> } >> =20 >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethd= ev.h >> index fbe7ac1..91d9e86 100644 >> --- a/lib/librte_ether/rte_ethdev.h >> +++ b/lib/librte_ether/rte_ethdev.h >> @@ -1678,6 +1678,27 @@ typedef int (*eth_dev_init_t)(struct eth_driver= *eth_drv, >> =20 >> /** >> * @internal >> + * Finalization function of an Ethernet driver invoked for each match= ing >> + * Ethernet PCI device detected during the PCI closing phase. >> + * >> + * @param eth_drv >> + * The pointer to the [matching] Ethernet driver structure supplied= by >> + * the PMD when it registered itself. >> + * @param eth_dev >> + * The *eth_dev* pointer is the address of the *rte_eth_dev* struct= ure >> + * associated with the matching device and which have been [automat= ically] >> + * allocated in the *rte_eth_devices* array. >> + * @return >> + * - 0: Success, the device is properly finalized by the driver. >> + * In particular, the driver MUST free the *dev_ops* pointer >> + * of the *eth_dev* structure. >> + * - <0: Error code of the device initialization failure. >> + */ >> +typedef int (*eth_dev_uninit_t)(struct eth_driver *eth_drv, >> + struct rte_eth_dev *eth_dev); >> + >> +/** >> + * @internal >> * The structure associated with a PMD Ethernet driver. >> * >> * Each Ethernet driver acts as a PCI driver and is represented by a = generic >> @@ -1687,11 +1708,14 @@ typedef int (*eth_dev_init_t)(struct eth_drive= r *eth_drv, >> * >> * - The *eth_dev_init* function invoked for each matching PCI device= =2E >> * >> + * - The *eth_dev_uninit* function invoked for each matching PCI devi= ce. >> + * >> * - The size of the private data to allocate for each matching devic= e. >> */ >> struct eth_driver { >> struct rte_pci_driver pci_drv; /**< The PMD is also a PCI driver.= */ >> eth_dev_init_t eth_dev_init; /**< Device init function. */ >> + eth_dev_uninit_t eth_dev_uninit; /**< Device uninit function. */ >> unsigned int dev_private_size; /**< Size of device private data. = */ >> }; >> =20 >> >