Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-22 Thread Yisen Zhuang 


在 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 = 
> 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

>

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-22 Thread Yisen Zhuang 


在 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 = 
> 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

>

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-22 Thread 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 = 
> > > >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.

-- 

Andy Shevchenko 
Intel Finland Oy

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-22 Thread 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 = 
> > > >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.

-- 

Andy Shevchenko 
Intel Finland Oy

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-21 Thread Yisen Zhuang 


在 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 = >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.

And here we put everything to one to reduce codes.

I will generate a new patch to fix other comments.

Thanks,

Yisen

> 
>> +p[29] = hw_stats->tx_pkts;
>> +return [30];
>> +}
>>  
>> +p[13] = hw_stats->tx_pkts;
>>  return [14];
>>  }
>

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-21 Thread Yisen Zhuang 


在 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 = >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.

And here we put everything to one to reduce codes.

I will generate a new patch to fix other comments.

Thanks,

Yisen

> 
>> +p[29] = hw_stats->tx_pkts;
>> +return [30];
>> +}
>>  
>> +p[13] = hw_stats->tx_pkts;
>>  return [14];
>>  }
>

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-21 Thread 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 = >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.

> + p[29] = hw_stats->tx_pkts;
> + return [30];
> + }
>  
> + p[13] = hw_stats->tx_pkts;
>   return [14];
>  }

-- 

Andy Shevchenko 
Intel Finland Oy

Re: [PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-21 Thread 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 = >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.

> + p[29] = hw_stats->tx_pkts;
> + return [30];
> + }
>  
> + p[13] = hw_stats->tx_pkts;
>   return [14];
>  }

-- 

Andy Shevchenko 
Intel Finland Oy

[PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-20 Thread Yisen Zhuang 
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.

Signed-off-by: Daode Huang 
Signed-off-by: Yisen Zhuang 
---
 drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c  |  6 +-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 72 +++---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 10 ++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  |  5 ++
 4 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c 
b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
index d37b778..b97cc75 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
@@ -587,6 +587,7 @@ void hns_ae_get_strings(struct hnae_handle *handle,
int idx;
struct hns_mac_cb *mac_cb;
struct hns_ppe_cb *ppe_cb;
+   struct dsaf_device *dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
u8 *p = data;
struct  hnae_vf_cb *vf_cb;
 
@@ -609,13 +610,14 @@ void hns_ae_get_strings(struct hnae_handle *handle,
p += ETH_GSTRING_LEN * hns_mac_get_sset_count(mac_cb, stringset);
 
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
-   hns_dsaf_get_strings(stringset, p, port);
+   hns_dsaf_get_strings(stringset, p, port, dsaf_dev);
 }
 
 int hns_ae_get_sset_count(struct hnae_handle *handle, int stringset)
 {
u32 sset_count = 0;
struct hns_mac_cb *mac_cb;
+   struct dsaf_device *dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
 
assert(handle);
 
@@ -626,7 +628,7 @@ int hns_ae_get_sset_count(struct hnae_handle *handle, int 
stringset)
sset_count += hns_mac_get_sset_count(mac_cb, stringset);
 
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
-   sset_count += hns_dsaf_get_sset_count(stringset);
+   sset_count += hns_dsaf_get_sset_count(dsaf_dev, stringset);
 
return sset_count;
 }
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index b8b2ff9..0edea9c 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
@@ -2096,11 +2096,24 @@ void hns_dsaf_fix_mac_mode(struct hns_mac_cb *mac_cb)
hns_dsaf_port_work_rate_cfg(dsaf_dev, mac_id, mode);
 }
 
+static u32 hns_dsaf_get_inode_prio_reg(int index)
+{
+   int base_index, offset;
+   u32 base_addr = DSAF_INODE_IN_PRIO_PAUSE_BASE_REG;
+
+   base_index = (index + 1) / DSAF_REG_PER_ZONE;
+   offset = (index + 1) % DSAF_REG_PER_ZONE;
+
+   return base_addr + DSAF_INODE_IN_PRIO_PAUSE_BASE_OFFSET * base_index +
+   DSAF_INODE_IN_PRIO_PAUSE_OFFSET * offset;
+}
+
 void hns_dsaf_update_stats(struct dsaf_device *dsaf_dev, u32 node_num)
 {
struct dsaf_hw_stats *hw_stats
= _dev->hw_stats[node_num];
bool is_ver1 = AE_IS_VER1(dsaf_dev->dsaf_ver);
+   int i;
u32 reg_tmp;
 
hw_stats->pad_drop += dsaf_read_dev(dsaf_dev,
@@ -2135,6 +2148,18 @@ void hns_dsaf_update_stats(struct dsaf_device *dsaf_dev, 
u32 node_num)
hw_stats->stp_drop += dsaf_read_dev(dsaf_dev,
DSAF_INODE_IN_DATA_STP_DISC_0_REG + 0x80 * (u64)node_num);
 
+   /* pfc pause frame statistics stored in dsaf inode*/
+   if ((node_num < DSAF_SERVICE_NW_NUM) && !is_ver1) {
+   for (i = 0; i < DSAF_PRIO_NR; i++) {
+   reg_tmp = hns_dsaf_get_inode_prio_reg(i);
+   hw_stats->rx_pfc[i] += dsaf_read_dev(dsaf_dev,
+   reg_tmp + 0x4 * (u64)node_num);
+   hw_stats->tx_pfc[i] += dsaf_read_dev(dsaf_dev,
+   DSAF_XOD_XGE_PFC_PRIO_CNT_BASE_REG +
+   DSAF_XOD_XGE_PFC_PRIO_CNT_OFFSET * i +
+   0xF0 * (u64)node_num);
+   }
+   }
hw_stats->tx_pkts += dsaf_read_dev(dsaf_dev,
DSAF_XOD_RCVPKT_CNT_0_REG + 0x90 * (u64)node_num);
 }
@@ -2472,9 +2497,12 @@ void hns_dsaf_get_regs(struct dsaf_device *ddev, u32 
port, void *data)
p[i] = 0x;
 }
 
-static char *hns_dsaf_get_node_stats_strings(char *data, int node)
+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,

[PATCH net-next 10/19] net: hns: bugfix about pfc pause frame statistics

2016-06-20 Thread Yisen Zhuang 
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.

Signed-off-by: Daode Huang 
Signed-off-by: Yisen Zhuang 
---
 drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c  |  6 +-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 72 +++---
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 10 ++-
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  |  5 ++
 4 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c 
b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
index d37b778..b97cc75 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
@@ -587,6 +587,7 @@ void hns_ae_get_strings(struct hnae_handle *handle,
int idx;
struct hns_mac_cb *mac_cb;
struct hns_ppe_cb *ppe_cb;
+   struct dsaf_device *dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
u8 *p = data;
struct  hnae_vf_cb *vf_cb;
 
@@ -609,13 +610,14 @@ void hns_ae_get_strings(struct hnae_handle *handle,
p += ETH_GSTRING_LEN * hns_mac_get_sset_count(mac_cb, stringset);
 
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
-   hns_dsaf_get_strings(stringset, p, port);
+   hns_dsaf_get_strings(stringset, p, port, dsaf_dev);
 }
 
 int hns_ae_get_sset_count(struct hnae_handle *handle, int stringset)
 {
u32 sset_count = 0;
struct hns_mac_cb *mac_cb;
+   struct dsaf_device *dsaf_dev = hns_ae_get_dsaf_dev(handle->dev);
 
assert(handle);
 
@@ -626,7 +628,7 @@ int hns_ae_get_sset_count(struct hnae_handle *handle, int 
stringset)
sset_count += hns_mac_get_sset_count(mac_cb, stringset);
 
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
-   sset_count += hns_dsaf_get_sset_count(stringset);
+   sset_count += hns_dsaf_get_sset_count(dsaf_dev, stringset);
 
return sset_count;
 }
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
index b8b2ff9..0edea9c 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
@@ -2096,11 +2096,24 @@ void hns_dsaf_fix_mac_mode(struct hns_mac_cb *mac_cb)
hns_dsaf_port_work_rate_cfg(dsaf_dev, mac_id, mode);
 }
 
+static u32 hns_dsaf_get_inode_prio_reg(int index)
+{
+   int base_index, offset;
+   u32 base_addr = DSAF_INODE_IN_PRIO_PAUSE_BASE_REG;
+
+   base_index = (index + 1) / DSAF_REG_PER_ZONE;
+   offset = (index + 1) % DSAF_REG_PER_ZONE;
+
+   return base_addr + DSAF_INODE_IN_PRIO_PAUSE_BASE_OFFSET * base_index +
+   DSAF_INODE_IN_PRIO_PAUSE_OFFSET * offset;
+}
+
 void hns_dsaf_update_stats(struct dsaf_device *dsaf_dev, u32 node_num)
 {
struct dsaf_hw_stats *hw_stats
= _dev->hw_stats[node_num];
bool is_ver1 = AE_IS_VER1(dsaf_dev->dsaf_ver);
+   int i;
u32 reg_tmp;
 
hw_stats->pad_drop += dsaf_read_dev(dsaf_dev,
@@ -2135,6 +2148,18 @@ void hns_dsaf_update_stats(struct dsaf_device *dsaf_dev, 
u32 node_num)
hw_stats->stp_drop += dsaf_read_dev(dsaf_dev,
DSAF_INODE_IN_DATA_STP_DISC_0_REG + 0x80 * (u64)node_num);
 
+   /* pfc pause frame statistics stored in dsaf inode*/
+   if ((node_num < DSAF_SERVICE_NW_NUM) && !is_ver1) {
+   for (i = 0; i < DSAF_PRIO_NR; i++) {
+   reg_tmp = hns_dsaf_get_inode_prio_reg(i);
+   hw_stats->rx_pfc[i] += dsaf_read_dev(dsaf_dev,
+   reg_tmp + 0x4 * (u64)node_num);
+   hw_stats->tx_pfc[i] += dsaf_read_dev(dsaf_dev,
+   DSAF_XOD_XGE_PFC_PRIO_CNT_BASE_REG +
+   DSAF_XOD_XGE_PFC_PRIO_CNT_OFFSET * i +
+   0xF0 * (u64)node_num);
+   }
+   }
hw_stats->tx_pkts += dsaf_read_dev(dsaf_dev,
DSAF_XOD_RCVPKT_CNT_0_REG + 0x90 * (u64)node_num);
 }
@@ -2472,9 +2497,12 @@ void hns_dsaf_get_regs(struct dsaf_device *ddev, u32 
port, void *data)
p[i] = 0x;
 }
 
-static char *hns_dsaf_get_node_stats_strings(char *data, int node)
+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 <