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 EECE7A0613 for ; Tue, 24 Sep 2019 10:54:29 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B4CDA2C18; Tue, 24 Sep 2019 10:54:28 +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 37F682BB5 for ; Tue, 24 Sep 2019 10:54:27 +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-us5.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 4462B800067; Tue, 24 Sep 2019 08:54:25 +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; Tue, 24 Sep 2019 09:54:19 +0100 To: Ferruh Yigit , Thomas Monjalon CC: References: <1568031190-16510-1-git-send-email-arybchenko@solarflare.com> <1568031190-16510-6-git-send-email-arybchenko@solarflare.com> From: Andrew Rybchenko Message-ID: <11a22a10-ea03-fb02-6c83-eb08a8f4281d@solarflare.com> Date: Tue, 24 Sep 2019 11:54:16 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: 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-24930.003 X-TM-AS-Result: No-10.474600-8.000000-10 X-TMASE-MatchedRID: EMyCvCfVN1GlVsVeMS8PK/ZvT2zYoYOwC/ExpXrHizwda1Vk3RqxOGly s1PDhWLoNG3Uztvy88AzbxRNgA4zXUVPHDxFKiiVthWd7lUQSlUPo0vi0aZfNQl4w4lfxz2cjJ2 ZvLV5Bft1n85W2MiQ4eaugX18VkoiNVuJQ19puLT9sJwUtpEqywZyESFXAljfva4hxzuVF7hAHO g8qEtqyNwZQbxQpGvb1IEfWLIyokhV2C/9+oeSXg97mDMXdNW3mdndBk1gfyVXPwnnY5XL5NX3H uAvaX8yaGAximQIMfSfhMKTVhcb3vpSyLV/GaEmLyz9QvAyHjpM8zV+hbhmA8OtrkhuZC9WFfuD UoPurdqbc2U22TB+JUcIEB3jcSAOffs/oS3Fvp/1WO1NzV/CYF/Pp0rGTp40B4sIhfTjqIHMRUM adz4R8nlBiMWIMKVz0AAj4Lcjr8UjYMsN7MGDGOFgDmzNVVKoJd2n2XoSRFlpPI6NmimM1InCPg yVYrjjTOCZcxBO5Fjdwiy/73zXOu8/qMB6+F5bBxsweNg3EaGFkCkkB0UMNpsoi2XrUn/JyeMtM D9QOgChMIDkR/KfwCIQ5mZ5SqHP0KSH08Z8ahWRVRUQQ/zueNJCex4qFM30GdV16YzjibROmveI Y5dCN5XEU2tssMdo9g0BN6wVCqwaJD08MmEwNos7+hvp0PkoS4W/MRhJ1X4= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--10.474600-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24930.003 X-MDID: 1569315266-s4F-syxAvrKl 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 5/7] ethdev: do nothing if all-multicast mode is applied again 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/24/19 11:36 AM, Ferruh Yigit wrote: > On 9/9/2019 1:13 PM, Andrew Rybchenko wrote: >> Since driver callbacks return status code now, there is no necessity >> to enable or disable all-multicast mode once again if it is already >> successfully enabled or disabled. >> >> Configuration restore at startup tries to ensure that configured >> all-multicast mode is applied and start will return error if it fails. >> >> Also it avoids theoretical cases when already configured all-multicast >> mode is applied once again and fails. In this cases it is unclear >> which value should be reported on get (configured or opposite). >> >> Signed-off-by: Andrew Rybchenko >> --- >> lib/librte_ethdev/rte_ethdev.c | 40 ++++++++++++++++++++-------------- >> 1 file changed, 24 insertions(+), 16 deletions(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index 8115226c91..e1921e8225 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -1416,16 +1416,22 @@ rte_eth_dev_config_restore(struct rte_eth_dev *dev, >> } >> >> /* replay all multicast configuration */ >> - if (rte_eth_allmulticast_get(port_id) == 1) { >> - ret = rte_eth_allmulticast_enable(port_id); >> + /* >> + * use callbacks directly since we don't need port_id check and >> + * would like to bypass the same value set >> + */ >> + if (rte_eth_allmulticast_get(port_id) == 1 && >> + *dev->dev_ops->allmulticast_enable != NULL) { >> + ret = (*dev->dev_ops->allmulticast_enable)(dev); > I am for using the API here, it is more abstract instead of adding the dev_ops > null checks etc. Will there be any downside to use the API? API does not call operation if value matches and it will exactly match here for sure since we just get it and applying once again. Can't say that I like usage callback directly here, but it looks acceptable for me. I've tried to clarify why it is done this way in the comment above. > <...> > >> @@ -1962,16 +1968,17 @@ int >> rte_eth_allmulticast_enable(uint16_t port_id) >> { >> struct rte_eth_dev *dev; >> - uint8_t old_allmulticast; >> - int diag; >> + int diag = 0; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_enable, -ENOTSUP); >> - old_allmulticast = dev->data->all_multicast; >> - diag = (*dev->dev_ops->allmulticast_enable)(dev); >> - dev->data->all_multicast = (diag == 0) ? 1 : old_allmulticast; >> + >> + if (dev->data->all_multicast == 0) { > What about adding this check even before 'allmulticast_enable' check, so if the > multicast is already enabled why bother having dev_ops or not: > > if (dev->data->all_multicast == 1) > return eth_err(port_id, diag); Yes, it is a good idea. If so, similar fix up will be required for promiscuous mode. >> + diag = (*dev->dev_ops->allmulticast_enable)(dev); >> + dev->data->all_multicast = (diag == 0) ? 1 : 0; >> + } >> >> return eth_err(port_id, diag); >> } >> @@ -1980,18 +1987,19 @@ int >> rte_eth_allmulticast_disable(uint16_t port_id) >> { >> struct rte_eth_dev *dev; >> - uint8_t old_allmulticast; >> - int diag; >> + int diag = 0; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->allmulticast_disable, -ENOTSUP); >> - old_allmulticast = dev->data->all_multicast; >> - dev->data->all_multicast = 0; >> - diag = (*dev->dev_ops->allmulticast_disable)(dev); >> - if (diag != 0) >> - dev->data->all_multicast = old_allmulticast; >> + >> + if (dev->data->all_multicast == 1) { > Same comment with above, can we move this check above.. Yes, will fix in the next version as well. Thanks for review. >> + dev->data->all_multicast = 0; >> + diag = (*dev->dev_ops->allmulticast_disable)(dev); >> + if (diag != 0) >> + dev->data->all_multicast = 1; >> + } >> >> return eth_err(port_id, diag); >> } >>