From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v2 8/8] ethdev: Decouple interrupt handling from PCI device Date: Tue, 20 Dec 2016 18:15:10 +0530 Message-ID: <4eca27f2-de7c-6f56-738a-09000c637a58@nxp.com> References: <1479747322-5774-1-git-send-email-jblunck@infradead.org> <1479747322-5774-8-git-send-email-jblunck@infradead.org> <84460a6f-3574-8da2-3ccc-6f1e23fb50e7@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: , David Marchand To: Jan Blunck Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0072.outbound.protection.outlook.com [104.47.41.72]) by dpdk.org (Postfix) with ESMTP id E1782FB6B for ; Tue, 20 Dec 2016 13:41:35 +0100 (CET) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tuesday 20 December 2016 04:21 PM, Jan Blunck wrote: > On Tue, Nov 22, 2016 at 1:57 PM, Shreyansh Jain wrote: >> On Monday 21 November 2016 10:25 PM, Jan Blunck wrote: >>> >>> The struct rte_intr_handle is an abstraction layer for different types of >>> interrupt mechanisms. It is embedded in the low-level device (e.g. PCI). >>> On allocation of a struct rte_eth_dev a reference to the intr_handle >>> should be stored for devices supporting interrupts. >>> >>> Signed-off-by: Jan Blunck >>> --- >>> lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++-- >>> lib/librte_ether/rte_ethdev.h | 1 + >>> 2 files changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >>> index 4288577..4ecea50 100644 >>> --- a/lib/librte_ether/rte_ethdev.c >>> +++ b/lib/librte_ether/rte_ethdev.c >>> @@ -258,6 +258,7 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv, >>> rte_panic("Cannot allocate memzone for private >>> port data\n"); >>> } >>> eth_dev->pci_dev = pci_dev; >>> + eth_dev->intr_handle = &pci_dev->intr_handle; (#) See below. >>> eth_dev->driver = eth_drv; >>> eth_dev->data->rx_mbuf_alloc_failed = 0; >>> >>> @@ -2543,7 +2544,13 @@ rte_eth_dev_rx_intr_ctl(uint8_t port_id, int epfd, >>> int op, void *data) >>> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >>> >>> dev = &rte_eth_devices[port_id]; >>> - intr_handle = &dev->pci_dev->intr_handle; >>> + >>> + if (!dev->intr_handle) { >>> + RTE_PMD_DEBUG_TRACE("RX Intr handle unset\n"); >>> + return -ENOTSUP; >>> + } >>> + >>> + intr_handle = dev->intr_handle; >>> if (!intr_handle->intr_vec) { >>> RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n"); >>> return -EPERM; >>> @@ -2603,7 +2610,12 @@ rte_eth_dev_rx_intr_ctl_q(uint8_t port_id, uint16_t >>> queue_id, >>> return -EINVAL; >>> } >>> >>> - intr_handle = &dev->pci_dev->intr_handle; >>> + if (!dev->intr_handle) { >>> + RTE_PMD_DEBUG_TRACE("RX Intr handle unset\n"); >>> + return -ENOTSUP; >>> + } >>> + >>> + intr_handle = dev->intr_handle; >>> if (!intr_handle->intr_vec) { >>> RTE_PMD_DEBUG_TRACE("RX Intr vector unset\n"); >>> return -EPERM; >>> @@ -3205,6 +3217,8 @@ rte_eth_copy_pci_info(struct rte_eth_dev *eth_dev, >>> struct rte_pci_device *pci_de >>> return; >>> } >>> >>> + eth_dev->intr_handle = &pci_dev->intr_handle; >>> + >>> eth_dev->data->dev_flags = 0; >>> if (pci_dev->driver->drv_flags & RTE_PCI_DRV_INTR_LSC) >>> eth_dev->data->dev_flags |= RTE_ETH_DEV_INTR_LSC; >>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h >>> index 3adbb2b..f1f656a 100644 >>> --- a/lib/librte_ether/rte_ethdev.h >>> +++ b/lib/librte_ether/rte_ethdev.h >>> @@ -1629,6 +1629,7 @@ struct rte_eth_dev { >>> const struct eth_driver *driver;/**< Driver for this device */ >>> const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD >>> */ >>> struct rte_pci_device *pci_dev; /**< PCI info. supplied by probing >>> */ >>> + struct rte_intr_handle *intr_handle; /**< Device interrupt handle >>> */ >>> /** User application callbacks for NIC interrupts */ >>> struct rte_eth_dev_cb_list link_intr_cbs; >>> /** >>> >> >> Is there another patch which replaces all uses of >> eth_dev->pci_dev->intr_handle with that of eth_dev->intr_handle? >> >> Now that eth_dev has a reference which is initialized as early as probe, we >> should use that rather than pci_dev->intr_handle for PCI PMDs. >> > > I've added this indirection because it is required for the ethdev > function. The drivers shouldn't use the indirection through ethdev to > access the intr_handle because they do have direct access to the > device. This is my mistake - I posted my review at a wrong location. It should have been at (#) above. My intention was that we should use ETH_DEV_INTR_HANDLE like macro for accessing intr_handle. Anyways, I see that you have already posted v3. I will review that. You can consider this comment as retracted from my side. > > >> OR maybe, ETH_DEV_INTR_HANDLE() like macro which you have introduced in >> i40e_ethdev.h. >> >> - >> Shreyansh > - Shreyansh