From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 7C0691B4B9 for ; Thu, 2 Aug 2018 16:42:11 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 5B4AE4C0059; Thu, 2 Aug 2018 14:42:09 +0000 (UTC) Received: from [192.168.1.16] (85.187.13.33) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1044.25; Thu, 2 Aug 2018 15:42:03 +0100 To: Kevin Traynor , References: <1533213223-24028-1-git-send-email-ktraynor@redhat.com> <661cb701-42a8-1416-1e78-8fa316f0300a@solarflare.com> <7f50a75f-d585-d8fd-5b31-783a5c4a37bc@solarflare.com> <9c85e414-f5d3-2450-7ddb-c954ce737c31@redhat.com> CC: Ferruh Yigit , Thomas Monjalon From: Andrew Rybchenko Message-ID: <00c35ad3-e063-b14e-ef56-e4308ec83f52@solarflare.com> Date: Thu, 2 Aug 2018 17:41:58 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <9c85e414-f5d3-2450-7ddb-c954ce737c31@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.33] 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-24006.003 X-TM-AS-Result: No-18.137800-8.000000-10 X-TMASE-MatchedRID: X4bcv0S75KkOwH4pD14DsPHkpkyUphL9IaLR+2xKRDKjD9XxfoENjnkD xC8oBMkzMJPHfuZG0eOri0qvV9KSBCxpw1FoxdYV4pdq9sdj8LXyoRTj7qeEaNVTZaI6TuNovph VFiQ+RgJL1JirBB3uqGZUyDrAiJrGOsXhsCoCbSb0hv/rD7WVZETrIRzbtv0Gnp9KgXcu34z13O PkzR0M6UNCyXpg+JXbPIgnFej44f7NypWj1MXWMqJVTu7sjgg1zHmrvxa8UmQGW3hFnC9N1SuF/ AYkobNTbJ3ozcI9XLdS+YBHD/ziqEkSb14OJl/x5gCHftmwEMJzd7C7BtJobvt592eq2xoT9wEv JvmkUrYYZgO/81ts0cdnz6O/QLNgDPIzF4wRfrA5f9Xw/xqKXVkMvWAuahr8+gD2vYtOFhgqtq5 d3cxkNemMD2aAwnjEEGMZXPuu8sapZbq29e2uR1xho2aNa1MawDGQk2UsRgM= X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--18.137800-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24006.003 X-MDID: 1533220930-2Bh1yk1NKf5X Subject: Re: [dpdk-dev] [PATCH] ethdev: decrease log level for successful API 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: , X-List-Received-Date: Thu, 02 Aug 2018 14:42:11 -0000 On 02.08.2018 17:09, Kevin Traynor wrote: > On 08/02/2018 02:52 PM, Andrew Rybchenko wrote: >> On 02.08.2018 16:35, Kevin Traynor wrote: >>> On 08/02/2018 01:59 PM, Andrew Rybchenko wrote: >>>> On 02.08.2018 15:33, Kevin Traynor wrote: >>>>> Change log level of messages from ERR back to DEBUG where >>>>> existing API indicates that the condition is a success. >>>>> >>>>> This means applications using the API in its current form will >>>>> not get new ERR logs. >>>>> >>>>> Fixes: bea1e0c70cfc ("ethdev: convert static log type usage to >>>>> dynamic") >>>>> >>>>> Signed-off-by: Kevin Traynor >>>>> --- >>>>> lib/librte_ethdev/rte_ethdev.c | 12 ++++++------ >>>>> 1 file changed, 6 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c >>>>> b/lib/librte_ethdev/rte_ethdev.c >>>>> index c7ab157..16656d2 100644 >>>>> --- a/lib/librte_ethdev/rte_ethdev.c >>>>> +++ b/lib/librte_ethdev/rte_ethdev.c >>>>> @@ -797,5 +797,5 @@ struct rte_eth_dev * >>>>> if (dev->data->rx_queue_state[rx_queue_id] != >>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>> already started\n", >>>>> rx_queue_id, port_id); >>>>> @@ -824,5 +824,5 @@ struct rte_eth_dev * >>>>> if (dev->data->rx_queue_state[rx_queue_id] == >>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>> already stopped\n", >>>>> rx_queue_id, port_id); >>>>> @@ -857,5 +857,5 @@ struct rte_eth_dev * >>>>> if (dev->data->tx_queue_state[tx_queue_id] != >>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>> already started\n", >>>>> tx_queue_id, port_id); >>>>> @@ -882,5 +882,5 @@ struct rte_eth_dev * >>>>> if (dev->data->tx_queue_state[tx_queue_id] == >>>>> RTE_ETH_QUEUE_STATE_STOPPED) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Queue %"PRIu16" of device with port_id=%"PRIu16" >>>>> already stopped\n", >>>>> tx_queue_id, port_id); >>>>> @@ -1287,5 +1287,5 @@ struct rte_eth_dev * >>>>> if (dev->data->dev_started != 0) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Device with port_id=%"PRIu16" already started\n", >>>>> port_id); >>>>> @@ -1319,5 +1319,5 @@ struct rte_eth_dev * >>>>> if (dev->data->dev_started == 0) { >>>>> - RTE_ETHDEV_LOG(ERR, >>>>> + RTE_ETHDEV_LOG(DEBUG, >>>>> "Device with port_id=%"PRIu16" already stopped\n", >>>>> port_id); >>>> I would suggest to use WARNING here. Yes, it is not an error since >>>> nothing bad has >>>> happened and we handle duplicate stop and duplicate start, >>>> but I think it is bad that (buggy?) application does it. Making it debug >>>> we simply >>>> hide it too much. >>>> >>> I think an application following the documented API is not bad or buggy. >> I've failed to find the place were it is documented that device/queue >> may be stopped/started twice. Could you point out? >> Yes, return value 0 means success, but it is a separate thing. >> > I was commenting directly on the API and it's documentation e.g. below > for dev start. I'm not aware of other documentation specifying how it > can/cannot be called. I would not say so. "0: Success. Ethernet device started" means that function managed to do the job and finally the device is started. Never-mind it is not that important and already paid to much attention. I've included in CC other ethdev maintainers (who should be there from the very beginning). I don't mind if it is acked by other ethdev maintainer and applied. It is definitely not an error as it is now. Thanks. > * @return > * - 0: Success, Ethernet device started. > * - <0: Error code of the driver device start function. > */ > int rte_eth_dev_start(uint16_t port_id); > > int > rte_eth_dev_start(uint16_t port_id) > { > ... > if (dev->data->dev_started != 0) { > RTE_ETHDEV_LOG(DEBUG, > "Device with port_id=%"PRIu16" already started\n", > port_id); > return 0; > } > ... > } > > >>> An application may not maintain some state and always call a stop before >>> a start etc. in line with the API. >>> >>> I don't think an API success condition should be changed from a debug >>> message to anything else. Otherwise, it is trying to flag a warning >>> about the application which is following the API! It and will just spook >>> people when they see a new warning.