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 66997A04E6; Wed, 9 Dec 2020 16:43:01 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A33D7BE77; Wed, 9 Dec 2020 16:42:59 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id C964B98 for ; Wed, 9 Dec 2020 16:42:56 +0100 (CET) IronPort-SDR: S7FmK0EfIGlbAcQpZkm7E+3ViHryMZPHftsJXhe5pHXKpdSLU3C8cXKnjcpWNU5Roxz8KDcz4F jo/UzlCAC3RQ== X-IronPort-AV: E=McAfee;i="6000,8403,9829"; a="174239922" X-IronPort-AV: E=Sophos;i="5.78,405,1599548400"; d="scan'208";a="174239922" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2020 07:42:54 -0800 IronPort-SDR: qNJFTGFXrdBzcZaGbwwWWalh7/0kyUCSwrzgkYbPkBFsBcKMIUgSsgOrA9rsKPW0cevHRAeJjb ybssSm500Pkg== X-IronPort-AV: E=Sophos;i="5.78,405,1599548400"; d="scan'208";a="318329051" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.215.89]) ([10.213.215.89]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Dec 2020 07:42:53 -0800 To: Andrew Boyer Cc: dev@dpdk.org, Alfredo Cardigliano References: <20201203203418.15064-1-aboyer@pensando.io> <20201204201646.51746-10-aboyer@pensando.io> <5febeda3-f733-303c-0a13-6e9f1dfe4306@intel.com> From: Ferruh Yigit Message-ID: <79aebafa-6b07-c546-0356-97b56f28241a@intel.com> Date: Wed, 9 Dec 2020 15:42:49 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 9/9] net/ionic: minor logging fixups 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 12/9/2020 2:45 PM, Andrew Boyer wrote: > > >> On Dec 9, 2020, at 8:47 AM, Ferruh Yigit > > wrote: >> >> On 12/4/2020 8:16 PM, Andrew Boyer wrote: >>> Expose ionic_opcode_to_str() so it can be used for dev cmds, too. >>> Store the device name in struct adapter. >>> Switch to memcpy() to work around gcc false positives. >>> Signed-off-by: Andrew Boyer > >>> --- >>>  drivers/net/ionic/ionic.h         |  1 + >>>  drivers/net/ionic/ionic_dev.c     |  5 +++ >>>  drivers/net/ionic/ionic_dev.h     |  2 + >>>  drivers/net/ionic/ionic_ethdev.c  |  4 +- >>>  drivers/net/ionic/ionic_lif.c     | 68 ++++++++++++++++--------------- >>>  drivers/net/ionic/ionic_mac_api.c |  4 +- >>>  drivers/net/ionic/ionic_main.c    | 32 ++++++++------- >>>  drivers/net/ionic/ionic_rxtx.c    | 41 ++++++++----------- >>>  8 files changed, 84 insertions(+), 73 deletions(-) >> >> <...> >> >>> @@ -1217,12 +1221,11 @@ ionic_lif_notifyq_init(struct ionic_lif *lif) >>> } >>> }; >>>  -IONIC_PRINT(DEBUG, "notifyq_init.index %d", >>> -ctx.cmd.q_init.index); >>> -IONIC_PRINT(DEBUG, "notifyq_init.ring_base 0x%" PRIx64 "", >>> -ctx.cmd.q_init.ring_base); >>> +IONIC_PRINT(DEBUG, "notifyq_init.index %d", q->index); >>> +IONIC_PRINT(DEBUG, "notifyq_init.ring_base %#jx", q->base_pa); >> >> There are lots of similar PRIx64 -> %j change in this patch, >> '%j' specifier is for 'intmax_t' and which seems 64bit storage, so this should >> work with 64 bit variable 'q->base_pa', >> but the variable is explicitly uint64_t why replacing 'PRIx64' usage which is >> correct and more common usage in the DPDK? Why ionic is want to do this in its >> own way, I am not clear of the motivation of these changes really, can you >> please clarify? > > As best I know, I am following the (two different) contribute guidelines pages, > both of which direct submitters to run checkpatch. One of things checkpatch > flags is lines over 80 columns. Many of these lines were over 80 columns or > oddly broken to meet the 80 column limit. > > %j is used in many other places in this PMD - as originally written by Alfredo, > one of your core contributors. If we are allowed to use %j, I want to, since I > much prefer it to the hideous PRIx64. > %j is accepted, that is not an issue. But you are making an active effort to convert PRIx64 -> %j, which is very unnecessary in my opinion. 80 column limit is not for log strings, but even if you are fixing them that is different thing from the PRIx64 -> %j conversion, you can keep PRIx64 and stay in 80 columns, and indeed lots of the cases the column limit seems not an issue at all. Andrew, this is a driver currently marked as 'UNMAINTAINED', I kindly suggest focusing your 70+ functional changes instead of this PRIx64 -> %j syntax changes, but it is all up to you of course. >> <...> >> >>> @@ -1448,8 +1450,9 @@ ionic_lif_set_name(struct ionic_lif *lif) >>> }, >>> }; >>>  -snprintf(ctx.cmd.lif_setattr.name, sizeof(ctx.cmd.lif_setattr.name), >>> -"%d", lif->port_id); >>> +/* FW is responsible for NULL terminating this field */ >>> +memcpy(ctx.cmd.lif_setattr.name, lif->name, >>> +sizeof(ctx.cmd.lif_setattr.name)); >> >> Even though FW may be guaranting the string will be null terminated, won't it >> be nice to provide input as null terminated if this is the expectation? > > No, that is not the expectation. We prefer it to be this way. > It is know that FW will add NULL terminate the string but you "prefer" to provide 'name' without NULL termination. Why? "we prefer it to be this way" is not a good justification, please either change or explain in a logical way.