.../ethernet/freescale/dpaa/dpaa_ethtool.c | 40 ++++++------------- .../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 15 +++---- .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 7 +--- .../freescale/dpaa2/dpaa2-switch-ethtool.c | 9 ++--- .../ethernet/freescale/enetc/enetc_ethtool.c | 35 +++++----------- .../net/ethernet/freescale/gianfar_ethtool.c | 8 ++-- .../net/ethernet/freescale/ucc_geth_ethtool.c | 21 +++++----- 7 files changed, 49 insertions(+), 86 deletions(-)
The latter is the preferred way to copy ethtool strings.
Avoids manually incrementing the pointer. Cleans up the code quite well.
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
.../ethernet/freescale/dpaa/dpaa_ethtool.c | 40 ++++++-------------
.../ethernet/freescale/dpaa2/dpaa2-ethtool.c | 15 +++----
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 7 +---
.../freescale/dpaa2/dpaa2-switch-ethtool.c | 9 ++---
.../ethernet/freescale/enetc/enetc_ethtool.c | 35 +++++-----------
.../net/ethernet/freescale/gianfar_ethtool.c | 8 ++--
.../net/ethernet/freescale/ucc_geth_ethtool.c | 21 +++++-----
7 files changed, 49 insertions(+), 86 deletions(-)
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
index b0060cf96090..10c5fa4d23d2 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
@@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
u8 *data)
{
- unsigned int i, j, num_cpus, size;
- char string_cpu[ETH_GSTRING_LEN];
- u8 *strings;
+ unsigned int i, j, num_cpus;
- memset(string_cpu, 0, sizeof(string_cpu));
- strings = data;
- num_cpus = num_online_cpus();
- size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
+ num_cpus = num_online_cpus();
for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
- for (j = 0; j < num_cpus; j++) {
- snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
- dpaa_stats_percpu[i], j);
- memcpy(strings, string_cpu, ETH_GSTRING_LEN);
- strings += ETH_GSTRING_LEN;
- }
- snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
- dpaa_stats_percpu[i]);
- memcpy(strings, string_cpu, ETH_GSTRING_LEN);
- strings += ETH_GSTRING_LEN;
- }
- for (j = 0; j < num_cpus; j++) {
- snprintf(string_cpu, ETH_GSTRING_LEN,
- "bpool [CPU %d]", j);
- memcpy(strings, string_cpu, ETH_GSTRING_LEN);
- strings += ETH_GSTRING_LEN;
+ for (j = 0; j < num_cpus; j++)
+ ethtool_sprintf(&data, "%s [CPU %d]",
+ dpaa_stats_percpu[i], j);
+
+ ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
}
- snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
- memcpy(strings, string_cpu, ETH_GSTRING_LEN);
- strings += ETH_GSTRING_LEN;
+ for (i = 0; j < num_cpus; i++)
+ ethtool_sprintf(&data, "bpool [CPU %d]", i);
+
+ ethtool_puts(&data, "bpool [TOTAL]");
- memcpy(strings, dpaa_stats_global, size);
+ for (i = 0; i < DPAA_STATS_GLOBAL_LEN; i++)
+ ethtool_puts(&data, dpaa_stats_global[i]);
}
static int dpaa_get_hash_opts(struct net_device *dev,
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index 7f476519b7ad..fd05dd12f107 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -217,20 +217,15 @@ static int dpaa2_eth_set_pauseparam(struct net_device *net_dev,
static void dpaa2_eth_get_strings(struct net_device *netdev, u32 stringset,
u8 *data)
{
- u8 *p = data;
int i;
switch (stringset) {
case ETH_SS_STATS:
- for (i = 0; i < DPAA2_ETH_NUM_STATS; i++) {
- strscpy(p, dpaa2_ethtool_stats[i], ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
- }
- for (i = 0; i < DPAA2_ETH_NUM_EXTRA_STATS; i++) {
- strscpy(p, dpaa2_ethtool_extras[i], ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
- }
- dpaa2_mac_get_strings(p);
+ for (i = 0; i < DPAA2_ETH_NUM_STATS; i++)
+ ethtool_puts(&data, dpaa2_ethtool_stats[i]);
+ for (i = 0; i < DPAA2_ETH_NUM_EXTRA_STATS; i++)
+ ethtool_puts(&data, dpaa2_ethtool_extras[i]);
+ dpaa2_mac_get_strings(data);
break;
}
}
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index a69bb22c37ea..892ed2f91084 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -560,13 +560,10 @@ int dpaa2_mac_get_sset_count(void)
void dpaa2_mac_get_strings(u8 *data)
{
- u8 *p = data;
int i;
- for (i = 0; i < DPAA2_MAC_NUM_STATS; i++) {
- strscpy(p, dpaa2_mac_ethtool_stats[i], ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
- }
+ for (i = 0; i < DPAA2_MAC_NUM_STATS; i++)
+ ethtool_puts(&data, dpaa2_mac_ethtool_stats[i]);
}
void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
index 6bc1988be311..cdcf03c8c552 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch-ethtool.c
@@ -170,17 +170,16 @@ dpaa2_switch_ethtool_get_sset_count(struct net_device *netdev, int sset)
static void dpaa2_switch_ethtool_get_strings(struct net_device *netdev,
u32 stringset, u8 *data)
{
- u8 *p = data;
+ const char *str;
int i;
switch (stringset) {
case ETH_SS_STATS:
for (i = 0; i < DPAA2_SWITCH_NUM_COUNTERS; i++) {
- memcpy(p, dpaa2_switch_ethtool_counters[i].name,
- ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
+ str = dpaa2_switch_ethtool_counters[i].name;
+ ethtool_puts(&data, str);
}
- dpaa2_mac_get_strings(p);
+ dpaa2_mac_get_strings(data);
break;
}
}
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 2563eb8ac7b6..e1745b89362d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -247,38 +247,25 @@ static int enetc_get_sset_count(struct net_device *ndev, int sset)
static void enetc_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
{
struct enetc_ndev_priv *priv = netdev_priv(ndev);
- u8 *p = data;
int i, j;
switch (stringset) {
case ETH_SS_STATS:
- for (i = 0; i < ARRAY_SIZE(enetc_si_counters); i++) {
- strscpy(p, enetc_si_counters[i].name, ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
- }
- for (i = 0; i < priv->num_tx_rings; i++) {
- for (j = 0; j < ARRAY_SIZE(tx_ring_stats); j++) {
- snprintf(p, ETH_GSTRING_LEN, tx_ring_stats[j],
- i);
- p += ETH_GSTRING_LEN;
- }
- }
- for (i = 0; i < priv->num_rx_rings; i++) {
- for (j = 0; j < ARRAY_SIZE(rx_ring_stats); j++) {
- snprintf(p, ETH_GSTRING_LEN, rx_ring_stats[j],
- i);
- p += ETH_GSTRING_LEN;
- }
- }
+ for (i = 0; i < ARRAY_SIZE(enetc_si_counters); i++)
+ ethtool_puts(&data, enetc_si_counters[i].name);
+ for (i = 0; i < priv->num_tx_rings; i++)
+ for (j = 0; j < ARRAY_SIZE(tx_ring_stats); j++)
+ ethtool_sprintf(&data, tx_ring_stats[j], i);
+ for (i = 0; i < priv->num_rx_rings; i++)
+ for (j = 0; j < ARRAY_SIZE(rx_ring_stats); j++)
+ ethtool_sprintf(&data, rx_ring_stats[j], i);
if (!enetc_si_is_pf(priv->si))
break;
- for (i = 0; i < ARRAY_SIZE(enetc_port_counters); i++) {
- strscpy(p, enetc_port_counters[i].name,
- ETH_GSTRING_LEN);
- p += ETH_GSTRING_LEN;
- }
+ for (i = 0; i < ARRAY_SIZE(enetc_port_counters); i++)
+ ethtool_puts(&data, enetc_port_counters[i].name);
+
break;
}
}
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index a99b95c4bcfb..781d92e703cb 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -115,12 +115,14 @@ static const char stat_gstrings[][ETH_GSTRING_LEN] = {
static void gfar_gstrings(struct net_device *dev, u32 stringset, u8 * buf)
{
struct gfar_private *priv = netdev_priv(dev);
+ int i;
if (priv->device_flags & FSL_GIANFAR_DEV_HAS_RMON)
- memcpy(buf, stat_gstrings, GFAR_STATS_LEN * ETH_GSTRING_LEN);
+ for (i = 0; i < GFAR_STATS_LEN; i++)
+ ethtool_puts(&buf, stat_gstrings[i]);
else
- memcpy(buf, stat_gstrings,
- GFAR_EXTRA_STATS_LEN * ETH_GSTRING_LEN);
+ for (i = 0; i < GFAR_EXTRA_STATS_LEN; i++)
+ ethtool_puts(&buf, stat_gstrings[i]);
}
/* Fill in an array of 64-bit statistics from various sources.
diff --git a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c
index 601beb93d3b3..699f346faf5c 100644
--- a/drivers/net/ethernet/freescale/ucc_geth_ethtool.c
+++ b/drivers/net/ethernet/freescale/ucc_geth_ethtool.c
@@ -287,20 +287,17 @@ static void uec_get_strings(struct net_device *netdev, u32 stringset, u8 *buf)
{
struct ucc_geth_private *ugeth = netdev_priv(netdev);
u32 stats_mode = ugeth->ug_info->statisticsMode;
+ int i;
- if (stats_mode & UCC_GETH_STATISTICS_GATHERING_MODE_HARDWARE) {
- memcpy(buf, hw_stat_gstrings, UEC_HW_STATS_LEN *
- ETH_GSTRING_LEN);
- buf += UEC_HW_STATS_LEN * ETH_GSTRING_LEN;
- }
- if (stats_mode & UCC_GETH_STATISTICS_GATHERING_MODE_FIRMWARE_TX) {
- memcpy(buf, tx_fw_stat_gstrings, UEC_TX_FW_STATS_LEN *
- ETH_GSTRING_LEN);
- buf += UEC_TX_FW_STATS_LEN * ETH_GSTRING_LEN;
- }
+ if (stats_mode & UCC_GETH_STATISTICS_GATHERING_MODE_HARDWARE)
+ for (i = 0; i < UEC_HW_STATS_LEN; i++)
+ ethtool_puts(&buf, hw_stat_gstrings[i]);
+ if (stats_mode & UCC_GETH_STATISTICS_GATHERING_MODE_FIRMWARE_TX)
+ for (i = 0; i < UEC_TX_FW_STATS_LEN; i++)
+ ethtool_puts(&buf, tx_fw_stat_gstrings[i]);
if (stats_mode & UCC_GETH_STATISTICS_GATHERING_MODE_FIRMWARE_RX)
- memcpy(buf, rx_fw_stat_gstrings, UEC_RX_FW_STATS_LEN *
- ETH_GSTRING_LEN);
+ for (i = 0; i < UEC_RX_FW_STATS_LEN; i++)
+ ethtool_puts(&buf, rx_fw_stat_gstrings[i]);
}
static void uec_get_ethtool_stats(struct net_device *netdev,
--
2.47.0Hi Rosen,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Rosen-Penev/net-freescale-use-ethtool-string-helpers/20241025-045447
base: net-next/main
patch link: https://lore.kernel.org/r/20241024205257.574836-1-rosenp%40gmail.com
patch subject: [PATCH net-next] net: freescale: use ethtool string helpers
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241025/202410252249.mmE2EZPz-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 5886454669c3c9026f7f27eab13509dd0241f2d6)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410252249.mmE2EZPz-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410252249.mmE2EZPz-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c:11:
In file included from include/linux/platform_device.h:13:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:181:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c:13:
In file included from include/linux/fsl/ptp_qoriq.h:9:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:95:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c:13:
In file included from include/linux/fsl/ptp_qoriq.h:9:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:95:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c:13:
In file included from include/linux/fsl/ptp_qoriq.h:9:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:95:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
>> drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c:257:14: warning: variables 'j' and 'num_cpus' used in loop condition not modified in loop body [-Wfor-loop-analysis]
257 | for (i = 0; j < num_cpus; i++)
| ^ ~~~~~~~~
17 warnings generated.
vim +257 drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
242
243 static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
244 u8 *data)
245 {
246 unsigned int i, j, num_cpus;
247
248 num_cpus = num_online_cpus();
249
250 for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
251 for (j = 0; j < num_cpus; j++)
252 ethtool_sprintf(&data, "%s [CPU %d]",
253 dpaa_stats_percpu[i], j);
254
255 ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
256 }
> 257 for (i = 0; j < num_cpus; i++)
258 ethtool_sprintf(&data, "bpool [CPU %d]", i);
259
260 ethtool_puts(&data, "bpool [TOTAL]");
261
262 for (i = 0; i < DPAA_STATS_GLOBAL_LEN; i++)
263 ethtool_puts(&data, dpaa_stats_global[i]);
264 }
265
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, Oct 24, 2024 at 01:52:57PM -0700, Rosen Penev wrote:
> The latter is the preferred way to copy ethtool strings.
>
> Avoids manually incrementing the pointer. Cleans up the code quite well.
>
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
...
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> index b0060cf96090..10c5fa4d23d2 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> @@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
> static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
> u8 *data)
> {
> - unsigned int i, j, num_cpus, size;
> - char string_cpu[ETH_GSTRING_LEN];
> - u8 *strings;
> + unsigned int i, j, num_cpus;
>
> - memset(string_cpu, 0, sizeof(string_cpu));
> - strings = data;
> - num_cpus = num_online_cpus();
> - size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> + num_cpus = num_online_cpus();
>
> for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
> - for (j = 0; j < num_cpus; j++) {
> - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> - dpaa_stats_percpu[i], j);
> - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> - strings += ETH_GSTRING_LEN;
> - }
> - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> - dpaa_stats_percpu[i]);
> - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> - strings += ETH_GSTRING_LEN;
> - }
> - for (j = 0; j < num_cpus; j++) {
> - snprintf(string_cpu, ETH_GSTRING_LEN,
> - "bpool [CPU %d]", j);
> - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> - strings += ETH_GSTRING_LEN;
> + for (j = 0; j < num_cpus; j++)
> + ethtool_sprintf(&data, "%s [CPU %d]",
> + dpaa_stats_percpu[i], j);
> +
> + ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
> }
> - snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
> - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> - strings += ETH_GSTRING_LEN;
> + for (i = 0; j < num_cpus; i++)
Perhaps this should consistently use i, rather than i and j:
for (i = 0; i < num_cpus; i++)
Flagged by W=1 builds with clang-18.
> + ethtool_sprintf(&data, "bpool [CPU %d]", i);
> +
> + ethtool_puts(&data, "bpool [TOTAL]");
>
> - memcpy(strings, dpaa_stats_global, size);
> + for (i = 0; i < DPAA_STATS_GLOBAL_LEN; i++)
> + ethtool_puts(&data, dpaa_stats_global[i]);
> }
>
> static int dpaa_get_hash_opts(struct net_device *dev,
...
On Fri, Oct 25, 2024 at 5:57 AM Simon Horman <horms@kernel.org> wrote:
>
> On Thu, Oct 24, 2024 at 01:52:57PM -0700, Rosen Penev wrote:
> > The latter is the preferred way to copy ethtool strings.
> >
> > Avoids manually incrementing the pointer. Cleans up the code quite well.
> >
> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>
> ...
>
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > index b0060cf96090..10c5fa4d23d2 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > @@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
> > static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
> > u8 *data)
> > {
> > - unsigned int i, j, num_cpus, size;
> > - char string_cpu[ETH_GSTRING_LEN];
> > - u8 *strings;
> > + unsigned int i, j, num_cpus;
> >
> > - memset(string_cpu, 0, sizeof(string_cpu));
> > - strings = data;
> > - num_cpus = num_online_cpus();
> > - size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> > + num_cpus = num_online_cpus();
> >
> > for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
> > - for (j = 0; j < num_cpus; j++) {
> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> > - dpaa_stats_percpu[i], j);
> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > - strings += ETH_GSTRING_LEN;
> > - }
> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> > - dpaa_stats_percpu[i]);
> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > - strings += ETH_GSTRING_LEN;
> > - }
> > - for (j = 0; j < num_cpus; j++) {
> > - snprintf(string_cpu, ETH_GSTRING_LEN,
> > - "bpool [CPU %d]", j);
> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > - strings += ETH_GSTRING_LEN;
> > + for (j = 0; j < num_cpus; j++)
> > + ethtool_sprintf(&data, "%s [CPU %d]",
> > + dpaa_stats_percpu[i], j);
> > +
> > + ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
> > }
> > - snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > - strings += ETH_GSTRING_LEN;
> > + for (i = 0; j < num_cpus; i++)
>
> Perhaps this should consistently use i, rather than i and j:
>
> for (i = 0; i < num_cpus; i++)
>
> Flagged by W=1 builds with clang-18.
I really need to compile test this on a PPC system.
>
> > + ethtool_sprintf(&data, "bpool [CPU %d]", i);
> > +
> > + ethtool_puts(&data, "bpool [TOTAL]");
> >
> > - memcpy(strings, dpaa_stats_global, size);
> > + for (i = 0; i < DPAA_STATS_GLOBAL_LEN; i++)
> > + ethtool_puts(&data, dpaa_stats_global[i]);
> > }
> >
> > static int dpaa_get_hash_opts(struct net_device *dev,
>
> ...
Rosen Penev <rosenp@gmail.com> writes:
> On Fri, Oct 25, 2024 at 5:57 AM Simon Horman <horms@kernel.org> wrote:
>>
>> On Thu, Oct 24, 2024 at 01:52:57PM -0700, Rosen Penev wrote:
>> > The latter is the preferred way to copy ethtool strings.
>> >
>> > Avoids manually incrementing the pointer. Cleans up the code quite well.
>> >
>> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
>>
>> ...
>>
>> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
>> > index b0060cf96090..10c5fa4d23d2 100644
>> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
>> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
>> > @@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
>> > static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
>> > u8 *data)
>> > {
>> > - unsigned int i, j, num_cpus, size;
>> > - char string_cpu[ETH_GSTRING_LEN];
>> > - u8 *strings;
>> > + unsigned int i, j, num_cpus;
>> >
>> > - memset(string_cpu, 0, sizeof(string_cpu));
>> > - strings = data;
>> > - num_cpus = num_online_cpus();
>> > - size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
>> > + num_cpus = num_online_cpus();
>> >
>> > for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
>> > - for (j = 0; j < num_cpus; j++) {
>> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
>> > - dpaa_stats_percpu[i], j);
>> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
>> > - strings += ETH_GSTRING_LEN;
>> > - }
>> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
>> > - dpaa_stats_percpu[i]);
>> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
>> > - strings += ETH_GSTRING_LEN;
>> > - }
>> > - for (j = 0; j < num_cpus; j++) {
>> > - snprintf(string_cpu, ETH_GSTRING_LEN,
>> > - "bpool [CPU %d]", j);
>> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
>> > - strings += ETH_GSTRING_LEN;
>> > + for (j = 0; j < num_cpus; j++)
>> > + ethtool_sprintf(&data, "%s [CPU %d]",
>> > + dpaa_stats_percpu[i], j);
>> > +
>> > + ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
>> > }
>> > - snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
>> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
>> > - strings += ETH_GSTRING_LEN;
>> > + for (i = 0; j < num_cpus; i++)
>>
>> Perhaps this should consistently use i, rather than i and j:
>>
>> for (i = 0; i < num_cpus; i++)
>>
>> Flagged by W=1 builds with clang-18.
> I really need to compile test this on a PPC system.
Cross compiling should be sufficient.
There's some pointers here:
https://github.com/linuxppc/wiki/wiki/Building-powerpc-kernels
Or there's also libc-less cross compilers on kernel.org, eg:
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/14.2.0/x86_64-gcc-14.2.0-nolibc-powerpc64-linux.tar.xz
cheers
On Sun, Oct 27, 2024 at 7:32 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Rosen Penev <rosenp@gmail.com> writes:
> > On Fri, Oct 25, 2024 at 5:57 AM Simon Horman <horms@kernel.org> wrote:
> >>
> >> On Thu, Oct 24, 2024 at 01:52:57PM -0700, Rosen Penev wrote:
> >> > The latter is the preferred way to copy ethtool strings.
> >> >
> >> > Avoids manually incrementing the pointer. Cleans up the code quite well.
> >> >
> >> > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> >>
> >> ...
> >>
> >> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> >> > index b0060cf96090..10c5fa4d23d2 100644
> >> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> >> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> >> > @@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
> >> > static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
> >> > u8 *data)
> >> > {
> >> > - unsigned int i, j, num_cpus, size;
> >> > - char string_cpu[ETH_GSTRING_LEN];
> >> > - u8 *strings;
> >> > + unsigned int i, j, num_cpus;
> >> >
> >> > - memset(string_cpu, 0, sizeof(string_cpu));
> >> > - strings = data;
> >> > - num_cpus = num_online_cpus();
> >> > - size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> >> > + num_cpus = num_online_cpus();
> >> >
> >> > for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
> >> > - for (j = 0; j < num_cpus; j++) {
> >> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> >> > - dpaa_stats_percpu[i], j);
> >> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> >> > - strings += ETH_GSTRING_LEN;
> >> > - }
> >> > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> >> > - dpaa_stats_percpu[i]);
> >> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> >> > - strings += ETH_GSTRING_LEN;
> >> > - }
> >> > - for (j = 0; j < num_cpus; j++) {
> >> > - snprintf(string_cpu, ETH_GSTRING_LEN,
> >> > - "bpool [CPU %d]", j);
> >> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> >> > - strings += ETH_GSTRING_LEN;
> >> > + for (j = 0; j < num_cpus; j++)
> >> > + ethtool_sprintf(&data, "%s [CPU %d]",
> >> > + dpaa_stats_percpu[i], j);
> >> > +
> >> > + ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
> >> > }
> >> > - snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
> >> > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> >> > - strings += ETH_GSTRING_LEN;
> >> > + for (i = 0; j < num_cpus; i++)
> >>
> >> Perhaps this should consistently use i, rather than i and j:
> >>
> >> for (i = 0; i < num_cpus; i++)
> >>
> >> Flagged by W=1 builds with clang-18.
>
> > I really need to compile test this on a PPC system.
>
> Cross compiling should be sufficient.
>
> There's some pointers here:
> https://github.com/linuxppc/wiki/wiki/Building-powerpc-kernels
>
> Or there's also libc-less cross compilers on kernel.org, eg:
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/14.2.0/x86_64-gcc-14.2.0-nolibc-powerpc64-linux.tar.xz
I ended up building linux on cfarm.
>
>
> cheers
On Fri, Oct 25, 2024 at 12:32:27PM -0700, Rosen Penev wrote:
> On Fri, Oct 25, 2024 at 5:57 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Thu, Oct 24, 2024 at 01:52:57PM -0700, Rosen Penev wrote:
> > > The latter is the preferred way to copy ethtool strings.
> > >
> > > Avoids manually incrementing the pointer. Cleans up the code quite well.
> > >
> > > Signed-off-by: Rosen Penev <rosenp@gmail.com>
> >
> > ...
> >
> > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > > index b0060cf96090..10c5fa4d23d2 100644
> > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_ethtool.c
> > > @@ -243,38 +243,24 @@ static void dpaa_get_ethtool_stats(struct net_device *net_dev,
> > > static void dpaa_get_strings(struct net_device *net_dev, u32 stringset,
> > > u8 *data)
> > > {
> > > - unsigned int i, j, num_cpus, size;
> > > - char string_cpu[ETH_GSTRING_LEN];
> > > - u8 *strings;
> > > + unsigned int i, j, num_cpus;
> > >
> > > - memset(string_cpu, 0, sizeof(string_cpu));
> > > - strings = data;
> > > - num_cpus = num_online_cpus();
> > > - size = DPAA_STATS_GLOBAL_LEN * ETH_GSTRING_LEN;
> > > + num_cpus = num_online_cpus();
> > >
> > > for (i = 0; i < DPAA_STATS_PERCPU_LEN; i++) {
> > > - for (j = 0; j < num_cpus; j++) {
> > > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [CPU %d]",
> > > - dpaa_stats_percpu[i], j);
> > > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > > - strings += ETH_GSTRING_LEN;
> > > - }
> > > - snprintf(string_cpu, ETH_GSTRING_LEN, "%s [TOTAL]",
> > > - dpaa_stats_percpu[i]);
> > > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > > - strings += ETH_GSTRING_LEN;
> > > - }
> > > - for (j = 0; j < num_cpus; j++) {
> > > - snprintf(string_cpu, ETH_GSTRING_LEN,
> > > - "bpool [CPU %d]", j);
> > > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > > - strings += ETH_GSTRING_LEN;
> > > + for (j = 0; j < num_cpus; j++)
> > > + ethtool_sprintf(&data, "%s [CPU %d]",
> > > + dpaa_stats_percpu[i], j);
> > > +
> > > + ethtool_sprintf(&data, "%s [TOTAL]", dpaa_stats_percpu[i]);
> > > }
> > > - snprintf(string_cpu, ETH_GSTRING_LEN, "bpool [TOTAL]");
> > > - memcpy(strings, string_cpu, ETH_GSTRING_LEN);
> > > - strings += ETH_GSTRING_LEN;
> > > + for (i = 0; j < num_cpus; i++)
> >
> > Perhaps this should consistently use i, rather than i and j:
> >
> > for (i = 0; i < num_cpus; i++)
> >
> > Flagged by W=1 builds with clang-18.
> I really need to compile test this on a PPC system.
Depending on your aims and hardware availability,
cross compiling may be easier.
But in any case, I don't think this problem relates to PPC.
> >
> > > + ethtool_sprintf(&data, "bpool [CPU %d]", i);
> > > +
> > > + ethtool_puts(&data, "bpool [TOTAL]");
> > >
> > > - memcpy(strings, dpaa_stats_global, size);
> > > + for (i = 0; i < DPAA_STATS_GLOBAL_LEN; i++)
> > > + ethtool_puts(&data, dpaa_stats_global[i]);
> > > }
> > >
> > > static int dpaa_get_hash_opts(struct net_device *dev,
> >
> > ...
>
© 2016 - 2024 Red Hat, Inc.