[PATCH net-next 0/2] gianfar: make local stats atomic64

[PATCH net-next 0/2] gianfar: make local stats atomic64

[Paul Gortmaker]

Eric noticed that the handling of local u64 ethtool counters for
this driver commonly found on Freescale ppc-32 boards was racy.

However, before converting them over to atomic64_t, I noticed
that an internal struct was being used to determine the offsets
for exporting this data into the ethtool buffer, and in doing
so, it assumed that the counters would always be u64.  Rather
than keep this implicit assumption, a simple code cleanup gets
rid of the struct completely, and leaves less conversion sites.

The alternative solution would have been to take advantage of
the fact that the counters are all relating to error conditions,
and hence make them internally u32.  In doing so, we'd be assuming
that U32_MAX of any particular error condition is highly unlikely.
This might have made sense if any increments were in a hot path.

Tested with "ethtool -S eth0" on sbc8548 board.

Paul.
--

The following changes since commit 0790bbb68f9d483348c1d65381f3dd92602bfd05:

  netpoll: cleanup sparse warnings (2013-02-11 19:19:58 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/paulg/linux.git gfar-ethtool-atomic

for you to fetch changes up to 212079df6d77c0daada96b1d906f4b7749871411:

  gianfar: convert u64 status counters to atomic64_t (2013-02-12 19:08:27 -0500)

----------------------------------------------------------------
Paul Gortmaker (2):
      gianfar: remove largely unused gfar_stats struct
      gianfar: convert u64 status counters to atomic64_t

 drivers/net/ethernet/freescale/gianfar.c         | 26 ++++++++--------
 drivers/net/ethernet/freescale/gianfar.h         | 39 +++++++++++-------------
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 17 +++++------
 3 files changed, 37 insertions(+), 45 deletions(-)

[PATCH net-next 1/2] gianfar: remove largely unused gfar_stats struct

[Paul Gortmaker]

The gfar_stats struct is only used in copying out data
via ethtool.  It is declared as the extra stats, followed
by the rmon stats.  However, the rmon stats are never
actually ever used in the driver; instead the rmon data
is a u32 register read that is cast directly into the
ethtool buf.

It seems the only reason rmon is in the struct at all is
to give the offset(s) at which it should be exported into
the ethtool buffer.  But note gfar_stats doesn't contain
a gfar_extra_stats as a substruct -- instead it contains
a u64 array of equal element count.  This implicitly means
we have two independent declarations of what gfar_extra_stats
really is.  Rather than have this duality, we already have
defines which give us the offset directly, and hence do not
need the struct at all.

Further, since we know the extra_stats is unconditionally
always present, we can write it out to the ethtool buf
1st, and then optionally write out the rmon data.  There
is no need for two independent loops, both of which are
simply copying out the extra_stats to buf offset zero.

This also helps pave the way towards allowing the extra
stats fields to be converted to atomic64_t values, without
having their types directly influencing the ethtool stats
export code (gfar_fill_stats) that expects to deal with u64.

Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.h         |  8 +-------
 drivers/net/ethernet/freescale/gianfar_ethtool.c | 15 ++++++---------
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 71793f4..61b1785 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -646,15 +646,9 @@ struct gfar_extra_stats {
 #define GFAR_RMON_LEN ((sizeof(struct rmon_mib) - 16)/sizeof(u32))
 #define GFAR_EXTRA_STATS_LEN (sizeof(struct gfar_extra_stats)/sizeof(u64))
 
-/* Number of stats in the stats structure (ignore car and cam regs)*/
+/* Number of stats exported via ethtool */
 #define GFAR_STATS_LEN (GFAR_RMON_LEN + GFAR_EXTRA_STATS_LEN)
 
-struct gfar_stats {
-	u64 extra[GFAR_EXTRA_STATS_LEN];
-	u64 rmon[GFAR_RMON_LEN];
-};
-
-
 struct gfar {
 	u32	tsec_id;	/* 0x.000 - Controller ID register */
 	u32	tsec_id2;	/* 0x.004 - Controller ID2 register */
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 45e59d5..172acb9 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -151,18 +151,15 @@ static void gfar_fill_stats(struct net_device *dev, struct ethtool_stats *dummy,
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
 	u64 *extra = (u64 *) & priv->extra_stats;
 
+	for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
+		buf[i] = extra[i];
+
 	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_RMON) {
 		u32 __iomem *rmon = (u32 __iomem *) &regs->rmon;
-		struct gfar_stats *stats = (struct gfar_stats *) buf;
-
-		for (i = 0; i < GFAR_RMON_LEN; i++)
-			stats->rmon[i] = (u64) gfar_read(&rmon[i]);
 
-		for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
-			stats->extra[i] = extra[i];
-	} else
-		for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
-			buf[i] = extra[i];
+		for (; i < GFAR_STATS_LEN; i++, rmon++)
+			buf[i] = (u64) gfar_read(rmon);
+	}
 }
 
 static int gfar_sset_count(struct net_device *dev, int sset)
-- 
1.8.1.2

[PATCH net-next 2/2] gianfar: convert u64 status counters to atomic64_t

[Paul Gortmaker]

While looking at some asm dump for an unrelated change, Eric
noticed in the following stats count increment code:

    50b8:       81 3c 01 f8     lwz     r9,504(r28)
    50bc:       81 5c 01 fc     lwz     r10,508(r28)
    50c0:       31 4a 00 01     addic   r10,r10,1
    50c4:       7d 29 01 94     addze   r9,r9
    50c8:       91 3c 01 f8     stw     r9,504(r28)
    50cc:       91 5c 01 fc     stw     r10,508(r28)

that a 64 bit counter was used on ppc-32 without sync
and hence the "ethtool -S" output was racy.

Here we convert all the values to use atomic64_t so that
the output will always be consistent.

Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/net/ethernet/freescale/gianfar.c         | 26 ++++++++++----------
 drivers/net/ethernet/freescale/gianfar.h         | 31 ++++++++++++------------
 drivers/net/ethernet/freescale/gianfar_ethtool.c |  4 +--
 3 files changed, 31 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index ab32bd0..c82f677 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2648,7 +2648,7 @@ static inline void count_errors(unsigned short status, struct net_device *dev)
 	if (status & RXBD_TRUNCATED) {
 		stats->rx_length_errors++;
 
-		estats->rx_trunc++;
+		atomic64_inc(&estats->rx_trunc);
 
 		return;
 	}
@@ -2657,20 +2657,20 @@ static inline void count_errors(unsigned short status, struct net_device *dev)
 		stats->rx_length_errors++;
 
 		if (status & RXBD_LARGE)
-			estats->rx_large++;
+			atomic64_inc(&estats->rx_large);
 		else
-			estats->rx_short++;
+			atomic64_inc(&estats->rx_short);
 	}
 	if (status & RXBD_NONOCTET) {
 		stats->rx_frame_errors++;
-		estats->rx_nonoctet++;
+		atomic64_inc(&estats->rx_nonoctet);
 	}
 	if (status & RXBD_CRCERR) {
-		estats->rx_crcerr++;
+		atomic64_inc(&estats->rx_crcerr);
 		stats->rx_crc_errors++;
 	}
 	if (status & RXBD_OVERRUN) {
-		estats->rx_overrun++;
+		atomic64_inc(&estats->rx_overrun);
 		stats->rx_crc_errors++;
 	}
 }
@@ -2744,7 +2744,7 @@ static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
 	ret = napi_gro_receive(napi, skb);
 
 	if (GRO_DROP == ret)
-		priv->extra_stats.kernel_dropped++;
+		atomic64_inc(&priv->extra_stats.kernel_dropped);
 
 	return 0;
 }
@@ -2812,7 +2812,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 			} else {
 				netif_warn(priv, rx_err, dev, "Missing skb!\n");
 				rx_queue->stats.rx_dropped++;
-				priv->extra_stats.rx_skbmissing++;
+				atomic64_inc(&priv->extra_stats.rx_skbmissing);
 			}
 
 		}
@@ -3245,7 +3245,7 @@ static irqreturn_t gfar_error(int irq, void *grp_id)
 			netif_dbg(priv, tx_err, dev,
 				  "TX FIFO underrun, packet dropped\n");
 			dev->stats.tx_dropped++;
-			priv->extra_stats.tx_underrun++;
+			atomic64_inc(&priv->extra_stats.tx_underrun);
 
 			local_irq_save(flags);
 			lock_tx_qs(priv);
@@ -3260,7 +3260,7 @@ static irqreturn_t gfar_error(int irq, void *grp_id)
 	}
 	if (events & IEVENT_BSY) {
 		dev->stats.rx_errors++;
-		priv->extra_stats.rx_bsy++;
+		atomic64_inc(&priv->extra_stats.rx_bsy);
 
 		gfar_receive(irq, grp_id);
 
@@ -3269,19 +3269,19 @@ static irqreturn_t gfar_error(int irq, void *grp_id)
 	}
 	if (events & IEVENT_BABR) {
 		dev->stats.rx_errors++;
-		priv->extra_stats.rx_babr++;
+		atomic64_inc(&priv->extra_stats.rx_babr);
 
 		netif_dbg(priv, rx_err, dev, "babbling RX error\n");
 	}
 	if (events & IEVENT_EBERR) {
-		priv->extra_stats.eberr++;
+		atomic64_inc(&priv->extra_stats.eberr);
 		netif_dbg(priv, rx_err, dev, "bus error\n");
 	}
 	if (events & IEVENT_RXC)
 		netif_dbg(priv, rx_status, dev, "control frame\n");
 
 	if (events & IEVENT_BABT) {
-		priv->extra_stats.tx_babt++;
+		atomic64_inc(&priv->extra_stats.tx_babt);
 		netif_dbg(priv, tx_err, dev, "babbling TX error\n");
 	}
 	return IRQ_HANDLED;
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 61b1785..78125f1 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -627,24 +627,25 @@ struct rmon_mib
 };
 
 struct gfar_extra_stats {
-	u64 kernel_dropped;
-	u64 rx_large;
-	u64 rx_short;
-	u64 rx_nonoctet;
-	u64 rx_crcerr;
-	u64 rx_overrun;
-	u64 rx_bsy;
-	u64 rx_babr;
-	u64 rx_trunc;
-	u64 eberr;
-	u64 tx_babt;
-	u64 tx_underrun;
-	u64 rx_skbmissing;
-	u64 tx_timeout;
+	atomic64_t kernel_dropped;
+	atomic64_t rx_large;
+	atomic64_t rx_short;
+	atomic64_t rx_nonoctet;
+	atomic64_t rx_crcerr;
+	atomic64_t rx_overrun;
+	atomic64_t rx_bsy;
+	atomic64_t rx_babr;
+	atomic64_t rx_trunc;
+	atomic64_t eberr;
+	atomic64_t tx_babt;
+	atomic64_t tx_underrun;
+	atomic64_t rx_skbmissing;
+	atomic64_t tx_timeout;
 };
 
 #define GFAR_RMON_LEN ((sizeof(struct rmon_mib) - 16)/sizeof(u32))
-#define GFAR_EXTRA_STATS_LEN (sizeof(struct gfar_extra_stats)/sizeof(u64))
+#define GFAR_EXTRA_STATS_LEN \
+	(sizeof(struct gfar_extra_stats)/sizeof(atomic64_t))
 
 /* Number of stats exported via ethtool */
 #define GFAR_STATS_LEN (GFAR_RMON_LEN + GFAR_EXTRA_STATS_LEN)
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 172acb9..75e89ac 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -149,10 +149,10 @@ static void gfar_fill_stats(struct net_device *dev, struct ethtool_stats *dummy,
 	int i;
 	struct gfar_private *priv = netdev_priv(dev);
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
-	u64 *extra = (u64 *) & priv->extra_stats;
+	atomic64_t *extra = (atomic64_t *)&priv->extra_stats;
 
 	for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
-		buf[i] = extra[i];
+		buf[i] = atomic64_read(&extra[i]);
 
 	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_RMON) {
 		u32 __iomem *rmon = (u32 __iomem *) &regs->rmon;
-- 
1.8.1.2

Re: [PATCH net-next 1/2] gianfar: remove largely unused gfar_stats struct

[Claudiu Manoil]

On 2/13/2013 2:24 AM, Paul Gortmaker wrote:
> The gfar_stats struct is only used in copying out data
> via ethtool.  It is declared as the extra stats, followed
> by the rmon stats.  However, the rmon stats are never
> actually ever used in the driver; instead the rmon data
> is a u32 register read that is cast directly into the
> ethtool buf.
>
> It seems the only reason rmon is in the struct at all is
> to give the offset(s) at which it should be exported into
> the ethtool buffer.  But note gfar_stats doesn't contain
> a gfar_extra_stats as a substruct -- instead it contains
> a u64 array of equal element count.  This implicitly means
> we have two independent declarations of what gfar_extra_stats
> really is.  Rather than have this duality, we already have
> defines which give us the offset directly, and hence do not
> need the struct at all.
>
> Further, since we know the extra_stats is unconditionally
> always present, we can write it out to the ethtool buf
> 1st, and then optionally write out the rmon data.  There
> is no need for two independent loops, both of which are
> simply copying out the extra_stats to buf offset zero.
>
> This also helps pave the way towards allowing the extra
> stats fields to be converted to atomic64_t values, without
> having their types directly influencing the ethtool stats
> export code (gfar_fill_stats) that expects to deal with u64.
>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Reviewed-by: Claudiu Manoil <claudiu.manoil@freescale.com>

> ---
>   drivers/net/ethernet/freescale/gianfar.h         |  8 +-------
>   drivers/net/ethernet/freescale/gianfar_ethtool.c | 15 ++++++---------
>   2 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
> index 71793f4..61b1785 100644
> --- a/drivers/net/ethernet/freescale/gianfar.h
> +++ b/drivers/net/ethernet/freescale/gianfar.h
> @@ -646,15 +646,9 @@ struct gfar_extra_stats {
>   #define GFAR_RMON_LEN ((sizeof(struct rmon_mib) - 16)/sizeof(u32))
>   #define GFAR_EXTRA_STATS_LEN (sizeof(struct gfar_extra_stats)/sizeof(u64))
>
> -/* Number of stats in the stats structure (ignore car and cam regs)*/
> +/* Number of stats exported via ethtool */
>   #define GFAR_STATS_LEN (GFAR_RMON_LEN + GFAR_EXTRA_STATS_LEN)
>
> -struct gfar_stats {
> -	u64 extra[GFAR_EXTRA_STATS_LEN];
> -	u64 rmon[GFAR_RMON_LEN];
> -};
> -
> -
>   struct gfar {
>   	u32	tsec_id;	/* 0x.000 - Controller ID register */
>   	u32	tsec_id2;	/* 0x.004 - Controller ID2 register */
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 45e59d5..172acb9 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -151,18 +151,15 @@ static void gfar_fill_stats(struct net_device *dev, struct ethtool_stats *dummy,
>   	struct gfar __iomem *regs = priv->gfargrp[0].regs;
>   	u64 *extra = (u64 *) & priv->extra_stats;
>
> +	for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
> +		buf[i] = extra[i];
> +
>   	if (priv->device_flags & FSL_GIANFAR_DEV_HAS_RMON) {
>   		u32 __iomem *rmon = (u32 __iomem *) &regs->rmon;
> -		struct gfar_stats *stats = (struct gfar_stats *) buf;
> -
> -		for (i = 0; i < GFAR_RMON_LEN; i++)
> -			stats->rmon[i] = (u64) gfar_read(&rmon[i]);
>
> -		for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
> -			stats->extra[i] = extra[i];
> -	} else
> -		for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
> -			buf[i] = extra[i];
> +		for (; i < GFAR_STATS_LEN; i++, rmon++)
> +			buf[i] = (u64) gfar_read(rmon);
> +	}
>   }
>
>   static int gfar_sset_count(struct net_device *dev, int sset)
>

Re: [PATCH net-next 2/2] gianfar: convert u64 status counters to atomic64_t

[Claudiu Manoil]

On 2/13/2013 2:24 AM, Paul Gortmaker wrote:
> While looking at some asm dump for an unrelated change, Eric
> noticed in the following stats count increment code:
>
>      50b8:       81 3c 01 f8     lwz     r9,504(r28)
>      50bc:       81 5c 01 fc     lwz     r10,508(r28)
>      50c0:       31 4a 00 01     addic   r10,r10,1
>      50c4:       7d 29 01 94     addze   r9,r9
>      50c8:       91 3c 01 f8     stw     r9,504(r28)
>      50cc:       91 5c 01 fc     stw     r10,508(r28)
>
> that a 64 bit counter was used on ppc-32 without sync
> and hence the "ethtool -S" output was racy.
>
> Here we convert all the values to use atomic64_t so that
> the output will always be consistent.
>

At least it seems that this conversion results in fewer asm
instructions, as apparently addze and the double lwz/stw are
not generated anymore. Hopefully it's faster too :P

Reviewed-by: Claudiu Manoil <claudiu.manoil@freescale.com>

Thanks,
Claudiu

Re: [PATCH net-next 2/2] gianfar: convert u64 status counters to atomic64_t

[Eric Dumazet]

On Wed, 2013-02-13 at 17:22 +0200, Claudiu Manoil wrote:

> At least it seems that this conversion results in fewer asm
> instructions, as apparently addze and the double lwz/stw are
> not generated anymore. Hopefully it's faster too :P

Strange, could you show us these asm instructions ?

Re: [PATCH net-next 2/2] gianfar: convert u64 status counters to atomic64_t

[Claudiu Manoil]

On 2/13/2013 6:14 PM, Eric Dumazet wrote:
> On Wed, 2013-02-13 at 17:22 +0200, Claudiu Manoil wrote:
>
>> At least it seems that this conversion results in fewer asm
>> instructions, as apparently addze and the double lwz/stw are
>> not generated anymore. Hopefully it's faster too :P
>
> Strange, could you show us these asm instructions ?
>
>
>

Ok, I'm looking over gfar_clean_rx_ring's asm code, with and w/o this
patch. They are difficult to compare as asm code changed considerably,
the initial version having more lines.
The first thing I notice is that the initial ver has 13 'addic'
instructions, and the new version has 7.

Now taking the code around the last 'addic' instruction (from the
gfar_clean_rx_ring function):

Initial version looks like this:

     5024:	4b ff ff ac 	b       4fd0 <gfar_clean_rx_ring+0x450>
     5028:	81 5d 06 30 	lwz     r10,1584(r29)
     502c:	81 7d 06 34 	lwz     r11,1588(r29)
     5030:	31 6b 00 01 	addic   r11,r11,1
     5034:	7d 4a 01 94 	addze   r10,r10
     5038:	91 5d 06 30 	stw     r10,1584(r29)
     503c:	91 7d 06 34 	stw     r11,1588(r29)
     5040:	4b ff fe fc 	b       4f3c <gfar_clean_rx_ring+0x3bc>

New version looks like this:

     4ff8:	4b ff fd a8 	b       4da0 <gfar_clean_rx_ring+0x1ec>
     4ffc:	80 1c 00 a0 	lwz     r0,160(r28)
     5000:	38 60 00 00 	li      r3,0
     5004:	80 a1 00 18 	lwz     r5,24(r1)
     5008:	38 80 00 01 	li      r4,1
     500c:	30 00 00 01 	addic   r0,r0,1
     5010:	90 1c 00 a0 	stw     r0,160(r28)
     5014:	48 00 00 01 	bl      5014 <gfar_clean_rx_ring+0x460>
     5018:	4b ff ff 4c 	b       4f64 <gfar_clean_rx_ring+0x3b0>


I have the whole function's asm excepts, if needed.

Re: [PATCH net-next 2/2] gianfar: convert u64 status counters to atomic64_t

[Eric Dumazet]

On Wed, 2013-02-13 at 19:47 +0200, Claudiu Manoil wrote:
> On 2/13/2013 6:14 PM, Eric Dumazet wrote:
> > On Wed, 2013-02-13 at 17:22 +0200, Claudiu Manoil wrote:
> >
> >> At least it seems that this conversion results in fewer asm
> >> instructions, as apparently addze and the double lwz/stw are
> >> not generated anymore. Hopefully it's faster too :P
> >
> > Strange, could you show us these asm instructions ?
> >
> >
> >
> 
> Ok, I'm looking over gfar_clean_rx_ring's asm code, with and w/o this
> patch. They are difficult to compare as asm code changed considerably,
> the initial version having more lines.
> The first thing I notice is that the initial ver has 13 'addic'
> instructions, and the new version has 7.
> 
> Now taking the code around the last 'addic' instruction (from the
> gfar_clean_rx_ring function):
> 
> Initial version looks like this:
> 
>      5024:	4b ff ff ac 	b       4fd0 <gfar_clean_rx_ring+0x450>
>      5028:	81 5d 06 30 	lwz     r10,1584(r29)
>      502c:	81 7d 06 34 	lwz     r11,1588(r29)
>      5030:	31 6b 00 01 	addic   r11,r11,1
>      5034:	7d 4a 01 94 	addze   r10,r10
>      5038:	91 5d 06 30 	stw     r10,1584(r29)
>      503c:	91 7d 06 34 	stw     r11,1588(r29)
>      5040:	4b ff fe fc 	b       4f3c <gfar_clean_rx_ring+0x3bc>
> 
> New version looks like this:
> 
>      4ff8:	4b ff fd a8 	b       4da0 <gfar_clean_rx_ring+0x1ec>
>      4ffc:	80 1c 00 a0 	lwz     r0,160(r28)
>      5000:	38 60 00 00 	li      r3,0
>      5004:	80 a1 00 18 	lwz     r5,24(r1)
>      5008:	38 80 00 01 	li      r4,1
>      500c:	30 00 00 01 	addic   r0,r0,1
>      5010:	90 1c 00 a0 	stw     r0,160(r28)
>      5014:	48 00 00 01 	bl      5014 <gfar_clean_rx_ring+0x460>
>      5018:	4b ff ff 4c 	b       4f64 <gfar_clean_rx_ring+0x3b0>
> 
> 
> I have the whole function's asm excepts, if needed.
> 

I guess you are not looking at the right spot.

32bit powerpc probably use the generic atomic64

Your kernel should have an atomic64_add() function (in lib/atomic64.c)
and gianfar should call it.

This is certainly expensive, but these counters are not in fast path.

If they were in fast path, include/linux/u64_stats_sync.h would be a
better choice.

Re: [PATCH net-next 0/2] gianfar: make local stats atomic64

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Tue, 12 Feb 2013 19:24:22 -0500

> Eric noticed that the handling of local u64 ethtool counters for
> this driver commonly found on Freescale ppc-32 boards was racy.
> 
> However, before converting them over to atomic64_t, I noticed
> that an internal struct was being used to determine the offsets
> for exporting this data into the ethtool buffer, and in doing
> so, it assumed that the counters would always be u64.  Rather
> than keep this implicit assumption, a simple code cleanup gets
> rid of the struct completely, and leaves less conversion sites.
> 
> The alternative solution would have been to take advantage of
> the fact that the counters are all relating to error conditions,
> and hence make them internally u32.  In doing so, we'd be assuming
> that U32_MAX of any particular error condition is highly unlikely.
> This might have made sense if any increments were in a hot path.
> 
> Tested with "ethtool -S eth0" on sbc8548 board.
 ...
>   git://git.kernel.org/pub/scm/linux/kernel/git/paulg/linux.git gfar-ethtool-atomic

Pulled, thanks Paul.