From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id F1F381B525 for ; Wed, 11 Jul 2018 10:49:56 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us1.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id E789E940064; Wed, 11 Jul 2018 08:49:54 +0000 (UTC) Received: from [192.168.1.16] (85.187.13.33) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Wed, 11 Jul 2018 09:49:43 +0100 To: Jeff Guo , , , , , , , , , , , , , CC: , , , References: <1530787185-5915-1-git-send-email-jia.guo@intel.com> <1531227098-29564-1-git-send-email-jia.guo@intel.com> <1531227098-29564-2-git-send-email-jia.guo@intel.com> From: Andrew Rybchenko Message-ID: Date: Wed, 11 Jul 2018 11:49:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <1531227098-29564-2-git-send-email-jia.guo@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Originating-IP: [85.187.13.33] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23960.003 X-TM-AS-Result: No--16.461200-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1531298996-HZAsCIJghZRu Subject: Re: [dpdk-dev] [PATCH v4 1/4] ethdev: Add API to enable device event handler 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, 11 Jul 2018 08:49:57 -0000 On 10.07.2018 15:51, Jeff Guo wrote: > Implement a couple of eal device event handler install/uninstall APIs > in ethdev, it could let PMDs have chance to manage the eal device event, > such as register device event callback, then monitor and process the > hotplug event. I think it is moving in right direction, but my problem with the patch is that I don't understand what prevents us to make it even more generic. Right now it looks like that PCI driver flag RTE_PCI_DRV_INTR_RMV is sufficient and everything else could be done on ethdev layer. RTE_PCI_DRV_INTR_RMV  is mapped to RTE_ETH_DEV_INTR_RMV eth device flag. The flag may be used as an indication in rte_eth_dev_create() to register the callback. May be I'm completely wrong above, but if so, I'd like to understand why and prefer to see explanations in the patch description. > Signed-off-by: Jeff Guo > --- > v4->v3: > change to use eal device event handler install api > --- > doc/guides/rel_notes/release_18_08.rst | 12 +++++++ > lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++ > lib/librte_ethdev/rte_ethdev_driver.h | 32 ++++++++++++++++++ > 3 files changed, 103 insertions(+) > > diff --git a/doc/guides/rel_notes/release_18_08.rst b/doc/guides/rel_notes/release_18_08.rst > index bc01242..b6482ce 100644 > --- a/doc/guides/rel_notes/release_18_08.rst > +++ b/doc/guides/rel_notes/release_18_08.rst > @@ -46,6 +46,18 @@ New Features > Flow API support has been added to CXGBE Poll Mode Driver to offload > flows to Chelsio T5/T6 NICs. > > +* **Added eal device event process helper in ethdev.** > + > + Implement a couple of eal device event handler install/uninstall APIs in > + ethdev, these helper could let PMDs have chance to manage the eal device > + event, such as register device event callback, then monitor and process > + hotplug event. > + > + * ``rte_eth_dev_event_handler_install`` for PMDs use to install the device > + event handler. > + * ``rte_eth_dev_event_handler_uninstall`` for PMDs use to uninstall the device > + event handler. > + > > API Changes > ----------- > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index a9977df..09ea02d 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -4518,6 +4518,65 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da) > return result; > } > > +static void __rte_experimental > +eth_dev_event_callback(char *device_name, enum rte_dev_event_type type, > + void *arg) > +{ > + struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg; > + > + switch (type) { > + case RTE_DEV_EVENT_REMOVE: > + ethdev_log(INFO, "The device: %s has been removed!\n", Colon after 'device' above looks strange for me. I'd suggest to remove it. If so, similar below for ADD. > + device_name); > + > + if (!device_name || !eth_dev) > + return; > + > + if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV)) It looks like a typo above. Shouldn't it be RTE_ETH_DEV_INTR_RMV? > + return; > + > + if (!strcmp(device_name, eth_dev->device->name)) > + _rte_eth_dev_callback_process(eth_dev, > + RTE_ETH_EVENT_INTR_RMV, > + NULL); > + break; > + case RTE_DEV_EVENT_ADD: > + ethdev_log(INFO, "The device: %s has been added!\n", > + device_name); > + break; > + default: > + break; > + } > +} > + > +int __rte_experimental > +rte_eth_dev_event_handler_install(struct rte_eth_dev *eth_dev) > +{ > + int result = 0; > + > + result = rte_dev_event_callback_register(eth_dev->device->name, > + eth_dev_event_callback, eth_dev); > + if (result) > + RTE_LOG(ERR, EAL, "device event callback register failed for " > + "device:%s!\n", eth_dev->device->name); Why ethdev_log() is used above, but here is RTE_LOG with EAL? Similar question below. > + > + return result; > +} > + > +int __rte_experimental > +rte_eth_dev_event_handler_uninstall(struct rte_eth_dev *eth_dev) > +{ > + int result = 0; > + > + result = rte_dev_event_callback_unregister(eth_dev->device->name, > + eth_dev_event_callback, eth_dev); > + if (result) > + RTE_LOG(ERR, EAL, "device event callback unregister failed for" > + " device:%s!\n", eth_dev->device->name); > + > + return result; > +} > + > RTE_INIT(ethdev_init_log); > static void > ethdev_init_log(void) <...>