From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 408D4A0C4D; Fri, 13 Aug 2021 11:31:13 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F05F940DF4; Fri, 13 Aug 2021 11:31:12 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 9699C40140 for ; Fri, 13 Aug 2021 11:31:11 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 155F97F53D; Fri, 13 Aug 2021 12:31:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 155F97F53D DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1628847071; bh=JKVtRNBZb4xlcBO1GO8BXgDwbcrCnB+xgablBU9sGTQ=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=xrT2aingUZZedNRchp7Y9dBDEXCkDUzyM8xvBTtnvNYciC3I0QMf8/5v2MMq3IDip imTW8FvUoIb4W6bKVtHsZrqK9XQdm/HMzhmDiY7lS2Fr25PsbB1t4mt0zuNYvq+ZDY QjpTnBDCS5C2WwYy2sfuuaD7jvk7FtZyEfJygG4s= To: "Xia, Chenbo" , Vijay Srivastava , "dev@dpdk.org" Cc: "maxime.coquelin@redhat.com" , Vijay Kumar Srivastava References: <20210706164418.32615-1-vsrivast@xilinx.com> <20210706164418.32615-2-vsrivast@xilinx.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <5f22804e-91f5-e837-c485-34af282f821d@oktetlabs.ru> Date: Fri, 13 Aug 2021 12:31:10 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 8/13/21 12:23 PM, Xia, Chenbo wrote: > Hi Andrew, > >> -----Original Message----- >> From: Andrew Rybchenko >> Sent: Friday, August 13, 2021 4:39 PM >> To: Xia, Chenbo ; Vijay Srivastava >> ; dev@dpdk.org >> Cc: maxime.coquelin@redhat.com; Vijay Kumar Srivastava >> Subject: Re: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver >> >> Hi Chenbo, >> >> many thanks for review. See few questions/notes below. >> >> On 8/11/21 5:26 AM, Xia, Chenbo wrote: >>> Hi Vijay, >>> >>>> -----Original Message----- >>>> From: Vijay Srivastava >>>> Sent: Wednesday, July 7, 2021 12:44 AM >>>> To: dev@dpdk.org >>>> Cc: maxime.coquelin@redhat.com; Xia, Chenbo ; >>>> andrew.rybchenko@oktetlabs.ru; Vijay Kumar Srivastava >>>> Subject: [PATCH 01/10] vdpa/sfc: introduce Xilinx vDPA driver >>>> >>>> From: Vijay Kumar Srivastava >>>> >>>> Add new vDPA PMD to support vDPA operation by Xilinx devices. >>> >>> vDPA operations of Xilinx devices >>> >>>> This patch implements probe and remove functions. >>>> >>>> Signed-off-by: Vijay Kumar Srivastava [snip] >>>> diff --git a/drivers/vdpa/sfc/sfc_vdpa_log.h >> b/drivers/vdpa/sfc/sfc_vdpa_log.h >>>> new file mode 100644 >>>> index 0000000..0a3d6ad >>>> --- /dev/null >>>> +++ b/drivers/vdpa/sfc/sfc_vdpa_log.h >>>> @@ -0,0 +1,77 @@ >>>> +/* SPDX-License-Identifier: BSD-3-Clause >>>> + * >>>> + * Copyright(c) 2020-2021 Xilinx, Inc. >>>> + */ >>>> + >>>> +#ifndef _SFC_VDPA_LOG_H_ >>>> +#define _SFC_VDPA_LOG_H_ >>>> + >>>> +/** Generic driver log type */ >>>> +extern int sfc_vdpa_logtype_driver; >>>> + >>>> +/** Common log type name prefix */ >>>> +#define SFC_VDPA_LOGTYPE_PREFIX "pmd.vdpa.sfc." >>>> + >>>> +/** Log PMD generic message, add a prefix and a line break */ >>>> +#define SFC_VDPA_GENERIC_LOG(level, ...) \ >>>> + rte_log(RTE_LOG_ ## level, sfc_vdpa_logtype_driver, \ >>>> + RTE_FMT("PMD: " RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \ >>>> + RTE_FMT_TAIL(__VA_ARGS__ ,))) >>>> + >>>> +/** Name prefix for the per-device log type used to report basic >> information >>>> */ >>>> +#define SFC_VDPA_LOGTYPE_MAIN_STR SFC_VDPA_LOGTYPE_PREFIX "main" >>>> + >>>> +#define SFC_VDPA_LOG_PREFIX_MAX 32 >>>> + >>>> +/* Log PMD message, automatically add prefix and \n */ >>>> +#define SFC_VDPA_LOG(sva, level, type, ...) \ >>>> + rte_log(level, type, \ >>>> + RTE_FMT("%s" RTE_FMT_HEAD(__VA_ARGS__ ,) "\n", \ >>>> + sva->log_prefix, \ >>>> + RTE_FMT_TAIL(__VA_ARGS__ ,))) >>>> + >>>> +#define sfc_vdpa_err(sva, ...) \ >>>> + do { \ >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \ >>>> + \ >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_ERR, \ >>>> + _sva->logtype_main, __VA_ARGS__); \ >>>> + } while (0) >>>> + >>>> +#define sfc_vdpa_warn(sva, ...) \ >>>> + do { \ >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \ >>>> + \ >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_WARNING, \ >>>> + _sva->logtype_main, __VA_ARGS__); \ >>>> + } while (0) >>>> + >>>> +#define sfc_vdpa_notice(sva, ...) \ >>>> + do { \ >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \ >>>> + \ >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_NOTICE, \ >>>> + _sva->logtype_main, __VA_ARGS__); \ >>>> + } while (0) >>>> + >>>> +#define sfc_vdpa_info(sva, ...) \ >>>> + do { \ >>>> + const struct sfc_vdpa_adapter *_sva = (sva); \ >>>> + \ >>>> + SFC_VDPA_LOG(_sva, RTE_LOG_INFO, \ >>>> + _sva->logtype_main, __VA_ARGS__); \ >>>> + } while (0) >>>> + >>> >>> For above log, can't we make log level a parameter? >>> Then above four define can be one. >> >> Yes, it definitely could, but it is more convenient to have >> dedicated macros for different log levels and corresponding >> lines shorter this way. > > It could save some chars in one log line but also introduce more > LOC. And you may have to change every macro if things like SFC_VDPA_LOG > or naming of sfc_vdpa_adapter change. I prefer combining but since > the duplication is acceptable, I'll let you balance the pros/cons. I see your point. I think it should be a macro sfc_vdpa_log() with log level and other sfc_vpda_*() macros should be just one liner like sfc_vdpa_log(sva, RTE_LOG_INFO, __VA_ARGS__) Do I understand correctly that it address your concerns? Andrew.