drivers/net/ethernet/broadcom/bcmsysport.c | 56 ++++++++++++---------- drivers/net/ethernet/broadcom/bcmsysport.h | 12 ++--- 2 files changed, 37 insertions(+), 31 deletions(-)
On 64bit arches, struct u64_stats_sync is empty and provides no help
against load/store tearing. Convert to u64_stats_t to ensure atomic
operations.
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
RFC Comment:
I couldn't find the lock associated with u64_stats_sync. Should this be
considered an issue?
drivers/net/ethernet/broadcom/bcmsysport.c | 56 ++++++++++++----------
drivers/net/ethernet/broadcom/bcmsysport.h | 12 ++---
2 files changed, 37 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
index bc4e1f3b3752..7017b2d2a1b5 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.c
+++ b/drivers/net/ethernet/broadcom/bcmsysport.c
@@ -435,8 +435,8 @@ static void bcm_sysport_update_tx_stats(struct bcm_sysport_priv *priv,
ring = &priv->tx_rings[q];
do {
start = u64_stats_fetch_begin(&priv->syncp);
- bytes = ring->bytes;
- packets = ring->packets;
+ bytes = u64_stats_read(&ring->bytes);
+ packets = u64_stats_read(&ring->packets);
} while (u64_stats_fetch_retry(&priv->syncp, start));
*tx_bytes += bytes;
@@ -458,8 +458,10 @@ static void bcm_sysport_get_stats(struct net_device *dev,
if (netif_running(dev)) {
bcm_sysport_update_mib_counters(priv);
bcm_sysport_update_tx_stats(priv, &tx_bytes, &tx_packets);
- stats64->tx_bytes = tx_bytes;
- stats64->tx_packets = tx_packets;
+ u64_stats_update_begin(&priv->syncp);
+ u64_stats_set(&stats64->tx_bytes, tx_bytes);
+ u64_stats_set(&stats64->tx_packets, tx_packets);
+ u64_stats_update_end(&priv->syncp);
}
for (i = 0, j = 0; i < BCM_SYSPORT_STATS_LEN; i++) {
@@ -482,28 +484,32 @@ static void bcm_sysport_get_stats(struct net_device *dev,
s->type == BCM_SYSPORT_STAT_NETDEV64) {
do {
start = u64_stats_fetch_begin(syncp);
- data[i] = *(u64 *)p;
+ data[i] = u64_stats_read((u64_stats_t *)p);
} while (u64_stats_fetch_retry(syncp, start));
} else
data[i] = *(u32 *)p;
j++;
}
- /* For SYSTEMPORT Lite since we have holes in our statistics, j would
- * be equal to BCM_SYSPORT_STATS_LEN at the end of the loop, but it
- * needs to point to how many total statistics we have minus the
- * number of per TX queue statistics
- */
- j = bcm_sysport_get_sset_count(dev, ETH_SS_STATS) -
- dev->num_tx_queues * NUM_SYSPORT_TXQ_STAT;
+ do {
+ /* For SYSTEMPORT Lite since we have holes in our statistics, j
+ * would be equal to BCM_SYSPORT_STATS_LEN at the end of the
+ * loop, but it needs to point to how many total statistics we
+ * have minus the number of per TX queue statistics
+ */
+ j = bcm_sysport_get_sset_count(dev, ETH_SS_STATS) -
+ dev->num_tx_queues * NUM_SYSPORT_TXQ_STAT;
- for (i = 0; i < dev->num_tx_queues; i++) {
- ring = &priv->tx_rings[i];
- data[j] = ring->packets;
- j++;
- data[j] = ring->bytes;
- j++;
- }
+ start = u64_stats_fetch_begin(syncp);
+
+ for (i = 0; i < dev->num_tx_queues; i++) {
+ ring = &priv->tx_rings[i];
+ data[j] = u64_stats_read(&ring->packets);
+ j++;
+ data[j] = u64_stats_read(&ring->bytes);
+ j++;
+ }
+ } while (u64_stats_fetch_retry(syncp, start));
}
static void bcm_sysport_get_wol(struct net_device *dev,
@@ -829,8 +835,8 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
ndev->stats.rx_packets++;
ndev->stats.rx_bytes += len;
u64_stats_update_begin(&priv->syncp);
- stats64->rx_packets++;
- stats64->rx_bytes += len;
+ u64_stats_inc(&stats64->rx_packets);
+ u64_stats_add(&stats64->rx_bytes, len);
u64_stats_update_end(&priv->syncp);
napi_gro_receive(&priv->napi, skb);
@@ -914,8 +920,8 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
}
u64_stats_update_begin(&priv->syncp);
- ring->packets += pkts_compl;
- ring->bytes += bytes_compl;
+ u64_stats_add(&ring->packets, pkts_compl);
+ u64_stats_add(&ring->bytes, bytes_compl);
u64_stats_update_end(&priv->syncp);
ring->c_index = c_index;
@@ -1857,8 +1863,8 @@ static void bcm_sysport_get_stats64(struct net_device *dev,
do {
start = u64_stats_fetch_begin(&priv->syncp);
- stats->rx_packets = stats64->rx_packets;
- stats->rx_bytes = stats64->rx_bytes;
+ stats->rx_packets = u64_stats_read(&stats64->rx_packets);
+ stats->rx_bytes = u64_stats_read(&stats64->rx_bytes);
} while (u64_stats_fetch_retry(&priv->syncp, start));
}
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
index a34296f989f1..91b1c8293a23 100644
--- a/drivers/net/ethernet/broadcom/bcmsysport.h
+++ b/drivers/net/ethernet/broadcom/bcmsysport.h
@@ -652,10 +652,10 @@ struct bcm_sysport_stats {
struct bcm_sysport_stats64 {
/* 64bit stats on 32bit/64bit Machine */
- u64 rx_packets;
- u64 rx_bytes;
- u64 tx_packets;
- u64 tx_bytes;
+ u64_stats_t rx_packets;
+ u64_stats_t rx_bytes;
+ u64_stats_t tx_packets;
+ u64_stats_t tx_bytes;
};
/* Software house keeping helper structure */
@@ -698,8 +698,8 @@ struct bcm_sysport_tx_ring {
unsigned int clean_index; /* Current clean index */
struct bcm_sysport_cb *cbs; /* Transmit control blocks */
struct bcm_sysport_priv *priv; /* private context backpointer */
- unsigned long packets; /* packets statistics */
- unsigned long bytes; /* bytes statistics */
+ u64_stats_t packets; /* packets statistics */
+ u64_stats_t bytes; /* bytes statistics */
unsigned int switch_queue; /* switch port queue number */
unsigned int switch_port; /* switch port queue number */
bool inspect; /* inspect switch port and queue */
--
2.51.0On 22/01/2026 20:15, David Yang wrote:
> On 64bit arches, struct u64_stats_sync is empty and provides no help
> against load/store tearing. Convert to u64_stats_t to ensure atomic
> operations.
>
> Signed-off-by: David Yang <mmyangfl@gmail.com>
> ---
> RFC Comment:
>
> I couldn't find the lock associated with u64_stats_sync. Should this be
> considered an issue?
Didn't get the question. u64_stats_sync structure has it's own lock for
cases where atomic operations are not possible.
> drivers/net/ethernet/broadcom/bcmsysport.c | 56 ++++++++++++----------
> drivers/net/ethernet/broadcom/bcmsysport.h | 12 ++---
> 2 files changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index bc4e1f3b3752..7017b2d2a1b5 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
> @@ -435,8 +435,8 @@ static void bcm_sysport_update_tx_stats(struct bcm_sysport_priv *priv,
> ring = &priv->tx_rings[q];
> do {
> start = u64_stats_fetch_begin(&priv->syncp);
> - bytes = ring->bytes;
> - packets = ring->packets;
> + bytes = u64_stats_read(&ring->bytes);
> + packets = u64_stats_read(&ring->packets);
> } while (u64_stats_fetch_retry(&priv->syncp, start));
please, look the comment below which includes reference to this block...
>
> *tx_bytes += bytes;
> @@ -458,8 +458,10 @@ static void bcm_sysport_get_stats(struct net_device *dev,
> if (netif_running(dev)) {
> bcm_sysport_update_mib_counters(priv);
> bcm_sysport_update_tx_stats(priv, &tx_bytes, &tx_packets);
> - stats64->tx_bytes = tx_bytes;
> - stats64->tx_packets = tx_packets;
> + u64_stats_update_begin(&priv->syncp);
> + u64_stats_set(&stats64->tx_bytes, tx_bytes);
> + u64_stats_set(&stats64->tx_packets, tx_packets);
> + u64_stats_update_end(&priv->syncp);
> }
>
> for (i = 0, j = 0; i < BCM_SYSPORT_STATS_LEN; i++) {
> @@ -482,28 +484,32 @@ static void bcm_sysport_get_stats(struct net_device *dev,
> s->type == BCM_SYSPORT_STAT_NETDEV64) {
> do {
> start = u64_stats_fetch_begin(syncp);
> - data[i] = *(u64 *)p;
> + data[i] = u64_stats_read((u64_stats_t *)p);
> } while (u64_stats_fetch_retry(syncp, start));
> } else
> data[i] = *(u32 *)p;
> j++;
> }
>
> - /* For SYSTEMPORT Lite since we have holes in our statistics, j would
> - * be equal to BCM_SYSPORT_STATS_LEN at the end of the loop, but it
> - * needs to point to how many total statistics we have minus the
> - * number of per TX queue statistics
> - */
> - j = bcm_sysport_get_sset_count(dev, ETH_SS_STATS) -
> - dev->num_tx_queues * NUM_SYSPORT_TXQ_STAT;
> + do {
> + /* For SYSTEMPORT Lite since we have holes in our statistics, j
> + * would be equal to BCM_SYSPORT_STATS_LEN at the end of the
> + * loop, but it needs to point to how many total statistics we
> + * have minus the number of per TX queue statistics
> + */
> + j = bcm_sysport_get_sset_count(dev, ETH_SS_STATS) -
> + dev->num_tx_queues * NUM_SYSPORT_TXQ_STAT;
>
> - for (i = 0; i < dev->num_tx_queues; i++) {
> - ring = &priv->tx_rings[i];
> - data[j] = ring->packets;
> - j++;
> - data[j] = ring->bytes;
> - j++;
> - }
> + start = u64_stats_fetch_begin(syncp);
> +
> + for (i = 0; i < dev->num_tx_queues; i++) {
> + ring = &priv->tx_rings[i];
> + data[j] = u64_stats_read(&ring->packets);
> + j++;
> + data[j] = u64_stats_read(&ring->bytes);
> + j++;
> + }
> + } while (u64_stats_fetch_retry(syncp, start));
ring is protected by ring::lock, there is no need to introduce another
per-device lock. ring statistics can be protected by the same lock, and
to improve the code, ring::lock can be converted into seqlock_t.
> }
>
> static void bcm_sysport_get_wol(struct net_device *dev,
> @@ -829,8 +835,8 @@ static unsigned int bcm_sysport_desc_rx(struct bcm_sysport_priv *priv,
> ndev->stats.rx_packets++;
> ndev->stats.rx_bytes += len;
> u64_stats_update_begin(&priv->syncp);
> - stats64->rx_packets++;
> - stats64->rx_bytes += len;
> + u64_stats_inc(&stats64->rx_packets);
> + u64_stats_add(&stats64->rx_bytes, len);
> u64_stats_update_end(&priv->syncp);
>
> napi_gro_receive(&priv->napi, skb);
> @@ -914,8 +920,8 @@ static unsigned int __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
> }
>
> u64_stats_update_begin(&priv->syncp);
> - ring->packets += pkts_compl;
> - ring->bytes += bytes_compl;
> + u64_stats_add(&ring->packets, pkts_compl);
> + u64_stats_add(&ring->bytes, bytes_compl);
> u64_stats_update_end(&priv->syncp);
here again, no need for device-wide lock in hot path.
>
> ring->c_index = c_index;
> @@ -1857,8 +1863,8 @@ static void bcm_sysport_get_stats64(struct net_device *dev,
>
> do {
> start = u64_stats_fetch_begin(&priv->syncp);
> - stats->rx_packets = stats64->rx_packets;
> - stats->rx_bytes = stats64->rx_bytes;
> + stats->rx_packets = u64_stats_read(&stats64->rx_packets);
> + stats->rx_bytes = u64_stats_read(&stats64->rx_bytes);
> } while (u64_stats_fetch_retry(&priv->syncp, start));
> }
>
> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h
> index a34296f989f1..91b1c8293a23 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.h
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.h
> @@ -652,10 +652,10 @@ struct bcm_sysport_stats {
>
> struct bcm_sysport_stats64 {
> /* 64bit stats on 32bit/64bit Machine */
> - u64 rx_packets;
> - u64 rx_bytes;
> - u64 tx_packets;
> - u64 tx_bytes;
> + u64_stats_t rx_packets;
> + u64_stats_t rx_bytes;
> + u64_stats_t tx_packets;
> + u64_stats_t tx_bytes;
> };
>
> /* Software house keeping helper structure */
> @@ -698,8 +698,8 @@ struct bcm_sysport_tx_ring {
> unsigned int clean_index; /* Current clean index */
> struct bcm_sysport_cb *cbs; /* Transmit control blocks */
> struct bcm_sysport_priv *priv; /* private context backpointer */
> - unsigned long packets; /* packets statistics */
> - unsigned long bytes; /* bytes statistics */
> + u64_stats_t packets; /* packets statistics */
> + u64_stats_t bytes; /* bytes statistics */
> unsigned int switch_queue; /* switch port queue number */
> unsigned int switch_port; /* switch port queue number */
> bool inspect; /* inspect switch port and queue */
© 2016 - 2026 Red Hat, Inc.