From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Rybchenko Subject: Re: [PATCH] ethdev: decrease log level for successful API Date: Thu, 2 Aug 2018 17:41:58 +0300 Message-ID: <00c35ad3-e063-b14e-ef56-e4308ec83f52@solarflare.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Ferruh Yigit , Thomas Monjalon To: Kevin Traynor , 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) In-Reply-To: <9c85e414-f5d3-2450-7ddb-c954ce737c31@redhat.com> Content-Language: en-US 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 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.