From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751670AbcFWFni (ORCPT ); Thu, 23 Jun 2016 01:43:38 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:34460 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751610AbcFWFnf (ORCPT ); Thu, 23 Jun 2016 01:43:35 -0400 Subject: Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics To: Andy Shevchenko , , , References: <1466481399-70080-1-git-send-email-Yisen.Zhuang@huawei.com> <1466481399-70080-11-git-send-email-Yisen.Zhuang@huawei.com> <1466505178.30123.201.camel@linux.intel.com> <5769ED55.3000408@huawei.com> <1466588498.30123.235.camel@linux.intel.com> CC: , , , , , , , , , , , From: Yisen Zhuang Message-ID: <576B76E2.9040402@huawei.com> Date: Thu, 23 Jun 2016 13:42:58 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1466588498.30123.235.camel@linux.intel.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.74.157.38] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090201.576B76F2.0033,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 3de68a1ddef72bb3f5fe072356d4cce1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2016/6/22 17:41, Andy Shevchenko 写道: > On Wed, 2016-06-22 at 09:43 +0800, Yisen Zhuang wrote: >> >> 在 2016/6/21 18:32, Andy Shevchenko 写道: >>> On Tue, 2016-06-21 at 11:56 +0800, Yisen Zhuang wrote: >>>> From: Daode Huang >>>> >>>> For SoC hip06, PFC pause handled in dsaf, while hip05 in XGMAC, >>>> so change the statistics of pfc pause in dsaf and remove the old >>>> pfc pause frame statistics. >>>> >>> >>> >>>> +static char *hns_dsaf_get_node_stats_strings(char *data, int >>>> node, >>>> + struct dsaf_device >>>> *dsaf_dev) >>>> { >>>> char *buff = data; >>>> + int i; >>>> + bool is_ver1 = AE_IS_VER1(dsaf_dev->dsaf_ver); >>>> >>>> snprintf(buff, ETH_GSTRING_LEN, "innod%d_pad_drop_pkts", >>>> node); >>>> buff = buff + ETH_GSTRING_LEN; >>>> @@ -2502,6 +2530,18 @@ static char >>>> *hns_dsaf_get_node_stats_strings(char *data, int node) >>>> buff = buff + ETH_GSTRING_LEN; >>>> snprintf(buff, ETH_GSTRING_LEN, "innod%d_stp_drop_pkts", >>>> node); >>>> buff = buff + ETH_GSTRING_LEN; >>>> + if ((node < DSAF_SERVICE_NW_NUM) && (!is_ver1)) { >>> >>> Redundant parens. >>> >>>> + for (i = 0; i < DSAF_PRIO_NR; i++) { >>>> + snprintf(buff, ETH_GSTRING_LEN, >>>> + "inod%d_pfc_prio%d_pkts", node, >>>> i); >>>> + buff = buff + ETH_GSTRING_LEN; >>> >>> buff += ... >>> >>>> + } >>>> + for (i = 0; i < DSAF_PRIO_NR; i++) { >>>> + snprintf(buff, ETH_GSTRING_LEN, >>>> + "onod%d_pfc_prio%d_pkts", node, >>>> i); >>>> + buff = buff + ETH_GSTRING_LEN; >>> >>> Ditto. >>> >>>> { >>>> u64 *p = data; >>>> + int i; >>>> struct dsaf_hw_stats *hw_stats = &ddev- >>>>> hw_stats[node_num]; >>>> + bool is_ver1 = AE_IS_VER1(ddev->dsaf_ver); >>>> >>>> p[0] = hw_stats->pad_drop; >>>> p[1] = hw_stats->man_pkts; >>>> @@ -2527,8 +2569,16 @@ static u64 *hns_dsaf_get_node_stats(struct >>>> dsaf_device *ddev, u64 *data, >>>> p[10] = hw_stats->local_addr_false; >>>> p[11] = hw_stats->vlan_drop; >>>> p[12] = hw_stats->stp_drop; >>>> - p[13] = hw_stats->tx_pkts; >>>> + if ((node_num < DSAF_SERVICE_NW_NUM) && (!is_ver1)) { >>>> + for (i = 0; i < DSAF_PRIO_NR; i++) { >>>> + p[13 + i] = hw_stats->rx_pfc[i]; >>>> + p[13 + i + DSAF_PRIO_NR] = hw_stats- >>>>> tx_pfc[i]; >>>> + } >>> >>> Two different approaches how to assign data. Above uses 2 for-loops, >>> here you put everything to one. >> >> Above cann't be merged to 1 for-loop, because lenght of the string is >> unknowable. > > It doesn't matter since you are incrementing start position by > constant. > > snprintf(buff, ETH_GSTRING_LEN, "inod%d_pfc_prio%d_pkts", node, i); > snprintf(buff, ETH_GSTRING_LEN, "onod%d_pfc_prio%d_pkts", node, i); > > Same approach as below can be used > > snprintf(buff + 0 * ETH_GSTRING_LEN * DSAF_PRIO_NR, ETH_GSTRING_LEN, ... > snprintf(buff + 1 * ETH_GSTRING_LEN * DSAF_PRIO_NR, ETH_GSTRING_LEN, ... > > Of course to make it less verbose you may add new definition(s) and/ or > variable(s). > >> >> And here we put everything to one to reduce codes. >> > > I would suggest to use following pattern for such lines > p[13 + i + 0 * DSAF_PRIO_NR] = hw_stats->rx_pfc[i]; > p[13 + i + 1 * DSAF_PRIO_NR] = hw_stats->tx_pfc[i]; > > That's allow reader to see what are you doing here. > > P.S. This is for the future patches since current is already applied. Hi Andy, Many thanks for you suggestions. I will fix it with a new patch. Thanks, Yisen >