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 EFF0FA0613 for ; Tue, 24 Sep 2019 13:50:13 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 055283423; Tue, 24 Sep 2019 13:50:13 +0200 (CEST) Received: from dispatchb-us1.ppe-hosted.com (dispatchb-us1.ppe-hosted.com [148.163.129.53]) by dpdk.org (Postfix) with ESMTP id 9BE3A3237 for ; Tue, 24 Sep 2019 13:50:11 +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 6E71C480072; Tue, 24 Sep 2019 11:50:09 +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 12:50:04 +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> <11a22a10-ea03-fb02-6c83-eb08a8f4281d@solarflare.com> From: Andrew Rybchenko Message-ID: <3da11f44-b618-1b3e-a9b7-a51f050ac16c@solarflare.com> Date: Tue, 24 Sep 2019 14:50:01 +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-11.228100-8.000000-10 X-TMASE-MatchedRID: x2HXvaraFomlVsVeMS8PK/ZvT2zYoYOwC/ExpXrHizwda1Vk3RqxOGly s1PDhWLoNG3Uztvy88AzbxRNgA4zXUVPHDxFKiiVthWd7lUQSlUPo0vi0aZfNQl4w4lfxz2cjJ2 ZvLV5Bft1n85W2MiQ4eaugX18VkoiNVuJQ19puLT9sJwUtpEqywZyESFXAljfva4hxzuVF7hAHO g8qEtqyNwZQbxQpGvb1IEfWLIyokhV2C/9+oeSXg97mDMXdNW3mdndBk1gfyVXPwnnY5XL5NX3H uAvaX8yaGAximQIMfSfhMKTVhcb3vpSyLV/GaEmX2GvVvTo4rUiJN3aXuV/objOUXWmQ3OWJ+9k zW+GmbSVta6x57a42/+IP72gDj7GnTnSdGjEqX1KzOvae5Q0rBfbPFE2GHrVzP9LEqj2Yni46Z2 0fFncG1yXm7O2r0MI3opt/3kOYz4YLyv8+KzdMWg4D2QV/2zLrogFtKd/P7fHiC0dwb5J71HXxC nNdK1O/YWrejG95X/uovghRtD3lmHIw6FQ9nuevMr1EZWBgiFTWG5FkMj76rhxy1YacWp7xTPN0 i7ijGF4XRigqDnz6g6ISk3q8Xjt319rGX8jiaGeAiCmPx4NwLTrdaH1ZWqC1B0Hk1Q1KyLUZxEA lFPo81CtqpPvhRDGv8mPreWmKJgvvXAgK5gQ92lnXnagNEcLnR+rNSVWOr7WQKFDwqd8ZmgrIBM yiviuJnNff0JcGrsjeD1CYZ0mKF3UZPissQMx X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--11.228100-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24930.003 X-MDID: 1569325810-N5yU3mSNBUgL 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 2:03 PM, Ferruh Yigit wrote: > On 9/24/2019 9:54 AM, Andrew Rybchenko wrote: >> 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. > Ah, right, we need it here as you explained. Thx. In fact, I think eth_err() is required to handle callback return value. I'll add it in v2. >> 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. I'd prefer to return 0 directly. >>>> + 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); >>>> } >>>>