Re: [PATCH] Broadcom Sysport: Support 64bit net status report on 32bit Platform

[kiki good]

Hi Florian:

Thanks for the comment.

The reason to use kzalloc() for bsysport_netstats instead of extending
bcm_sysport_priv to have the network
stats included in bcm_sysport_priv is to minimise the impact of
increasing stack size if there are more 64bit stats
to be added in the future.

What about your thoughts ?

I will do the changes as you suggested for the rest parts.

Thanks
Jmqiao

On Sat, Jul 15, 2017 at 5:11 PM, kiki good  wrote:
> Signed-off-by: Jianming Qiao 
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 81
> -
>  drivers/net/ethernet/broadcom/bcmsysport.h |  8 
>  2 files changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 5274501..263512f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -663,6 +663,7 @@ static unsigned int bcm_sysport_desc_rx(struct
> bcm_sysport_priv *priv,
>  unsigned int budget)
>  {
>  struct net_device *ndev = priv->netdev;
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
>  unsigned int processed = 0, to_process;
>  struct bcm_sysport_cb *cb;
>  struct sk_buff *skb;
> @@ -763,8 +764,10 @@ static unsigned int bcm_sysport_desc_rx(struct
> bcm_sysport_priv *priv,
>  }
>
>  skb->protocol = eth_type_trans(skb, ndev);
> -   ndev->stats.rx_packets++;
> -   ndev->stats.rx_bytes += len;
> +   u64_stats_update_begin(_netstats->syncp);
> +   bsysport_netstats->rx_packets++;
> +   bsysport_netstats->rx_bytes += len;
> +   u64_stats_update_end(_netstats->syncp);
>
>  napi_gro_receive(>napi, skb);
>  next:
> @@ -785,19 +788,26 @@ static void bcm_sysport_tx_reclaim_one(struct
> bcm_sysport_tx_ring *ring,
>  {
>  struct bcm_sysport_priv *priv = ring->priv;
>  struct device *kdev = >pdev->dev;
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
>
>  if (cb->skb) {
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->bytes += cb->skb->len;
> +   u64_stats_update_end(_netstats->syncp);
>  *bytes_compl += cb->skb->len;
>  dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
>   dma_unmap_len(cb, dma_len),
>   DMA_TO_DEVICE);
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->packets++;
> +   u64_stats_update_end(_netstats->syncp);
>  (*pkts_compl)++;
>  bcm_sysport_free_cb(cb);
>  /* SKB fragment */
>  } else if (dma_unmap_addr(cb, dma_addr)) {
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->bytes += dma_unmap_len(cb, dma_len);
> +   u64_stats_update_end(_netstats->syncp);
>  dma_unmap_page(kdev, dma_unmap_addr(cb, dma_addr),
> dma_unmap_len(cb, dma_len), DMA_TO_DEVICE);
>  dma_unmap_addr_set(cb, dma_addr, 0);
> @@ -1671,23 +1681,6 @@ static int bcm_sysport_change_mac(struct
> net_device *dev, void *p)
>  return 0;
>  }
>
> -static struct net_device_stats *bcm_sysport_get_nstats(struct net_device 
> *dev)
> -{
> -   struct bcm_sysport_priv *priv = netdev_priv(dev);
> -   unsigned long tx_bytes = 0, tx_packets = 0;
> -   struct bcm_sysport_tx_ring *ring;
> -   unsigned int q;
> -
> -   for (q = 0; q < dev->num_tx_queues; q++) {
> -   ring = >tx_rings[q];
> -   tx_bytes += ring->bytes;
> -   tx_packets += ring->packets;
> -   }
> -
> -   dev->stats.tx_bytes = tx_bytes;
> -   dev->stats.tx_packets = tx_packets;
> -   return >stats;
> -}
>
>  static void bcm_sysport_netif_start(struct net_device *dev)
>  {
> @@ -1923,6 +1916,49 @@ static int bcm_sysport_stop(struct net_device *dev)
>  return 0;
>  }
>
> +static int bcm_sysport_init(struct net_device *dev)
> +{
> +   struct bcm_sysport_priv *priv = netdev_priv(dev);
> +
> +   priv->bsysport_netstats = kzalloc(sizeof(*priv->bsysport_netstats),
> + GFP_KERNEL);
> +   if (!priv->bsysport_netstats)
> +   return -ENOMEM;
> +
> +   return 0;
> +}
> +
> +static void bcm_sysport_get_stats64(struct net_device *dev,
> +   struct rtnl_link_stats64 *stats)
> +{
> +   struct bcm_sysport_priv *priv = netdev_priv(dev);
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
> +   struct bcm_sysport_tx_ring 

Re: [PATCH] Broadcom Sysport: Support 64bit net status report on 32bit Platform

[Florian Fainelli]

On 07/15/2017 08:20 AM, ?? ? wrote:
> When using Broadcom Sysport device in 32bit Platform, ifconfig can only 
> report up to 4G
> tx,rx status, which will be wrapped to 0 when the number of incoming or 
> outgoing 
> packets exceeds 4G. 

The subject of this patch should be:

net: systemport: Support 64bit statistics

to be consistent with past submissions done to that driver and it should
be Broadcom SYSTEMPORT, Sysport is just the abbreviated name for the driver.

> 
> Signed-off-by: Jianming Qiao 
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 81 
> -
>  drivers/net/ethernet/broadcom/bcmsysport.h |  8 
>  2 files changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c 
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 5274501..263512f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -663,6 +663,7 @@ static unsigned int bcm_sysport_desc_rx(struct 
> bcm_sysport_priv *priv,
>   unsigned int budget)
>  {
>   struct net_device *ndev = priv->netdev;
> + struct bcmsysport_netstats *bsysport_netstats = priv->bsysport_netstats;

Please keep the reverse christmas tree declarations order, from longest
to shortest and I am not a fan of the name "bsysort_netstats" it's
already in the driver private structure, so just name the new member
"stats64" for instance, see at the very bottom for a complete suggestion.

>   unsigned int processed = 0, to_process;
>   struct bcm_sysport_cb *cb;
>   struct sk_buff *skb;
> @@ -763,8 +764,10 @@ static unsigned int bcm_sysport_desc_rx(struct 
> bcm_sysport_priv *priv,
>   }
>  
>   skb->protocol = eth_type_trans(skb, ndev);
> - ndev->stats.rx_packets++;
> - ndev->stats.rx_bytes += len;
> + u64_stats_update_begin(_netstats->syncp);
> + bsysport_netstats->rx_packets++;
> + bsysport_netstats->rx_bytes += len;
> + u64_stats_update_end(_netstats->syncp);
>  
>   napi_gro_receive(>napi, skb);
>  next:
> @@ -785,19 +788,26 @@ static void bcm_sysport_tx_reclaim_one(struct 
> bcm_sysport_tx_ring *ring,
>  {
>   struct bcm_sysport_priv *priv = ring->priv;
>   struct device *kdev = >pdev->dev;
> + struct bcmsysport_netstats *bsysport_netstats = priv->bsysport_netstats;
>  
>   if (cb->skb) {
> + u64_stats_update_begin(_netstats->syncp);
>   ring->bytes += cb->skb->len;
> + u64_stats_update_end(_netstats->syncp);
>   *bytes_compl += cb->skb->len;
>   dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
>dma_unmap_len(cb, dma_len),
>DMA_TO_DEVICE);
> + u64_stats_update_begin(_netstats->syncp);
>   ring->packets++;
> + u64_stats_update_end(_netstats->syncp);
>   (*pkts_compl)++;
>   bcm_sysport_free_cb(cb);
>   /* SKB fragment */
>   } else if (dma_unmap_addr(cb, dma_addr)) {
> + u64_stats_update_begin(_netstats->syncp);
>   ring->bytes += dma_unmap_len(cb, dma_len);
> + u64_stats_update_end(_netstats->syncp);
>   dma_unmap_page(kdev, dma_unmap_addr(cb, dma_addr),
>  dma_unmap_len(cb, dma_len), DMA_TO_DEVICE);
>   dma_unmap_addr_set(cb, dma_addr, 0);

Can you find a way to avoid putting 3 different
u64_stats_update_begin()/end() sections and instead just one, that would
be clearer.

> @@ -1671,23 +1681,6 @@ static int bcm_sysport_change_mac(struct net_device 
> *dev, void *p)
>   return 0;
>  }
>  
> -static struct net_device_stats *bcm_sysport_get_nstats(struct net_device 
> *dev)
> -{
> - struct bcm_sysport_priv *priv = netdev_priv(dev);
> - unsigned long tx_bytes = 0, tx_packets = 0;
> - struct bcm_sysport_tx_ring *ring;
> - unsigned int q;
> -
> - for (q = 0; q < dev->num_tx_queues; q++) {
> - ring = >tx_rings[q];
> - tx_bytes += ring->bytes;
> - tx_packets += ring->packets;
> - }
> -
> - dev->stats.tx_bytes = tx_bytes;
> - dev->stats.tx_packets = tx_packets;
> - return >stats;
> -}

Is there a problem if we keep both get_nstats and get_nstats64?

>  
>  static void bcm_sysport_netif_start(struct net_device *dev)
>  {
> @@ -1923,6 +1916,49 @@ static int bcm_sysport_stop(struct net_device *dev)
>   return 0;
>  }
>  
> +static int bcm_sysport_init(struct net_device *dev)
> +{
> + struct bcm_sysport_priv *priv = netdev_priv(dev);
> +
> + priv->bsysport_netstats = kzalloc(sizeof(*priv->bsysport_netstats),
> +   GFP_KERNEL);
> + if (!priv->bsysport_netstats)
> + return 

Re: [PATCH] Broadcom Sysport: Support 64bit net status report on 32bit Platform

[kiki good]

When using Broadcom Sysport device in 32bit Platform, ifconfig can
only report up to 4G
tx,rx status, which will be wrapped to 0 when the number of incoming or outgoing
packets exceeds 4G.

The patch is used to add 64bit support for Broadcom Sysport device in
32bit Platform.

On Sat, Jul 15, 2017 at 5:11 PM, kiki good  wrote:
> Signed-off-by: Jianming Qiao 
> ---
>  drivers/net/ethernet/broadcom/bcmsysport.c | 81
> -
>  drivers/net/ethernet/broadcom/bcmsysport.h |  8 
>  2 files changed, 68 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
> b/drivers/net/ethernet/broadcom/bcmsysport.c
> index 5274501..263512f 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -663,6 +663,7 @@ static unsigned int bcm_sysport_desc_rx(struct
> bcm_sysport_priv *priv,
>  unsigned int budget)
>  {
>  struct net_device *ndev = priv->netdev;
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
>  unsigned int processed = 0, to_process;
>  struct bcm_sysport_cb *cb;
>  struct sk_buff *skb;
> @@ -763,8 +764,10 @@ static unsigned int bcm_sysport_desc_rx(struct
> bcm_sysport_priv *priv,
>  }
>
>  skb->protocol = eth_type_trans(skb, ndev);
> -   ndev->stats.rx_packets++;
> -   ndev->stats.rx_bytes += len;
> +   u64_stats_update_begin(_netstats->syncp);
> +   bsysport_netstats->rx_packets++;
> +   bsysport_netstats->rx_bytes += len;
> +   u64_stats_update_end(_netstats->syncp);
>
>  napi_gro_receive(>napi, skb);
>  next:
> @@ -785,19 +788,26 @@ static void bcm_sysport_tx_reclaim_one(struct
> bcm_sysport_tx_ring *ring,
>  {
>  struct bcm_sysport_priv *priv = ring->priv;
>  struct device *kdev = >pdev->dev;
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
>
>  if (cb->skb) {
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->bytes += cb->skb->len;
> +   u64_stats_update_end(_netstats->syncp);
>  *bytes_compl += cb->skb->len;
>  dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
>   dma_unmap_len(cb, dma_len),
>   DMA_TO_DEVICE);
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->packets++;
> +   u64_stats_update_end(_netstats->syncp);
>  (*pkts_compl)++;
>  bcm_sysport_free_cb(cb);
>  /* SKB fragment */
>  } else if (dma_unmap_addr(cb, dma_addr)) {
> +   u64_stats_update_begin(_netstats->syncp);
>  ring->bytes += dma_unmap_len(cb, dma_len);
> +   u64_stats_update_end(_netstats->syncp);
>  dma_unmap_page(kdev, dma_unmap_addr(cb, dma_addr),
> dma_unmap_len(cb, dma_len), DMA_TO_DEVICE);
>  dma_unmap_addr_set(cb, dma_addr, 0);
> @@ -1671,23 +1681,6 @@ static int bcm_sysport_change_mac(struct
> net_device *dev, void *p)
>  return 0;
>  }
>
> -static struct net_device_stats *bcm_sysport_get_nstats(struct net_device 
> *dev)
> -{
> -   struct bcm_sysport_priv *priv = netdev_priv(dev);
> -   unsigned long tx_bytes = 0, tx_packets = 0;
> -   struct bcm_sysport_tx_ring *ring;
> -   unsigned int q;
> -
> -   for (q = 0; q < dev->num_tx_queues; q++) {
> -   ring = >tx_rings[q];
> -   tx_bytes += ring->bytes;
> -   tx_packets += ring->packets;
> -   }
> -
> -   dev->stats.tx_bytes = tx_bytes;
> -   dev->stats.tx_packets = tx_packets;
> -   return >stats;
> -}
>
>  static void bcm_sysport_netif_start(struct net_device *dev)
>  {
> @@ -1923,6 +1916,49 @@ static int bcm_sysport_stop(struct net_device *dev)
>  return 0;
>  }
>
> +static int bcm_sysport_init(struct net_device *dev)
> +{
> +   struct bcm_sysport_priv *priv = netdev_priv(dev);
> +
> +   priv->bsysport_netstats = kzalloc(sizeof(*priv->bsysport_netstats),
> + GFP_KERNEL);
> +   if (!priv->bsysport_netstats)
> +   return -ENOMEM;
> +
> +   return 0;
> +}
> +
> +static void bcm_sysport_get_stats64(struct net_device *dev,
> +   struct rtnl_link_stats64 *stats)
> +{
> +   struct bcm_sysport_priv *priv = netdev_priv(dev);
> +   struct bcmsysport_netstats *bsysport_netstats = 
> priv->bsysport_netstats;
> +   struct bcm_sysport_tx_ring *ring;
> +   unsigned int q;
> +   u64 tx_packets = 0, tx_bytes = 0;
> +   unsigned int start;
> +
> +   

[PATCH] Broadcom Sysport: Support 64bit net status report on 32bit Platform

[kiki good]

Signed-off-by: Jianming Qiao 
---
 drivers/net/ethernet/broadcom/bcmsysport.c | 81
-
 drivers/net/ethernet/broadcom/bcmsysport.h |  8 
 2 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
b/drivers/net/ethernet/broadcom/bcmsysport.c
index 5274501..263512f 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -663,6 +663,7 @@ static unsigned int bcm_sysport_desc_rx(struct
bcm_sysport_priv *priv,
 unsigned int budget)
 {
 struct net_device *ndev = priv->netdev;
+   struct bcmsysport_netstats *bsysport_netstats = priv->bsysport_netstats;
 unsigned int processed = 0, to_process;
 struct bcm_sysport_cb *cb;
 struct sk_buff *skb;
@@ -763,8 +764,10 @@ static unsigned int bcm_sysport_desc_rx(struct
bcm_sysport_priv *priv,
 }

 skb->protocol = eth_type_trans(skb, ndev);
-   ndev->stats.rx_packets++;
-   ndev->stats.rx_bytes += len;
+   u64_stats_update_begin(_netstats->syncp);
+   bsysport_netstats->rx_packets++;
+   bsysport_netstats->rx_bytes += len;
+   u64_stats_update_end(_netstats->syncp);

 napi_gro_receive(>napi, skb);
 next:
@@ -785,19 +788,26 @@ static void bcm_sysport_tx_reclaim_one(struct
bcm_sysport_tx_ring *ring,
 {
 struct bcm_sysport_priv *priv = ring->priv;
 struct device *kdev = >pdev->dev;
+   struct bcmsysport_netstats *bsysport_netstats = priv->bsysport_netstats;

 if (cb->skb) {
+   u64_stats_update_begin(_netstats->syncp);
 ring->bytes += cb->skb->len;
+   u64_stats_update_end(_netstats->syncp);
 *bytes_compl += cb->skb->len;
 dma_unmap_single(kdev, dma_unmap_addr(cb, dma_addr),
  dma_unmap_len(cb, dma_len),
  DMA_TO_DEVICE);
+   u64_stats_update_begin(_netstats->syncp);
 ring->packets++;
+   u64_stats_update_end(_netstats->syncp);
 (*pkts_compl)++;
 bcm_sysport_free_cb(cb);
 /* SKB fragment */
 } else if (dma_unmap_addr(cb, dma_addr)) {
+   u64_stats_update_begin(_netstats->syncp);
 ring->bytes += dma_unmap_len(cb, dma_len);
+   u64_stats_update_end(_netstats->syncp);
 dma_unmap_page(kdev, dma_unmap_addr(cb, dma_addr),
dma_unmap_len(cb, dma_len), DMA_TO_DEVICE);
 dma_unmap_addr_set(cb, dma_addr, 0);
@@ -1671,23 +1681,6 @@ static int bcm_sysport_change_mac(struct
net_device *dev, void *p)
 return 0;
 }

-static struct net_device_stats *bcm_sysport_get_nstats(struct net_device *dev)
-{
-   struct bcm_sysport_priv *priv = netdev_priv(dev);
-   unsigned long tx_bytes = 0, tx_packets = 0;
-   struct bcm_sysport_tx_ring *ring;
-   unsigned int q;
-
-   for (q = 0; q < dev->num_tx_queues; q++) {
-   ring = >tx_rings[q];
-   tx_bytes += ring->bytes;
-   tx_packets += ring->packets;
-   }
-
-   dev->stats.tx_bytes = tx_bytes;
-   dev->stats.tx_packets = tx_packets;
-   return >stats;
-}

 static void bcm_sysport_netif_start(struct net_device *dev)
 {
@@ -1923,6 +1916,49 @@ static int bcm_sysport_stop(struct net_device *dev)
 return 0;
 }

+static int bcm_sysport_init(struct net_device *dev)
+{
+   struct bcm_sysport_priv *priv = netdev_priv(dev);
+
+   priv->bsysport_netstats = kzalloc(sizeof(*priv->bsysport_netstats),
+ GFP_KERNEL);
+   if (!priv->bsysport_netstats)
+   return -ENOMEM;
+
+   return 0;
+}
+
+static void bcm_sysport_get_stats64(struct net_device *dev,
+   struct rtnl_link_stats64 *stats)
+{
+   struct bcm_sysport_priv *priv = netdev_priv(dev);
+   struct bcmsysport_netstats *bsysport_netstats = priv->bsysport_netstats;
+   struct bcm_sysport_tx_ring *ring;
+   unsigned int q;
+   u64 tx_packets = 0, tx_bytes = 0;
+   unsigned int start;
+
+   netdev_stats_to_stats64(stats, >stats);
+
+   for (q = 0; q < dev->num_tx_queues; q++) {
+   ring = >tx_rings[q];
+   do {
+   start =
u64_stats_fetch_begin_irq(_netstats->syncp);
+   tx_bytes += ring->bytes;
+   tx_packets += ring->packets;
+   } while
(u64_stats_fetch_retry_irq(_netstats->syncp, start));
+   }
+
+   stats->tx_packets = tx_packets;
+   stats->tx_bytes = tx_bytes;
+
+   do {
+   start = u64_stats_fetch_begin_irq(_netstats->syncp);
+