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 36601A2EDB for ; Fri, 6 Sep 2019 09:30:53 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 506EA1F15B; Fri, 6 Sep 2019 09:30:52 +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 E42531F155 for ; Fri, 6 Sep 2019 09:30:50 +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-us3.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 53071B40057; Fri, 6 Sep 2019 07:30:48 +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; Fri, 6 Sep 2019 08:30:36 +0100 To: Ferruh Yigit , Neil Horman , John McNamara , "Marko Kovacevic" , Thomas Monjalon CC: , Ivan Ilchenko References: <1566915962-5472-1-git-send-email-arybchenko@solarflare.com> <1567519051-28189-1-git-send-email-arybchenko@solarflare.com> <1567519051-28189-3-git-send-email-arybchenko@solarflare.com> <928f04e3-4c03-bdeb-fcd9-b0743ee12c0a@intel.com> From: Andrew Rybchenko Message-ID: <767126d0-3e36-d4d9-1c9f-66a62f95c5f5@solarflare.com> Date: Fri, 6 Sep 2019 10:30:32 +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: <928f04e3-4c03-bdeb-fcd9-b0743ee12c0a@intel.com> 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-24892.003 X-TM-AS-Result: No-10.758400-8.000000-10 X-TMASE-MatchedRID: DuKherWvI/s4Q7K/HmUF53bspjK6JP6qJuDBbd4NSqQNWA0aQ3FdiCGU b2JNxi1qFm9c8s2OWMvSBOyM/pHn2nd1uJvxyvjSbrJ9gVnOsZ1gz2zq3DI3CV99KTJvos9Ioaf A+3xdt/f3fvqqd34PPsktuXc71isfYTXYZm8jWOVOHhVJB+QhXOkIk90rAX8h5DJ1FS+XdBO5Up Wx7YiH+bBIP5ssk1RdwoeVJAYvYhPjYQlvTm22Y4icBKfMHlV8GUqOjzOC7IowCbMdSs+QHvLhB 2SekyqipHMI9S5lkTkxfjSti6FOY13BsNr/lvRt/ccgt/EtX/0vV5f7P0HVDMc322cceBeE8jyS 9yXDuv0kM2V3jz3nR9MptZq9LGYP+E5rRFaVfuWqNnzrkU+2muRjZuXE0WlH1R/ptYWR8C7UZZs o3zLrryNBXu8ljadXAUZWrMnznJrxlOJuQNHlfdm6V9lyTcKhiFhr30/WaYwZSz1vvG+0mh7kND mLpsL2aMZTln0fgKT7oAlJCBIVEvXe67dfbMS+UeavKZUnS5B+jSWdx0YUU4RNXza/PSwJ807t+ VEWpeaJmZ8FliAavzfOthqaB6FuLDC3FGTHI3doUArKobkzYhVuy5YTh3+3w7+XQ3Lk9nlVLuHT GzSDmZdhH+FqHRs7TDHSmp/st804c5PfPWZTtoaztvqcYVZPHAwy0XhreD35lA8aRaIEF6PFjJE Fr+olA9Mriq0CDAgBi3kqJOK62XnN0DN7HnFmlEx0Kb3RVfjXw3mfa87KVI1JlcMUXtqIiHkkHL Heuh5qn8IWNLFvGpi8y9aSKdgsx0zVmF8O40+3b2GudzuFe72El4GP2pwpftwZ3X11IV0= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--10.758400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24892.003 X-MDID: 1567755050-aG6nM5n8S9Ut Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v2 02/54] 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" On 9/4/19 7:40 PM, Ferruh Yigit wrote: > On 9/3/2019 2:56 PM, 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. >> >> Signed-off-by: Ivan Ilchenko >> Signed-off-by: Andrew Rybchenko > <...> > >> @@ -1144,7 +1143,9 @@ struct rte_eth_dev * > The diff output seems not able to detect the function/block which makes it > harder to review, same patch I am getting a diff output like below: > @@ -1144,7 +1143,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, > uint16_t nb_tx_q, > > Can you please either check a newer version of git, as far as I can see you are > using '1.8.3.1', or can you please try by removing/updating '.gitattributes' file? Newer Git version solves the problem. >> */ >> memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf)); >> >> - rte_eth_dev_info_get(port_id, &dev_info); >> + ret = rte_eth_dev_info_get(port_id, &dev_info); >> + if (ret != 0) >> + return ret; > 'dev->data->dev_conf' should be restored from 'orig_conf' on failure, before return. Thanks, will fix in v3. > <...> > >> -void >> +int >> rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info) >> { >> struct rte_eth_dev *dev; >> @@ -2558,7 +2566,7 @@ struct rte_eth_dev * >> */ >> memset(dev_info, 0, sizeof(struct rte_eth_dev_info)); >> >> - RTE_ETH_VALID_PORTID_OR_RET(port_id); >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> >> dev_info->rx_desc_lim = lim; >> @@ -2567,13 +2575,15 @@ struct rte_eth_dev * >> dev_info->min_mtu = RTE_ETHER_MIN_MTU; >> dev_info->max_mtu = UINT16_MAX; >> >> - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get); >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP); >> (*dev->dev_ops->dev_infos_get)(dev, dev_info); >> dev_info->driver_name = dev->device->driver->name; >> dev_info->nb_rx_queues = dev->data->nb_rx_queues; >> dev_info->nb_tx_queues = dev->data->nb_tx_queues; >> >> dev_info->dev_flags = &dev->data->dev_flags; >> + >> + return 0; > Should API use "return eth_err(port_id, ret);" syntax, as other APIs do, > I am not very clear about the 'eth_err()', Thomas can provide better > description/suggestion on this. In this particular case eth_err() is irrelevant. It makes sense when callback is updated to return int and I'll use it there. In any case better description would be useful. > <...> > >> @@ -3100,9 +3118,13 @@ struct rte_eth_dev * >> struct rte_eth_dev_info dev_info; >> struct rte_eth_dev *dev = &rte_eth_devices[port_id]; >> unsigned i; >> + int ret; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> - rte_eth_dev_info_get(port_id, &dev_info); >> + >> + ret = rte_eth_dev_info_get(port_id, &dev_info); >> + if (ret != 0) >> + return ret; > This is 'get_mac_addr_index()' static function, and it should either send > negative value for error, or 'index' value in case of success. So should we be > sure here only negative value sent? Incase 'rte_eth_dev_info_get()' returns > positive value in the future. In fact, I think there is no necessity to check port_id here since it is a static function and port_id is now checked in rte_eth_dev_info_get() anyway. Also, I think it is better to return -1 here since it is bad to mix -errno and -1 return codes. -errno would be useful if caller really takes it into account and have different processing for -ENOENT which makes sense in this case. >> >> for (i = 0; i < dev_info.max_mac_addrs; i++) >> if (memcmp(addr, &dev->data->mac_addrs[i], >> @@ -3233,8 +3255,14 @@ struct rte_eth_dev * >> struct rte_eth_dev_info dev_info; >> struct rte_eth_dev *dev = &rte_eth_devices[port_id]; >> unsigned i; >> + int ret; >> + >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> + >> + ret = rte_eth_dev_info_get(port_id, &dev_info); >> + if (ret != 0) >> + return ret; > Same comment as above, for this 'get_hash_mac_addr_index()' static function. Same as above. Many thanks for review.