diff mbox

[net-next,6/9] net: hns: normalize two different loop

Message ID 1467021255-95900-7-git-send-email-Yisen.Zhuang@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yisen.Zhuang(Zhuangyuzeng) June 27, 2016, 9:54 a.m. UTC
From: Daode Huang <huangdaode@hisilicon.com>

There are two approaches to assign data, one does 2 loops, another
does 1 loop. This patch normalize the different methods to 1 loop.

Signed-off-by: Daode Huang <huangdaode@hisilicon.com>
---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Joe Perches June 27, 2016, 11:49 a.m. UTC | #1
On Mon, 2016-06-27 at 17:54 +0800, Yisen Zhuang wrote:
> From: Daode Huang <huangdaode@hisilicon.com>
> 
> There are two approaches to assign data, one does 2 loops, another
> does 1 loop. This patch normalize the different methods to 1 loop.
[]
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
[]
> @@ -2567,15 +2567,15 @@ static char *hns_dsaf_get_node_stats_strings(char *data, int node,
>  	buff += ETH_GSTRING_LEN;
>  	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
>  		for (i = 0; i < DSAF_PRIO_NR; i++) {
> -			snprintf(buff, ETH_GSTRING_LEN,
> -				 "inod%d_pfc_prio%d_pkts", node, i);
> -			buff += ETH_GSTRING_LEN;
> -		}
> -		for (i = 0; i < DSAF_PRIO_NR; i++) {
> -			snprintf(buff, ETH_GSTRING_LEN,
> -				 "onod%d_pfc_prio%d_pkts", node, i);
> +			snprintf(buff + 0 * ETH_GSTRING_LEN * DSAF_PRIO_NR,
> +				 ETH_GSTRING_LEN, "inod%d_pfc_prio%d_pkts",
> +				 node, i);
> +			snprintf(buff + 1 * ETH_GSTRING_LEN * DSAF_PRIO_NR,
> +				 ETH_GSTRING_LEN, "onod%d_pfc_prio%d_pkts",
> +				 node, i);
>  			buff += ETH_GSTRING_LEN;

This looks odd and likely incorrect.

>  		}
> +		buff += 1 * DSAF_PRIO_NR * ETH_GSTRING_LEN;
>  	}
Andy Shevchenko June 27, 2016, noon UTC | #2
On Mon, 2016-06-27 at 04:49 -0700, Joe Perches wrote:
> On Mon, 2016-06-27 at 17:54 +0800, Yisen Zhuang wrote:
> > From: Daode Huang <huangdaode@hisilicon.com>
> > 
> > There are two approaches to assign data, one does 2 loops, another
> > does 1 loop. This patch normalize the different methods to 1 loop.
> []
> > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> []
> > @@ -2567,15 +2567,15 @@ static char
> > *hns_dsaf_get_node_stats_strings(char *data, int node,
> >  	buff += ETH_GSTRING_LEN;
> >  	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
> >  		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > -			snprintf(buff, ETH_GSTRING_LEN,
> > -				 "inod%d_pfc_prio%d_pkts", node,
> > i);
> > -			buff += ETH_GSTRING_LEN;
> > -		}
> > -		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > -			snprintf(buff, ETH_GSTRING_LEN,
> > -				 "onod%d_pfc_prio%d_pkts", node,
> > i);
> > +			snprintf(buff + 0 * ETH_GSTRING_LEN *
> > DSAF_PRIO_NR,
> > +				 ETH_GSTRING_LEN,
> > "inod%d_pfc_prio%d_pkts",
> > +				 node, i);
> > +			snprintf(buff + 1 * ETH_GSTRING_LEN *
> > DSAF_PRIO_NR,
> > +				 ETH_GSTRING_LEN,
> > "onod%d_pfc_prio%d_pkts",
> > +				 node, i);
> >  			buff += ETH_GSTRING_LEN;
> 
> This looks odd and likely incorrect.

Why? the idea is to print stats for Rx and Tx at once.

I hope it was tested.

> 
> >  		}
> > +		buff += 1 * DSAF_PRIO_NR * ETH_GSTRING_LEN;
> >  	}
>
Joe Perches June 27, 2016, 12:08 p.m. UTC | #3
On Mon, 2016-06-27 at 15:00 +0300, Andy Shevchenko wrote:
> On Mon, 2016-06-27 at 04:49 -0700, Joe Perches wrote:
> > 
> > On Mon, 2016-06-27 at 17:54 +0800, Yisen Zhuang wrote:
> > > 
> > > From: Daode Huang <huangdaode@hisilicon.com>
> > > 
> > > There are two approaches to assign data, one does 2 loops, another
> > > does 1 loop. This patch normalize the different methods to 1 loop.
> > []
> > > 
> > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > > b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > []
> > > 
> > > @@ -2567,15 +2567,15 @@ static char
> > > *hns_dsaf_get_node_stats_strings(char *data, int node,
> > >  	buff += ETH_GSTRING_LEN;
> > >  	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
> > >  		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > > -			snprintf(buff, ETH_GSTRING_LEN,
> > > -				 "inod%d_pfc_prio%d_pkts", node,
> > > i);
> > > -			buff += ETH_GSTRING_LEN;
> > > -		}
> > > -		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > > -			snprintf(buff, ETH_GSTRING_LEN,
> > > -				 "onod%d_pfc_prio%d_pkts", node,
> > > i);
> > > +			snprintf(buff + 0 * ETH_GSTRING_LEN *
> > > DSAF_PRIO_NR,
> > > +				 ETH_GSTRING_LEN,
> > > "inod%d_pfc_prio%d_pkts",
> > > +				 node, i);
> > > +			snprintf(buff + 1 * ETH_GSTRING_LEN *
> > > DSAF_PRIO_NR,
> > > +				 ETH_GSTRING_LEN,
> > > "onod%d_pfc_prio%d_pkts",
> > > +				 node, i);
> > >  			buff += ETH_GSTRING_LEN;
> > This looks odd and likely incorrect.
> Why? the idea is to print stats for Rx and Tx at once.
> 
> I hope it was tested.

It changes the order of the strings in buff.
Is a bug fix or a style fix?

If it's a bug fix, then it should likely be added
to the stable trees.
Andy Shevchenko June 27, 2016, 12:13 p.m. UTC | #4
On Mon, 2016-06-27 at 05:08 -0700, Joe Perches wrote:
> On Mon, 2016-06-27 at 15:00 +0300, Andy Shevchenko wrote:
> > On Mon, 2016-06-27 at 04:49 -0700, Joe Perches wrote:
> > > 
> > > On Mon, 2016-06-27 at 17:54 +0800, Yisen Zhuang wrote:
> > > > 
> > > > From: Daode Huang <huangdaode@hisilicon.com>
> > > > 
> > > > There are two approaches to assign data, one does 2 loops,
> > > > another
> > > > does 1 loop. This patch normalize the different methods to 1
> > > > loop.
> > > []
> > > > 
> > > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > > > b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> > > []
> > > > 
> > > > @@ -2567,15 +2567,15 @@ static char
> > > > *hns_dsaf_get_node_stats_strings(char *data, int node,
> > > >  	buff += ETH_GSTRING_LEN;
> > > >  	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
> > > >  		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > > > -			snprintf(buff, ETH_GSTRING_LEN,
> > > > -				 "inod%d_pfc_prio%d_pkts",
> > > > node,
> > > > i);
> > > > -			buff += ETH_GSTRING_LEN;
> > > > -		}
> > > > -		for (i = 0; i < DSAF_PRIO_NR; i++) {
> > > > -			snprintf(buff, ETH_GSTRING_LEN,
> > > > -				 "onod%d_pfc_prio%d_pkts",
> > > > node,
> > > > i);
> > > > +			snprintf(buff + 0 * ETH_GSTRING_LEN *
> > > > DSAF_PRIO_NR,
> > > > +				 ETH_GSTRING_LEN,
> > > > "inod%d_pfc_prio%d_pkts",
> > > > +				 node, i);
> > > > +			snprintf(buff + 1 * ETH_GSTRING_LEN *
> > > > DSAF_PRIO_NR,
> > > > +				 ETH_GSTRING_LEN,
> > > > "onod%d_pfc_prio%d_pkts",
> > > > +				 node, i);
> > > >  			buff += ETH_GSTRING_LEN;
> > > This looks odd and likely incorrect.
> > Why? the idea is to print stats for Rx and Tx at once.
> > 
> > I hope it was tested.
> 
> It changes the order of the strings in buff.

I don't see how.

> Is a bug fix or a style fix?
> 
> If it's a bug fix, then it should likely be added
> to the stable trees.

I doubt it's a bug fix.
huangdaode June 28, 2016, 5:41 a.m. UTC | #5
On 2016/6/27 20:13, Andy Shevchenko wrote:
> On Mon, 2016-06-27 at 05:08 -0700, Joe Perches wrote:
>> On Mon, 2016-06-27 at 15:00 +0300, Andy Shevchenko wrote:
>>> On Mon, 2016-06-27 at 04:49 -0700, Joe Perches wrote:
>>>> On Mon, 2016-06-27 at 17:54 +0800, Yisen Zhuang wrote:
>>>>> From: Daode Huang <huangdaode@hisilicon.com>
>>>>>
>>>>> There are two approaches to assign data, one does 2 loops,
>>>>> another
>>>>> does 1 loop. This patch normalize the different methods to 1
>>>>> loop.
>>>> []
>>>>> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
>>>>> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
>>>> []
>>>>> @@ -2567,15 +2567,15 @@ static char
>>>>> *hns_dsaf_get_node_stats_strings(char *data, int node,
>>>>>   	buff += ETH_GSTRING_LEN;
>>>>>   	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
>>>>>   		for (i = 0; i < DSAF_PRIO_NR; i++) {
>>>>> -			snprintf(buff, ETH_GSTRING_LEN,
>>>>> -				 "inod%d_pfc_prio%d_pkts",
>>>>> node,
>>>>> i);
>>>>> -			buff += ETH_GSTRING_LEN;
>>>>> -		}
>>>>> -		for (i = 0; i < DSAF_PRIO_NR; i++) {
>>>>> -			snprintf(buff, ETH_GSTRING_LEN,
>>>>> -				 "onod%d_pfc_prio%d_pkts",
>>>>> node,
>>>>> i);
>>>>> +			snprintf(buff + 0 * ETH_GSTRING_LEN *
>>>>> DSAF_PRIO_NR,
>>>>> +				 ETH_GSTRING_LEN,
>>>>> "inod%d_pfc_prio%d_pkts",
>>>>> +				 node, i);
>>>>> +			snprintf(buff + 1 * ETH_GSTRING_LEN *
>>>>> DSAF_PRIO_NR,
>>>>> +				 ETH_GSTRING_LEN,
>>>>> "onod%d_pfc_prio%d_pkts",
>>>>> +				 node, i);
>>>>>   			buff += ETH_GSTRING_LEN;
>>>> This looks odd and likely incorrect.
>>> Why? the idea is to print stats for Rx and Tx at once.
>>>
>>> I hope it was tested.
>> It changes the order of the strings in buff.
> I don't see how.
Hi Andy,
The patch has been tested when sent out.

>> Is a bug fix or a style fix?
>>
>> If it's a bug fix, then it should likely be added
>> to the stable trees.
> I doubt it's a bug fix.

Because the previous patch is accepted in net-next, and this set is
an appendix to the series, in order to avoid merge conflict, we also
send this bug fix to net-next.

thanks.

>
diff mbox

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index e36ee22..86ce28a 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
@@ -2567,15 +2567,15 @@  static char *hns_dsaf_get_node_stats_strings(char *data, int node,
 	buff += ETH_GSTRING_LEN;
 	if (node < DSAF_SERVICE_NW_NUM && !is_ver1) {
 		for (i = 0; i < DSAF_PRIO_NR; i++) {
-			snprintf(buff, ETH_GSTRING_LEN,
-				 "inod%d_pfc_prio%d_pkts", node, i);
-			buff += ETH_GSTRING_LEN;
-		}
-		for (i = 0; i < DSAF_PRIO_NR; i++) {
-			snprintf(buff, ETH_GSTRING_LEN,
-				 "onod%d_pfc_prio%d_pkts", node, i);
+			snprintf(buff + 0 * ETH_GSTRING_LEN * DSAF_PRIO_NR,
+				 ETH_GSTRING_LEN, "inod%d_pfc_prio%d_pkts",
+				 node, i);
+			snprintf(buff + 1 * ETH_GSTRING_LEN * DSAF_PRIO_NR,
+				 ETH_GSTRING_LEN, "onod%d_pfc_prio%d_pkts",
+				 node, i);
 			buff += ETH_GSTRING_LEN;
 		}
+		buff += 1 * DSAF_PRIO_NR * ETH_GSTRING_LEN;
 	}
 	snprintf(buff, ETH_GSTRING_LEN, "onnod%d_tx_pkts", node);
 	buff += ETH_GSTRING_LEN;
@@ -2606,8 +2606,8 @@  static u64 *hns_dsaf_get_node_stats(struct dsaf_device *ddev, u64 *data,
 	p[12] = hw_stats->stp_drop;
 	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];
+			p[13 + i + 0 * DSAF_PRIO_NR] = hw_stats->rx_pfc[i];
+			p[13 + i + 1 * DSAF_PRIO_NR] = hw_stats->tx_pfc[i];
 		}
 		p[29] = hw_stats->tx_pkts;
 		return &p[30];