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 DF78EA0613 for ; Tue, 24 Sep 2019 14:14:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AE4362C28; Tue, 24 Sep 2019 14:14:50 +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 926902C18 for ; Tue, 24 Sep 2019 14:14:48 +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-us4.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 5149F8007A; Tue, 24 Sep 2019 12:14:45 +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 13:14:24 +0100 To: Ferruh Yigit , Igor Russkikh , Pavel Belous , "Ravi Kumar" , Rasesh Mody , Shahed Shaikh , Ajit Khaparde , "Somnath Kotur" , Chas Williams , "Rahul Lakkireddy" , Hemant Agrawal , Sachin Saxena , Wenzhuo Lu , Gagandeep Singh , John Daley , Hyong Youb Kim , Gaetan Rivet , Qi Zhang , Xiao Wang , Beilei Xing , Jingjing Wu , Qiming Yang , Rosen Xu , Konstantin Ananyev , Shijith Thotton , Srisivasubramanian Srinivasan , Matan Azrad , Shahaf Shuler , Yongseok Koh , "Viacheslav Ovsiienko" , Tomasz Duszynski , Liron Himi , Stephen Hemminger , "K. Y. Srinivasan" , Haiyang Zhang , Rastislav Cernay , Jan Remes , Jerin Jacob , Nithin Dabilpuram , Kiran Kumar K , "Keith Wiles" , Maxime Coquelin , Tiwei Bie , Zhihong Wang , "Yong Wang" , Thomas Monjalon CC: , Ivan Ilchenko References: <1568031190-16510-1-git-send-email-arybchenko@solarflare.com> <1568031190-16510-5-git-send-email-arybchenko@solarflare.com> From: Andrew Rybchenko Message-ID: Date: Tue, 24 Sep 2019 15:14:21 +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-14.522400-8.000000-10 X-TMASE-MatchedRID: x2HXvaraFonA46G+uSzVzSLVdThWsHxY69aS+7/zbj+qvcIF1TcLYM7/ vP2uWVRBK/S70kzmfv7rL7s+y7z25R1YpEPWJiyzfzgVmnL/olXYuVu0X/rOkIoij12xHbPuHHu qL5Olwoh2wzMj33WdUyJjw2NZ1SCxVcopuCu3KHxIcJTn2HkqsV+s2/NiFe+MzVgwP7ZMYf9nhi wvctsrMSHK50a30T+hAvSAFl5CIlPwB8MaTaI0ljv/PfChpr20RiPTMMc/Mmlm9YF/kMfubQDCX Bzs8w7jxBf85/MElp+LLLxhTi4SUmGN6M1vhJ4H4h8r8l3l4ebdXhRKGhNdp1QmcY1GQfIzlCv9 qMQmd98Zsk/MbokSI/LexjuMUno075zkgvo5OA5FM72aEhcbjQGZ/+APXW9kxLMTZlZbhch51Gx Wl6MpnvSUoP7/DmBPkJ09j7qenzBOi++CZQcQcmg4D2QV/2zLrogFtKd/P7fHiC0dwb5J71HXxC nNdK1O/YWrejG95X/uovghRtD3lmHIw6FQ9nuevMr1EZWBgiFTWG5FkMj76rhxy1YacWp7xTPN0 i7ijGFN6Pt829oSoP++gjOGfzBm5UcZtwNsCro5f9Xw/xqKXVkMvWAuahr8+gD2vYtOFhirEHfa j14ZyT2eCk5Cvv/8vFW4ug6jevYxXblTYIq7Ckqb41+PifjAGAXtUdBHmlYCVJyTiyyvajlnFTT QUKVFtBmxsq70vedFC/qcloFqQG/7o4/k7tHk X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--14.522400-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24930.003 X-MDID: 1569327287-kPO-0UwzAGtz 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 4/7] ethdev: change allmulticast callbacks to return status 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:27 AM, Ferruh Yigit wrote: > On 9/9/2019 1:13 PM, Andrew Rybchenko wrote: >> From: Ivan Ilchenko >> >> Enabling/disabling of allmulticast mode is not always successful and >> it should be taken into account to be able to handle it properly. >> >> When correct return status is unclear from driver code, -EAGAIN is used. >> >> Signed-off-by: Ivan Ilchenko >> Signed-off-by: Andrew Rybchenko > <...> > >> @@ -1227,23 +1227,27 @@ atl_dev_promiscuous_disable(struct rte_eth_dev *dev) >> return 0; >> } >> >> -static void >> +static int >> atl_dev_allmulticast_enable(struct rte_eth_dev *dev) >> { >> struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> >> hw_atl_rpfl2_accept_all_mc_packets_set(hw, true); >> + >> + return 0; >> } >> >> -static void >> +static int >> atl_dev_allmulticast_disable(struct rte_eth_dev *dev) >> { >> struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> >> if (dev->data->promiscuous == 1) >> - return; /* must remain in all_multicast mode */ >> + return 0; /* must remain in all_multicast mode */ > What about making this change in the API level, so all dev_ops not need to do > the similar check. > Indeed I can see you are already adding this to API in following patches, but we > can document and guarantee this behavior in API level, so driver developers can > know and rely on this behavior, what do you think? > > And this can be a general guideline for all enable/disable APIs. If the feature > already enabled, detect in the API and don't call underlying enable dev_ops, > same for the disable. This saves doing the checks by multiple drivers. I'm a bit confused. It is all-multicast disable callback which check promiscuous mode and it relies on fact that promiscuous mode dominates. However, a driver could still have own settings which are used on promiscuous enable/disable to configure HW consistently. If we're talking about all-multicast mode check here, in general I agree, but there is rte_eth_dev_config_restore() which calls ops function to replay current settings. If driver cares about it on start itself, the check still makes sense. I would prefer to require it from drivers to apply correct settings on startup and remove rte_eth_dev_config_restore() function completely. It would allow to remove such checks from driver callbacks. However, I understand that current solution is generic and still makes sense. > <...> > >> @@ -58,10 +58,10 @@ typedef int (*eth_promiscuous_enable_t)(struct rte_eth_dev *dev); >> typedef int (*eth_promiscuous_disable_t)(struct rte_eth_dev *dev); >> /**< @internal Function used to disable the RX promiscuous mode of an Ethernet device. */ >> >> -typedef void (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev); >> +typedef int (*eth_allmulticast_enable_t)(struct rte_eth_dev *dev); >> /**< @internal Enable the receipt of all multicast packets by an Ethernet device. */ >> >> -typedef void (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev); >> +typedef int (*eth_allmulticast_disable_t)(struct rte_eth_dev *dev); >> /**< @internal Disable the receipt of all multicast packets by an Ethernet device. */ > Can you please document what a driver should return? As done in other patches. Yes, will do in v2.