Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-14 Thread David Miller 
From: Nicolas Ferre 
Date: Wed, 14 Jan 2015 16:53:25 +0100

> I see some issues with this patch: can you hold it a little bit please
> (aka NAK)?

I already applied these patches last night to my net-next tree, so
relative fixups against that will need to be submitted.

Submitting new versions of these patches therefore isn't going to
work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-14 Thread Nicolas Ferre 
Le 13/01/2015 23:15, Xander Huff a écrit :
> Currently `ethtool -S` simply returns "no stats available". It
> would be more useful to see what the various ethtool statistics
> registers' values are. This change implements get_ethtool_stats,
> get_strings, and get_sset_count functions to accomplish this.
> 
> Read all GEM statistics registers and sum them into
> macb.ethtool_stats. Add the necessary infrastructure to make this
> accessible via `ethtool -S`.
> 
> Update gem_update_stats to utilize ethtool_stats.
> 
> Signed-off-by: Xander Huff 

David,

I see some issues with this patch: can you hold it a little bit please
(aka NAK)?

Remarks enclosed:

> ---
>  drivers/net/ethernet/cadence/macb.c |  55 +++-
>  drivers/net/ethernet/cadence/macb.h | 256 
> 
>  2 files changed, 307 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c 
> b/drivers/net/ethernet/cadence/macb.c
> index 3767271..dd8c202 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1827,12 +1827,23 @@ static int macb_close(struct net_device *dev)
>  
>  static void gem_update_stats(struct macb *bp)
>  {
> - u32 __iomem *reg = bp->regs + GEM_OTX;
> + int i;
>   u32 *p = >hw_stats.gem.tx_octets_31_0;
> - u32 *end = >hw_stats.gem.rx_udp_checksum_errors + 1;
>  
> - for (; p < end; p++, reg++)
> - *p += __raw_readl(reg);
> + for (i = 0; i < GEM_STATS_LEN; ++i, ++p) {
> + u32 offset = gem_statistics[i].offset;
> + u64 val = __raw_readl(bp->regs+offset);
> +
> + bp->ethtool_stats[i] += val;
> + *p += val;
> +
> + if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) {
> + /* Add GEM_OCTTXH, GEM_OCTRXH */
> + val = __raw_readl(bp->regs+offset+4);

style: whitespace around '+'

> + bp->ethtool_stats[i] += ((u64)val)<<32;

style: ditto

> + *(++p) += val;
> + }
> + }
>  }
>  
>  static struct net_device_stats *gem_get_stats(struct macb *bp)
> @@ -1873,6 +1884,39 @@ static struct net_device_stats *gem_get_stats(struct 
> macb *bp)
>   return nstat;
>  }
>  
> +static void gem_get_ethtool_stats(struct net_device *dev,
> +   struct ethtool_stats *stats, u64 *data)
> +{
> + struct macb *bp;
> +
> + bp = netdev_priv(dev);
> + gem_update_stats(bp);
> + memcpy(data, >ethtool_stats, sizeof(u64)*GEM_STATS_LEN);

style: ditto

> +}
> +
> +static int gem_get_sset_count(struct net_device *dev, int sset)
> +{
> + switch (sset) {
> + case ETH_SS_STATS:
> + return GEM_STATS_LEN;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
> +{
> + int i;
> +
> + switch (sset) {
> + case ETH_SS_STATS:
> + for (i = 0; i < GEM_STATS_LEN; i++, p += ETH_GSTRING_LEN)
> + memcpy(p, gem_statistics[i].stat_string,
> +ETH_GSTRING_LEN);
> + break;
> + }
> +}
> +
>  struct net_device_stats *macb_get_stats(struct net_device *dev)
>  {
>   struct macb *bp = netdev_priv(dev);
> @@ -1988,6 +2032,9 @@ const struct ethtool_ops macb_ethtool_ops = {
>   .get_regs   = macb_get_regs,
>   .get_link   = ethtool_op_get_link,
>   .get_ts_info= ethtool_op_get_ts_info,
> + .get_ethtool_stats  = gem_get_ethtool_stats,
> + .get_strings= gem_get_ethtool_strings,
> + .get_sset_count = gem_get_sset_count,

I think that the 10/100 macb version of this IP doesn't have the same
statistic possibilities: so you shouldn't register these functions for
all the variants of the IP.
Can you please verify this and only register these functions in the proper
if (macb_is_gem(bp)) alternative?


>  };
>  EXPORT_SYMBOL_GPL(macb_ethtool_ops);
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h 
> b/drivers/net/ethernet/cadence/macb.h
> index 8e8c3c9..378b218 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -82,6 +82,159 @@
>  #define GEM_SA4B 0x00A0 /* Specific4 Bottom */
>  #define GEM_SA4T 0x00A4 /* Specific4 Top */
>  #define GEM_OTX  0x0100 /* Octets 
> transmitted */
> +#define GEM_OCTTXL   0x0100 /* Octets transmitted
> + * [31:0]
> + */

Well please place the comment on a single line. Even if it exceeds 80
characters, it can be an exception for this.
However, check with David but personally, I feel that this formatting is
not good...

> +#define GEM_OCTTXH   0x0104 /* Octets

Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-14 Thread David Miller 
From: Nicolas Ferre nicolas.fe...@atmel.com
Date: Wed, 14 Jan 2015 16:53:25 +0100

 I see some issues with this patch: can you hold it a little bit please
 (aka NAK)?

I already applied these patches last night to my net-next tree, so
relative fixups against that will need to be submitted.

Submitting new versions of these patches therefore isn't going to
work.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-14 Thread Nicolas Ferre 
Le 13/01/2015 23:15, Xander Huff a écrit :
 Currently `ethtool -S` simply returns no stats available. It
 would be more useful to see what the various ethtool statistics
 registers' values are. This change implements get_ethtool_stats,
 get_strings, and get_sset_count functions to accomplish this.
 
 Read all GEM statistics registers and sum them into
 macb.ethtool_stats. Add the necessary infrastructure to make this
 accessible via `ethtool -S`.
 
 Update gem_update_stats to utilize ethtool_stats.
 
 Signed-off-by: Xander Huff xander.h...@ni.com

David,

I see some issues with this patch: can you hold it a little bit please
(aka NAK)?

Remarks enclosed:

 ---
  drivers/net/ethernet/cadence/macb.c |  55 +++-
  drivers/net/ethernet/cadence/macb.h | 256 
 
  2 files changed, 307 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/net/ethernet/cadence/macb.c 
 b/drivers/net/ethernet/cadence/macb.c
 index 3767271..dd8c202 100644
 --- a/drivers/net/ethernet/cadence/macb.c
 +++ b/drivers/net/ethernet/cadence/macb.c
 @@ -1827,12 +1827,23 @@ static int macb_close(struct net_device *dev)
  
  static void gem_update_stats(struct macb *bp)
  {
 - u32 __iomem *reg = bp-regs + GEM_OTX;
 + int i;
   u32 *p = bp-hw_stats.gem.tx_octets_31_0;
 - u32 *end = bp-hw_stats.gem.rx_udp_checksum_errors + 1;
  
 - for (; p  end; p++, reg++)
 - *p += __raw_readl(reg);
 + for (i = 0; i  GEM_STATS_LEN; ++i, ++p) {
 + u32 offset = gem_statistics[i].offset;
 + u64 val = __raw_readl(bp-regs+offset);
 +
 + bp-ethtool_stats[i] += val;
 + *p += val;
 +
 + if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) {
 + /* Add GEM_OCTTXH, GEM_OCTRXH */
 + val = __raw_readl(bp-regs+offset+4);

style: whitespace around '+'

 + bp-ethtool_stats[i] += ((u64)val)32;

style: ditto

 + *(++p) += val;
 + }
 + }
  }
  
  static struct net_device_stats *gem_get_stats(struct macb *bp)
 @@ -1873,6 +1884,39 @@ static struct net_device_stats *gem_get_stats(struct 
 macb *bp)
   return nstat;
  }
  
 +static void gem_get_ethtool_stats(struct net_device *dev,
 +   struct ethtool_stats *stats, u64 *data)
 +{
 + struct macb *bp;
 +
 + bp = netdev_priv(dev);
 + gem_update_stats(bp);
 + memcpy(data, bp-ethtool_stats, sizeof(u64)*GEM_STATS_LEN);

style: ditto

 +}
 +
 +static int gem_get_sset_count(struct net_device *dev, int sset)
 +{
 + switch (sset) {
 + case ETH_SS_STATS:
 + return GEM_STATS_LEN;
 + default:
 + return -EOPNOTSUPP;
 + }
 +}
 +
 +static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
 +{
 + int i;
 +
 + switch (sset) {
 + case ETH_SS_STATS:
 + for (i = 0; i  GEM_STATS_LEN; i++, p += ETH_GSTRING_LEN)
 + memcpy(p, gem_statistics[i].stat_string,
 +ETH_GSTRING_LEN);
 + break;
 + }
 +}
 +
  struct net_device_stats *macb_get_stats(struct net_device *dev)
  {
   struct macb *bp = netdev_priv(dev);
 @@ -1988,6 +2032,9 @@ const struct ethtool_ops macb_ethtool_ops = {
   .get_regs   = macb_get_regs,
   .get_link   = ethtool_op_get_link,
   .get_ts_info= ethtool_op_get_ts_info,
 + .get_ethtool_stats  = gem_get_ethtool_stats,
 + .get_strings= gem_get_ethtool_strings,
 + .get_sset_count = gem_get_sset_count,

I think that the 10/100 macb version of this IP doesn't have the same
statistic possibilities: so you shouldn't register these functions for
all the variants of the IP.
Can you please verify this and only register these functions in the proper
if (macb_is_gem(bp)) alternative?


  };
  EXPORT_SYMBOL_GPL(macb_ethtool_ops);
  
 diff --git a/drivers/net/ethernet/cadence/macb.h 
 b/drivers/net/ethernet/cadence/macb.h
 index 8e8c3c9..378b218 100644
 --- a/drivers/net/ethernet/cadence/macb.h
 +++ b/drivers/net/ethernet/cadence/macb.h
 @@ -82,6 +82,159 @@
  #define GEM_SA4B 0x00A0 /* Specific4 Bottom */
  #define GEM_SA4T 0x00A4 /* Specific4 Top */
  #define GEM_OTX  0x0100 /* Octets 
 transmitted */
 +#define GEM_OCTTXL   0x0100 /* Octets transmitted
 + * [31:0]
 + */

Well please place the comment on a single line. Even if it exceeds 80
characters, it can be an exception for this.
However, check with David but personally, I feel that this formatting is
not good...

 +#define GEM_OCTTXH   0x0104 /* Octets transmitted
 + * [47:32]
 +

Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-13 Thread David Miller 
From: Xander Huff 
Date: Tue, 13 Jan 2015 16:15:51 -0600

> Currently `ethtool -S` simply returns "no stats available". It
> would be more useful to see what the various ethtool statistics
> registers' values are. This change implements get_ethtool_stats,
> get_strings, and get_sset_count functions to accomplish this.
> 
> Read all GEM statistics registers and sum them into
> macb.ethtool_stats. Add the necessary infrastructure to make this
> accessible via `ethtool -S`.
> 
> Update gem_update_stats to utilize ethtool_stats.
> 
> Signed-off-by: Xander Huff 

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-13 Thread Xander Huff 
Currently `ethtool -S` simply returns "no stats available". It
would be more useful to see what the various ethtool statistics
registers' values are. This change implements get_ethtool_stats,
get_strings, and get_sset_count functions to accomplish this.

Read all GEM statistics registers and sum them into
macb.ethtool_stats. Add the necessary infrastructure to make this
accessible via `ethtool -S`.

Update gem_update_stats to utilize ethtool_stats.

Signed-off-by: Xander Huff 
---
 drivers/net/ethernet/cadence/macb.c |  55 +++-
 drivers/net/ethernet/cadence/macb.h | 256 
 2 files changed, 307 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 3767271..dd8c202 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1827,12 +1827,23 @@ static int macb_close(struct net_device *dev)
 
 static void gem_update_stats(struct macb *bp)
 {
-   u32 __iomem *reg = bp->regs + GEM_OTX;
+   int i;
u32 *p = >hw_stats.gem.tx_octets_31_0;
-   u32 *end = >hw_stats.gem.rx_udp_checksum_errors + 1;
 
-   for (; p < end; p++, reg++)
-   *p += __raw_readl(reg);
+   for (i = 0; i < GEM_STATS_LEN; ++i, ++p) {
+   u32 offset = gem_statistics[i].offset;
+   u64 val = __raw_readl(bp->regs+offset);
+
+   bp->ethtool_stats[i] += val;
+   *p += val;
+
+   if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) {
+   /* Add GEM_OCTTXH, GEM_OCTRXH */
+   val = __raw_readl(bp->regs+offset+4);
+   bp->ethtool_stats[i] += ((u64)val)<<32;
+   *(++p) += val;
+   }
+   }
 }
 
 static struct net_device_stats *gem_get_stats(struct macb *bp)
@@ -1873,6 +1884,39 @@ static struct net_device_stats *gem_get_stats(struct 
macb *bp)
return nstat;
 }
 
+static void gem_get_ethtool_stats(struct net_device *dev,
+ struct ethtool_stats *stats, u64 *data)
+{
+   struct macb *bp;
+
+   bp = netdev_priv(dev);
+   gem_update_stats(bp);
+   memcpy(data, >ethtool_stats, sizeof(u64)*GEM_STATS_LEN);
+}
+
+static int gem_get_sset_count(struct net_device *dev, int sset)
+{
+   switch (sset) {
+   case ETH_SS_STATS:
+   return GEM_STATS_LEN;
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
+static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
+{
+   int i;
+
+   switch (sset) {
+   case ETH_SS_STATS:
+   for (i = 0; i < GEM_STATS_LEN; i++, p += ETH_GSTRING_LEN)
+   memcpy(p, gem_statistics[i].stat_string,
+  ETH_GSTRING_LEN);
+   break;
+   }
+}
+
 struct net_device_stats *macb_get_stats(struct net_device *dev)
 {
struct macb *bp = netdev_priv(dev);
@@ -1988,6 +2032,9 @@ const struct ethtool_ops macb_ethtool_ops = {
.get_regs   = macb_get_regs,
.get_link   = ethtool_op_get_link,
.get_ts_info= ethtool_op_get_ts_info,
+   .get_ethtool_stats  = gem_get_ethtool_stats,
+   .get_strings= gem_get_ethtool_strings,
+   .get_sset_count = gem_get_sset_count,
 };
 EXPORT_SYMBOL_GPL(macb_ethtool_ops);
 
diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8e8c3c9..378b218 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -82,6 +82,159 @@
 #define GEM_SA4B   0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T   0x00A4 /* Specific4 Top */
 #define GEM_OTX0x0100 /* Octets 
transmitted */
+#define GEM_OCTTXL 0x0100 /* Octets transmitted
+   * [31:0]
+   */
+#define GEM_OCTTXH 0x0104 /* Octets transmitted
+   * [47:32]
+   */
+#define GEM_TXCNT  0x0108 /* Error-free Frames
+   * Transmitted counter
+   */
+#define GEM_TXBCCNT0x010c /* Error-free Broadcast
+   * Frames counter
+   */
+#define GEM_TXMCCNT0x0110 /* Error-free Multicast
+   * Frames counter
+   */
+#define GEM_TXPAUSECNT 0x0114 /* Pause

Re: [PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-13 Thread David Miller 
From: Xander Huff xander.h...@ni.com
Date: Tue, 13 Jan 2015 16:15:51 -0600

 Currently `ethtool -S` simply returns no stats available. It
 would be more useful to see what the various ethtool statistics
 registers' values are. This change implements get_ethtool_stats,
 get_strings, and get_sset_count functions to accomplish this.
 
 Read all GEM statistics registers and sum them into
 macb.ethtool_stats. Add the necessary infrastructure to make this
 accessible via `ethtool -S`.
 
 Update gem_update_stats to utilize ethtool_stats.
 
 Signed-off-by: Xander Huff xander.h...@ni.com

Applied.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] net/macb: improved ethtool statistics support

2015-01-13 Thread Xander Huff 
Currently `ethtool -S` simply returns no stats available. It
would be more useful to see what the various ethtool statistics
registers' values are. This change implements get_ethtool_stats,
get_strings, and get_sset_count functions to accomplish this.

Read all GEM statistics registers and sum them into
macb.ethtool_stats. Add the necessary infrastructure to make this
accessible via `ethtool -S`.

Update gem_update_stats to utilize ethtool_stats.

Signed-off-by: Xander Huff xander.h...@ni.com
---
 drivers/net/ethernet/cadence/macb.c |  55 +++-
 drivers/net/ethernet/cadence/macb.h | 256 
 2 files changed, 307 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 3767271..dd8c202 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1827,12 +1827,23 @@ static int macb_close(struct net_device *dev)
 
 static void gem_update_stats(struct macb *bp)
 {
-   u32 __iomem *reg = bp-regs + GEM_OTX;
+   int i;
u32 *p = bp-hw_stats.gem.tx_octets_31_0;
-   u32 *end = bp-hw_stats.gem.rx_udp_checksum_errors + 1;
 
-   for (; p  end; p++, reg++)
-   *p += __raw_readl(reg);
+   for (i = 0; i  GEM_STATS_LEN; ++i, ++p) {
+   u32 offset = gem_statistics[i].offset;
+   u64 val = __raw_readl(bp-regs+offset);
+
+   bp-ethtool_stats[i] += val;
+   *p += val;
+
+   if (offset == GEM_OCTTXL || offset == GEM_OCTRXL) {
+   /* Add GEM_OCTTXH, GEM_OCTRXH */
+   val = __raw_readl(bp-regs+offset+4);
+   bp-ethtool_stats[i] += ((u64)val)32;
+   *(++p) += val;
+   }
+   }
 }
 
 static struct net_device_stats *gem_get_stats(struct macb *bp)
@@ -1873,6 +1884,39 @@ static struct net_device_stats *gem_get_stats(struct 
macb *bp)
return nstat;
 }
 
+static void gem_get_ethtool_stats(struct net_device *dev,
+ struct ethtool_stats *stats, u64 *data)
+{
+   struct macb *bp;
+
+   bp = netdev_priv(dev);
+   gem_update_stats(bp);
+   memcpy(data, bp-ethtool_stats, sizeof(u64)*GEM_STATS_LEN);
+}
+
+static int gem_get_sset_count(struct net_device *dev, int sset)
+{
+   switch (sset) {
+   case ETH_SS_STATS:
+   return GEM_STATS_LEN;
+   default:
+   return -EOPNOTSUPP;
+   }
+}
+
+static void gem_get_ethtool_strings(struct net_device *dev, u32 sset, u8 *p)
+{
+   int i;
+
+   switch (sset) {
+   case ETH_SS_STATS:
+   for (i = 0; i  GEM_STATS_LEN; i++, p += ETH_GSTRING_LEN)
+   memcpy(p, gem_statistics[i].stat_string,
+  ETH_GSTRING_LEN);
+   break;
+   }
+}
+
 struct net_device_stats *macb_get_stats(struct net_device *dev)
 {
struct macb *bp = netdev_priv(dev);
@@ -1988,6 +2032,9 @@ const struct ethtool_ops macb_ethtool_ops = {
.get_regs   = macb_get_regs,
.get_link   = ethtool_op_get_link,
.get_ts_info= ethtool_op_get_ts_info,
+   .get_ethtool_stats  = gem_get_ethtool_stats,
+   .get_strings= gem_get_ethtool_strings,
+   .get_sset_count = gem_get_sset_count,
 };
 EXPORT_SYMBOL_GPL(macb_ethtool_ops);
 
diff --git a/drivers/net/ethernet/cadence/macb.h 
b/drivers/net/ethernet/cadence/macb.h
index 8e8c3c9..378b218 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -82,6 +82,159 @@
 #define GEM_SA4B   0x00A0 /* Specific4 Bottom */
 #define GEM_SA4T   0x00A4 /* Specific4 Top */
 #define GEM_OTX0x0100 /* Octets 
transmitted */
+#define GEM_OCTTXL 0x0100 /* Octets transmitted
+   * [31:0]
+   */
+#define GEM_OCTTXH 0x0104 /* Octets transmitted
+   * [47:32]
+   */
+#define GEM_TXCNT  0x0108 /* Error-free Frames
+   * Transmitted counter
+   */
+#define GEM_TXBCCNT0x010c /* Error-free Broadcast
+   * Frames counter
+   */
+#define GEM_TXMCCNT0x0110 /* Error-free Multicast
+   * Frames counter
+   */
+#define GEM_TXPAUSECNT