| Message ID | 1467021255-95900-7-git-send-email-Yisen.Zhuang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > }
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; > > } >
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.
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.
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 --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];