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 303F4A00BE; Tue, 29 Oct 2019 17:53:49 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1A0D31BF2F; Tue, 29 Oct 2019 17:53:48 +0100 (CET) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 124F41BEF7 for ; Tue, 29 Oct 2019 17:53:46 +0100 (CET) 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-us1.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 81453940075; Tue, 29 Oct 2019 16:53:43 +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, 29 Oct 2019 16:53:36 +0000 To: , , , Thomas Monjalon CC: References: <20191029050312.2715-1-pbhagavatula@marvell.com> <20191029153722.4547-1-pbhagavatula@marvell.com> <20191029153722.4547-4-pbhagavatula@marvell.com> From: Andrew Rybchenko Message-ID: <6a48d129-8d68-38d6-f5c3-6c2a0d96e3ce@solarflare.com> Date: Tue, 29 Oct 2019 19:53:31 +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: <20191029153722.4547-4-pbhagavatula@marvell.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-25008.003 X-TM-AS-Result: No-14.497300-8.000000-10 X-TMASE-MatchedRID: C/snMIRQLS3mLzc6AOD8DfHkpkyUphL9Dvc/j9oMIgUcNByoSo036fVl 5vsoSSso4GLMXNDnp2rJADt1ASJDUlmGwsZJV9au224COALmKVeByxVkfd04JFMeFKd6rzCnpTM fRy1HRzk2muOXUVATZi+wKzmu8Az8YlldA0POS1IaPMGCcVm9DviH64jt3FfE4PdcWsl+C/ORq3 VkCbdk9bTBKVBe5KYmPSpMf0H2DY2frFd6kw/dZ+CFYN4xFYD9OYqKF7UrYh5Osq/n2Kz29Grjw a8vt1tRlsm/kMurFbjwUH4UFiyQgP4/PNt5uRwEUyxW4vmvLt13ipg0lIGuKAeLCIX046iBzEVD Gnc+EfJ5QYjFiDClc9AAI+C3I6/F1VsVJN4i0I9oUArKobkzYhVuy5YTh3+3QiO6TS4ko30sWVd l3YcDsJ19tSqG3E03FAhrH5Kra4OseK5NzquFhgcbMHjYNxGhhZApJAdFDDabKItl61J/ybLn+0 Vm71LcGua1AIchQ1B5zdAzex5xZu/SoM3IIt87sIkdWuJXyV5Kw7LG/OW27Vj9RwkWmZUEtiLxX F4SAMEtAJxABXnq/7842lgaQSypUjKAxKI/6Q8LzjE6GIiv3n7cGd19dSFd X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--14.497300-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-25008.003 X-MDID: 1572368024-GZWPzC6F_lCb Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v15 3/7] ethdev: add validation to offloads set by PMD 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 10/29/19 6:37 PM, pbhagavatula@marvell.com wrote: > From: Pavan Nikhilesh > > Some PMDs cannot work when certain offloads are enable/disabled, as a > workaround PMDs auto enable/disable offloads internally and expose it > through dev->data->dev_conf.rxmode.offloads. > > After device specific dev_configure is called compare the requested > offloads to the offloads exposed by the PMD and, if the PMD failed > to enable a given offload then log it and return -EINVAL from > rte_eth_dev_configure, else if the PMD failed to disable a given offload > log and continue with rte_eth_dev_configure. > > Suggested-by: Andrew Rybchenko > Signed-off-by: Pavan Nikhilesh Few minor notes below, many thanks Reviewed-by: Andrew Rybchenko > --- > lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 3f45b9e9c..8c58da91c 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload) > return name; > } > > +static int > +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads, I'm not sure about underscore in function name. May be Thomas or Ferruh can comment. > + uint64_t set_offloads, > + const char *(*f)(uint64_t)) > +{ > + uint64_t offloads_diff = req_offloads ^ set_offloads; > + uint64_t offloads_req_diff, offloads_set_diff; > + uint64_t offload; > + uint8_t err = 0; > + > + /* Check if any offload is advertised but not enabled. */ > + offloads_req_diff = offloads_diff & req_offloads; > + while (offloads_req_diff) { > + offload = 1ULL << __builtin_ctzll(offloads_req_diff); > + offloads_req_diff &= ~offload; > + RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload", Offload name does not include Rx/Tx, so IPV4_CKSUM is identical and it is required to log if it is Rx or Tx offload separately. Sounds like one more parameter. > + port_id, f(offload)); > + err = 1; > + } > + > + if (err) > + return -EINVAL; > + > + /* Chech if any offload couldn't be disabled. */ > + offloads_set_diff = offloads_diff & set_offloads; > + while (offloads_set_diff) { > + offload = 1ULL << __builtin_ctzll(offloads_set_diff); > + offloads_set_diff &= ~offload; > + RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload", > + port_id, f(offload)); > + } Consider to do it in one loop, something like:         int rc = 0;      while (offloads_diff != 0) { offload = 1ULL << __builtin_ctzll(offloads_diff); offloads_diff &= ~offload; if (offload & req_offload) { RTE_ETHDEV_LOG(INFO, "Port %u failed to enable %s %s offload", port_id, f(offload), rxtx); rc = -EINVAL; } else { RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s %s offload", port_id, f(offload), rxtx); }   } return rc; BTW, I'm not sure that -EINVAL is good here, but right now can't suggest anything better. > + > + return 0; > +} > + > int > rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > const struct rte_eth_conf *dev_conf) > @@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > goto rollback; > } > > + /* Validate Rx offloads. */ > + diag = _rte_eth_dev_validate_offloads(port_id, > + dev_conf->rxmode.offloads, > + dev->data->dev_conf.rxmode.offloads, > + rte_eth_dev_rx_offload_name); > + if (diag != 0) { > + rte_eth_dev_rx_queue_config(dev, 0); > + rte_eth_dev_tx_queue_config(dev, 0); May be it is a good moment to make one more rollback label with queues release and avoid duplicating it. > + ret = diag; > + goto rollback; > + } > + > + /* Validate Tx offloads. */ > + diag = _rte_eth_dev_validate_offloads(port_id, > + dev_conf->txmode.offloads, > + dev->data->dev_conf.txmode.offloads, > + rte_eth_dev_tx_offload_name); > + if (diag != 0) { > + rte_eth_dev_rx_queue_config(dev, 0); > + rte_eth_dev_tx_queue_config(dev, 0); > + ret = diag; > + goto rollback; > + } > + > return 0; > > rollback: > -- > 2.17.1 >