From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 165B5A0613 for ; Wed, 28 Aug 2019 16:40:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AD1A81C19B; Wed, 28 Aug 2019 16:40:08 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 22EBB1C197 for ; Wed, 28 Aug 2019 16:40:07 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us2.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id C36101C008C; Wed, 28 Aug 2019 14:40:04 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Wed, 28 Aug 2019 15:39:57 +0100 To: Jan Viktorin , Thomas Monjalon , Ferruh Yigit CC: Neil Horman , John McNamara , Marko Kovacevic , , Ivan Ilchenko References: <1566915962-5472-2-git-send-email-arybchenko@solarflare.com> <20190828115146.5812afa1@coaster.localdomain> <20190828132638.24193266@coaster.localdomain> From: Andrew Rybchenko Message-ID: Date: Wed, 28 Aug 2019 17:39:53 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190828132638.24193266@coaster.localdomain> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24874.003 X-TM-AS-Result: No-17.596100-8.000000-10 X-TMASE-MatchedRID: B0+RG8xpjtQ4Q7K/HmUF53bspjK6JP6qJuDBbd4NSqT0D0w0EqIArUn0 vjeeb/bZ/ye/3Hc9K1pqQJkq/iEjgnHj330r7L1ggYFDtM3wGBobAqzdFRyxuLytS1u1Z7z6Eg9 tpExnrPFSVjNR3ZVAKHNF+FbeOkYa7FZ5BZP2/uksIMJqMNlanUyQ5fRSh265F6oFlsTPQ1ANYj UOeWxUa4jhray1QAljffLk5O83BXuMcenel/LSesPRlYRmv+e2nKpQna4coUAGmHr1eMxt2WEap scdEYTvW3UnKumFTG4lX1jJyFnRiBmRKwimRosiimHWEC28pk0fqkfNzTRFSmMGm5UqGZgt8wio Ak+lBEvy/Cytt/CdRHoFXyGmQTZNjlL/hujrw1t2GcWKGZufBQ5k1ea+clp6vGAx/1ATZ5uiiaJ YBwWYAGYYFH/ShsdKoa4GM6KoDS0worcxDrbitQlpVkdtt3WuAZn/4A9db2SHv8otQeUIQkvN3o AimcnOTgrXmQhRj4lKIgO2xr3ctUpRLFni5BKBWHGJY8KbuMTGSb7RikwvV8uSXx71bvSLUkRBf e26rAToimozTHrjMqbsFBFWEwjmCuYbw9+K9RUcLuEDP+gqcqIik2/euMx1xzfbZxx4F4QGiSbg FOYq+Qojvy7EVxCl63GD0a4qZ+1w9QIA8zau7YpHR9xEGhE1Xs5nqGvDCfPXLRpcXl5f6Hvibgi OTC8xwjsFCtDiM0+jtX8jKOrIQhp6rGfLEa9iJEFMYGmfGMd9LQinZ4QefL6qvLNjDYTwsuf7RW bvUtx1r8FBbF0I5gtuKBGekqUpIG4YlbCDECvS3Rxy14J4N1KkB6kwjOZ/E40owA+ysAdjqNjCQ xHfm+o7czcnz8j9NkUSDDq742k= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--17.596100-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24874.003 X-MDID: 1567003206-6UjmyRP19q2o Subject: Re: [dpdk-dev] [PATCH 01/51] ethdev: change rte_eth_dev_info_get() return value to int 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" @Thomas, @Ferruh, please, see below. On 8/28/19 2:26 PM, Jan Viktorin wrote: > On Wed, 28 Aug 2019 13:09:51 +0300 > Andrew Rybchenko wrote: > >> On 8/28/19 12:51 PM, Jan Viktorin wrote: >>> On Tue, 27 Aug 2019 15:25:12 +0100 >>> Andrew Rybchenko wrote: >>> >>>> From: Ivan Ilchenko >>>> >>>> Change rte_eth_dev_info_get() return value from void to int and >>>> return negative errno values in case of error conditions. >>>> Modify rte_eth_dev_info_get() usage across the ethdev according >>>> to new return type. >>> Hello Andrew, >>> >>> I didn't find any cover letter describing the true intentions of >>> this patchset. Anyway, see below a short comment... >> The cover letter [1] was sent together with the patch. >> >> [1] http://mails.dpdk.org/archives/dev/2019-August/141593.html > Thanks. So, the goal is "just" to replace void by int. This is what I > was missing... Got it. Will try to improve it. > See below. See below as well. >>>> Signed-off-by: Ivan Ilchenko >>>> Signed-off-by: Andrew Rybchenko >>>> --- >>>> doc/guides/rel_notes/deprecation.rst | 1 - >>>> doc/guides/rel_notes/release_19_11.rst | 5 ++- >>>> lib/librte_ethdev/rte_ethdev.c | 71 >>>> ++++++++++++++++++++++++---------- lib/librte_ethdev/rte_ethdev.h >>>> | 6 ++- 4 files changed, 60 insertions(+), 23 deletions(-) >>> [...] >>> >>>> b/lib/librte_ethdev/rte_ethdev.h index dc6596b..09c278d 100644 >>>> --- a/lib/librte_ethdev/rte_ethdev.h >>>> +++ b/lib/librte_ethdev/rte_ethdev.h >>>> @@ -2366,8 +2366,12 @@ int >>>> rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, >>>> * @param dev_info >>>> * A pointer to a structure of type *rte_eth_dev_info* to be >>>> filled with >>>> * the contextual information of the Ethernet device. >>>> + * @return >>>> + * - (0) if successful. >>>> + * - (-ENOTSUP) if support for dev_infos_get() does not exist >>>> for the device. >>> I believe that allowing PMDs to return -ENOTSUP is not a good idea. >>> At the moment, all PMDs provides this kind of information. It is not >>> always very reliable piece of information but for me, it is a piece >>> of gold I would not like to loose when configuring devices. >>> >>> I think it should be mandatory for all PMDs to provide this >>> function. Another possible way, give a sane default contents of >>> this structure. But, please, do not return -ENOTSUP. >> I agree that dev_infos_get() callback should be mandatory, but >> what the function should do if the callback is not provided? > One way would be to fail to register a PMD without that callback. > Such PMD driver would be simply wrong. This is what I mean by saying > "mandatory" - the callback MUST be provided. Typically callbacks are set on probe and rte_eth_dev_pci_generic_probe() and similar functions could be updated to reject devices with missing dev_infos_get callback. However, there is a secondary process cases where dev_infos_get is not essential since control path may be unsupported in secondary process. Anyway, I think it is a separate story. > DPDK could be so nice to provide a default callback named like > default_dev_infos_get_when_you_are_incompetent_pmd_author() returning > mostly zeros and some always "known metadata" like device pointer, > driver_name, ... Thomas, Ferruh, what do you think? >> I think that a sane default contents is more harm than an error >> (basically that's what we had before the patch). > Well, the dev info API is not in the best possible condition. And I > believe that this is not a secret. Especially, I am missing a kind of > specification of the structure contents (API doc is not satisfactory at > the moment). E.g. what does it mean when dev_info.max_rx_queues == > 65535? Similarly max_rx_pktlen == 65535. IMHO, this is the source of > harm - no spec. Let's return back to the thread topic. I see. > For me, at the application level, I need to get at least identification > of the device - bus name, driver name, device ID. The dev info > structure gives me those. If there is a better way to retrieve these > info by port_id then I have no objections to allow to return -ENOTSUP. > However, the only well-defined way seems to be rte_eth_dev_info_get(). > If a PMD does not give it to me, such PMD becomes simply useless. > > I am currently experiencing 2 different PMDs and both provide slightly > different semantics of the dev info structure. That is bad, of course. > However, I can stand it if I know some info about the device - > as I've already mentioned: ID, driver and bus. > >> Since the function may return error, caller should take it into >> account anyway. Yes, some error codes could have special >> handling, but default error handling is required in any case. >> > You are absolutely right and I support such changes. Thanks, Andrew.