From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: Rosen Penev <rosenp@gmail.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>,
<netdev@vger.kernel.org>, "Andrew Lunn" <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Jesper Dangaard Brouer <hawk@kernel.org>,
"John Fastabend" <john.fastabend@gmail.com>,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@lists.osuosl.org>,
open list <linux-kernel@vger.kernel.org>,
"open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_)"
<bpf@vger.kernel.org>
Subject: Re: [PATCHv2 net-next iwl-next] net: intel: use ethtool string helpers
Date: Mon, 28 Oct 2024 11:12:49 +0100 [thread overview]
Message-ID: <ca89f03e-6dc1-44fa-bfd1-aac95ede0cbe@intel.com> (raw)
In-Reply-To: <20241025201713.286074-1-rosenp@gmail.com>
On 10/25/24 22:17, 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>
> ---
> v2: add iwl-next tag. use inline int in for loops.
> .../net/ethernet/intel/e1000/e1000_ethtool.c | 10 ++---
> drivers/net/ethernet/intel/e1000e/ethtool.c | 14 +++----
> .../net/ethernet/intel/fm10k/fm10k_ethtool.c | 10 ++---
> .../net/ethernet/intel/i40e/i40e_ethtool.c | 6 +--
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 37 +++++++++++--------
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 35 ++++++++++--------
> drivers/net/ethernet/intel/igbvf/ethtool.c | 10 ++---
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 36 +++++++++---------
> .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 32 ++++++++--------
for ice, igb, igc, and ixgbe the current code already uses ethtool
string helpers, and in many places you are just changing variable name,
"p" to "data", I would rather avoid that.
sorry for not spotting that earlier, and apologies that we have so many
drivers to fix up in the first place
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 2924ac61300d..62a152be8180 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -83,7 +83,7 @@ static const char ice_gstrings_test[][ETH_GSTRING_LEN] = {
> "Link test (on/offline)",
> };
>
> -#define ICE_TEST_LEN (sizeof(ice_gstrings_test) / ETH_GSTRING_LEN)
> +#define ICE_TEST_LEN ARRAY_SIZE(ice_gstrings_test)
>
> /* These PF_STATs might look like duplicates of some NETDEV_STATs,
> * but they aren't. This device is capable of supporting multiple
> @@ -1481,48 +1481,53 @@ static void
> __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
> struct ice_vsi *vsi)
> {
> + const char *str;
> unsigned int i;
> - u8 *p = data;
>
> switch (stringset) {
> case ETH_SS_STATS:
> - for (i = 0; i < ICE_VSI_STATS_LEN; i++)
> - ethtool_puts(&p, ice_gstrings_vsi_stats[i].stat_string);
> + for (i = 0; i < ICE_VSI_STATS_LEN; i++) {
> + str = ice_gstrings_vsi_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
>
> if (ice_is_port_repr_netdev(netdev))
> return;
>
> ice_for_each_alloc_txq(vsi, i) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> }
>
> ice_for_each_alloc_rxq(vsi, i) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> }
>
> if (vsi->type != ICE_VSI_PF)
> return;
>
> - for (i = 0; i < ICE_PF_STATS_LEN; i++)
> - ethtool_puts(&p, ice_gstrings_pf_stats[i].stat_string);
> + for (i = 0; i < ICE_PF_STATS_LEN; i++) {
> + str = ice_gstrings_pf_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
>
> for (i = 0; i < ICE_MAX_USER_PRIORITY; i++) {
> - ethtool_sprintf(&p, "tx_priority_%u_xon.nic", i);
> - ethtool_sprintf(&p, "tx_priority_%u_xoff.nic", i);
> + ethtool_sprintf(&data, "tx_priority_%u_xon.nic", i);
> + ethtool_sprintf(&data, "tx_priority_%u_xoff.nic", i);
> }
> for (i = 0; i < ICE_MAX_USER_PRIORITY; i++) {
> - ethtool_sprintf(&p, "rx_priority_%u_xon.nic", i);
> - ethtool_sprintf(&p, "rx_priority_%u_xoff.nic", i);
> + ethtool_sprintf(&data, "rx_priority_%u_xon.nic", i);
> + ethtool_sprintf(&data, "rx_priority_%u_xoff.nic", i);
> }
> break;
> case ETH_SS_TEST:
> - memcpy(data, ice_gstrings_test, ICE_TEST_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < ICE_TEST_LEN; i++)
> + ethtool_puts(&data, ice_gstrings_test[i]);
> break;
> case ETH_SS_PRIV_FLAGS:
> for (i = 0; i < ICE_PRIV_FLAG_ARRAY_SIZE; i++)
> - ethtool_puts(&p, ice_gstrings_priv_flags[i].name);
> + ethtool_puts(&data, ice_gstrings_priv_flags[i].name);
> break;
> default:
> break;
really no need to git-blame touch most of the code here
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index ca6ccbc13954..c4a8712389af 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -123,7 +123,7 @@ static const char igb_gstrings_test[][ETH_GSTRING_LEN] = {
> [TEST_LOOP] = "Loopback test (offline)",
> [TEST_LINK] = "Link test (on/offline)"
> };
> -#define IGB_TEST_LEN (sizeof(igb_gstrings_test) / ETH_GSTRING_LEN)
> +#define IGB_TEST_LEN ARRAY_SIZE(igb_gstrings_test)
>
> static const char igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
> #define IGB_PRIV_FLAGS_LEGACY_RX BIT(0)
> @@ -2347,35 +2347,38 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
> static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
> {
> struct igb_adapter *adapter = netdev_priv(netdev);
> - u8 *p = data;
> + const char *str;
> int i;
>
> switch (stringset) {
> case ETH_SS_TEST:
> - memcpy(data, igb_gstrings_test, sizeof(igb_gstrings_test));
> + for (i = 0; i < IGB_TEST_LEN; i++)
> + ethtool_puts(&data, igb_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, igb_gstrings_stats[i].stat_string);
> - for (i = 0; i < IGB_NETDEV_STATS_LEN; i++)
> - ethtool_puts(&p, igb_gstrings_net_stats[i].stat_string);
> + ethtool_puts(&data, igb_gstrings_stats[i].stat_string);
> + for (i = 0; i < IGB_NETDEV_STATS_LEN; i++) {
> + str = igb_gstrings_net_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < adapter->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "tx_queue_%u_restart", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_restart", i);
> }
> for (i = 0; i < adapter->num_rx_queues; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "rx_queue_%u_drops", i);
> - ethtool_sprintf(&p, "rx_queue_%u_csum_err", i);
> - ethtool_sprintf(&p, "rx_queue_%u_alloc_failed", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_drops", i);
> + ethtool_sprintf(&data, "rx_queue_%u_csum_err", i);
> + ethtool_sprintf(&data, "rx_queue_%u_alloc_failed", i);
> }
> /* BUG_ON(p - data != IGB_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, igb_priv_flags_strings,
> - IGB_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGB_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, igb_priv_flags_strings[i]);
> break;
> }
> }
ditto
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 5b0c6f433767..7b118fb7097b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -104,7 +104,7 @@ static const char igc_gstrings_test[][ETH_GSTRING_LEN] = {
> [TEST_LINK] = "Link test (on/offline)"
> };
>
> -#define IGC_TEST_LEN (sizeof(igc_gstrings_test) / ETH_GSTRING_LEN)
> +#define IGC_TEST_LEN ARRAY_SIZE(igc_gstrings_test)
>
> #define IGC_GLOBAL_STATS_LEN \
> (sizeof(igc_gstrings_stats) / sizeof(struct igc_stats))
> @@ -763,36 +763,38 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset,
> u8 *data)
> {
> struct igc_adapter *adapter = netdev_priv(netdev);
> - u8 *p = data;
> + const char *str;
> int i;
>
> switch (stringset) {
> case ETH_SS_TEST:
> - memcpy(data, *igc_gstrings_test,
> - IGC_TEST_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGC_TEST_LEN; i++)
> + ethtool_puts(&data, igc_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> for (i = 0; i < IGC_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, igc_gstrings_stats[i].stat_string);
> - for (i = 0; i < IGC_NETDEV_STATS_LEN; i++)
> - ethtool_puts(&p, igc_gstrings_net_stats[i].stat_string);
> + ethtool_puts(&data, igc_gstrings_stats[i].stat_string);
> + for (i = 0; i < IGC_NETDEV_STATS_LEN; i++) {
> + str = igc_gstrings_net_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < adapter->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "tx_queue_%u_restart", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_restart", i);
> }
> for (i = 0; i < adapter->num_rx_queues; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "rx_queue_%u_drops", i);
> - ethtool_sprintf(&p, "rx_queue_%u_csum_err", i);
> - ethtool_sprintf(&p, "rx_queue_%u_alloc_failed", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_drops", i);
> + ethtool_sprintf(&data, "rx_queue_%u_csum_err", i);
> + ethtool_sprintf(&data, "rx_queue_%u_alloc_failed", i);
> }
> /* BUG_ON(p - data != IGC_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, igc_priv_flags_strings,
> - IGC_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGC_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, igc_priv_flags_strings[i]);
> break;
> }
> }
ditto
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 9482e0cca8b7..b3b2e38c2ae6 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -129,7 +129,7 @@ static const char ixgbe_gstrings_test[][ETH_GSTRING_LEN] = {
> "Interrupt test (offline)", "Loopback test (offline)",
> "Link test (on/offline)"
> };
> -#define IXGBE_TEST_LEN sizeof(ixgbe_gstrings_test) / ETH_GSTRING_LEN
> +#define IXGBE_TEST_LEN ARRAY_SIZE(ixgbe_gstrings_test)
>
> static const char ixgbe_priv_flags_strings[][ETH_GSTRING_LEN] = {
> #define IXGBE_PRIV_FLAGS_LEGACY_RX BIT(0)
> @@ -1409,38 +1409,40 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
> static void ixgbe_get_strings(struct net_device *netdev, u32 stringset,
> u8 *data)
> {
> + const char *str;
> unsigned int i;
> - u8 *p = data;
>
> switch (stringset) {
> case ETH_SS_TEST:
> for (i = 0; i < IXGBE_TEST_LEN; i++)
> - ethtool_puts(&p, ixgbe_gstrings_test[i]);
> + ethtool_puts(&data, ixgbe_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> - for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, ixgbe_gstrings_stats[i].stat_string);
> + for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++) {
> + str = ixgbe_gstrings_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < netdev->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> }
> for (i = 0; i < IXGBE_NUM_RX_QUEUES; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> }
> for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++) {
> - ethtool_sprintf(&p, "tx_pb_%u_pxon", i);
> - ethtool_sprintf(&p, "tx_pb_%u_pxoff", i);
> + ethtool_sprintf(&data, "tx_pb_%u_pxon", i);
> + ethtool_sprintf(&data, "tx_pb_%u_pxoff", i);
> }
> for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++) {
> - ethtool_sprintf(&p, "rx_pb_%u_pxon", i);
> - ethtool_sprintf(&p, "rx_pb_%u_pxoff", i);
> + ethtool_sprintf(&data, "rx_pb_%u_pxon", i);
> + ethtool_sprintf(&data, "rx_pb_%u_pxoff", i);
> }
> /* BUG_ON(p - data != IXGBE_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, ixgbe_priv_flags_strings,
> - IXGBE_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IXGBE_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, ixgbe_priv_flags_strings[i]);
> }
> }
>
ditto here
WARNING: multiple messages have this Message-ID (diff)
From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: Rosen Penev <rosenp@gmail.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>,
<netdev@vger.kernel.org>, "Andrew Lunn" <andrew+netdev@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
"Eric Dumazet" <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Jesper Dangaard Brouer <hawk@kernel.org>,
"John Fastabend" <john.fastabend@gmail.com>,
"moderated list:INTEL ETHERNET DRIVERS"
<intel-wired-lan@lists.osuosl.org>,
open list <linux-kernel@vger.kernel.org>,
"open list:XDP (eXpress Data Path):Keyword:(?:b|_)xdp(?:b|_)"
<bpf@vger.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCHv2 net-next iwl-next] net: intel: use ethtool string helpers
Date: Mon, 28 Oct 2024 11:12:49 +0100 [thread overview]
Message-ID: <ca89f03e-6dc1-44fa-bfd1-aac95ede0cbe@intel.com> (raw)
In-Reply-To: <20241025201713.286074-1-rosenp@gmail.com>
On 10/25/24 22:17, 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>
> ---
> v2: add iwl-next tag. use inline int in for loops.
> .../net/ethernet/intel/e1000/e1000_ethtool.c | 10 ++---
> drivers/net/ethernet/intel/e1000e/ethtool.c | 14 +++----
> .../net/ethernet/intel/fm10k/fm10k_ethtool.c | 10 ++---
> .../net/ethernet/intel/i40e/i40e_ethtool.c | 6 +--
> drivers/net/ethernet/intel/ice/ice_ethtool.c | 37 +++++++++++--------
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 35 ++++++++++--------
> drivers/net/ethernet/intel/igbvf/ethtool.c | 10 ++---
> drivers/net/ethernet/intel/igc/igc_ethtool.c | 36 +++++++++---------
> .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 32 ++++++++--------
for ice, igb, igc, and ixgbe the current code already uses ethtool
string helpers, and in many places you are just changing variable name,
"p" to "data", I would rather avoid that.
sorry for not spotting that earlier, and apologies that we have so many
drivers to fix up in the first place
> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index 2924ac61300d..62a152be8180 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -83,7 +83,7 @@ static const char ice_gstrings_test[][ETH_GSTRING_LEN] = {
> "Link test (on/offline)",
> };
>
> -#define ICE_TEST_LEN (sizeof(ice_gstrings_test) / ETH_GSTRING_LEN)
> +#define ICE_TEST_LEN ARRAY_SIZE(ice_gstrings_test)
>
> /* These PF_STATs might look like duplicates of some NETDEV_STATs,
> * but they aren't. This device is capable of supporting multiple
> @@ -1481,48 +1481,53 @@ static void
> __ice_get_strings(struct net_device *netdev, u32 stringset, u8 *data,
> struct ice_vsi *vsi)
> {
> + const char *str;
> unsigned int i;
> - u8 *p = data;
>
> switch (stringset) {
> case ETH_SS_STATS:
> - for (i = 0; i < ICE_VSI_STATS_LEN; i++)
> - ethtool_puts(&p, ice_gstrings_vsi_stats[i].stat_string);
> + for (i = 0; i < ICE_VSI_STATS_LEN; i++) {
> + str = ice_gstrings_vsi_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
>
> if (ice_is_port_repr_netdev(netdev))
> return;
>
> ice_for_each_alloc_txq(vsi, i) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> }
>
> ice_for_each_alloc_rxq(vsi, i) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> }
>
> if (vsi->type != ICE_VSI_PF)
> return;
>
> - for (i = 0; i < ICE_PF_STATS_LEN; i++)
> - ethtool_puts(&p, ice_gstrings_pf_stats[i].stat_string);
> + for (i = 0; i < ICE_PF_STATS_LEN; i++) {
> + str = ice_gstrings_pf_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
>
> for (i = 0; i < ICE_MAX_USER_PRIORITY; i++) {
> - ethtool_sprintf(&p, "tx_priority_%u_xon.nic", i);
> - ethtool_sprintf(&p, "tx_priority_%u_xoff.nic", i);
> + ethtool_sprintf(&data, "tx_priority_%u_xon.nic", i);
> + ethtool_sprintf(&data, "tx_priority_%u_xoff.nic", i);
> }
> for (i = 0; i < ICE_MAX_USER_PRIORITY; i++) {
> - ethtool_sprintf(&p, "rx_priority_%u_xon.nic", i);
> - ethtool_sprintf(&p, "rx_priority_%u_xoff.nic", i);
> + ethtool_sprintf(&data, "rx_priority_%u_xon.nic", i);
> + ethtool_sprintf(&data, "rx_priority_%u_xoff.nic", i);
> }
> break;
> case ETH_SS_TEST:
> - memcpy(data, ice_gstrings_test, ICE_TEST_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < ICE_TEST_LEN; i++)
> + ethtool_puts(&data, ice_gstrings_test[i]);
> break;
> case ETH_SS_PRIV_FLAGS:
> for (i = 0; i < ICE_PRIV_FLAG_ARRAY_SIZE; i++)
> - ethtool_puts(&p, ice_gstrings_priv_flags[i].name);
> + ethtool_puts(&data, ice_gstrings_priv_flags[i].name);
> break;
> default:
> break;
really no need to git-blame touch most of the code here
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index ca6ccbc13954..c4a8712389af 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -123,7 +123,7 @@ static const char igb_gstrings_test[][ETH_GSTRING_LEN] = {
> [TEST_LOOP] = "Loopback test (offline)",
> [TEST_LINK] = "Link test (on/offline)"
> };
> -#define IGB_TEST_LEN (sizeof(igb_gstrings_test) / ETH_GSTRING_LEN)
> +#define IGB_TEST_LEN ARRAY_SIZE(igb_gstrings_test)
>
> static const char igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
> #define IGB_PRIV_FLAGS_LEGACY_RX BIT(0)
> @@ -2347,35 +2347,38 @@ static void igb_get_ethtool_stats(struct net_device *netdev,
> static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
> {
> struct igb_adapter *adapter = netdev_priv(netdev);
> - u8 *p = data;
> + const char *str;
> int i;
>
> switch (stringset) {
> case ETH_SS_TEST:
> - memcpy(data, igb_gstrings_test, sizeof(igb_gstrings_test));
> + for (i = 0; i < IGB_TEST_LEN; i++)
> + ethtool_puts(&data, igb_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> for (i = 0; i < IGB_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, igb_gstrings_stats[i].stat_string);
> - for (i = 0; i < IGB_NETDEV_STATS_LEN; i++)
> - ethtool_puts(&p, igb_gstrings_net_stats[i].stat_string);
> + ethtool_puts(&data, igb_gstrings_stats[i].stat_string);
> + for (i = 0; i < IGB_NETDEV_STATS_LEN; i++) {
> + str = igb_gstrings_net_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < adapter->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "tx_queue_%u_restart", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_restart", i);
> }
> for (i = 0; i < adapter->num_rx_queues; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "rx_queue_%u_drops", i);
> - ethtool_sprintf(&p, "rx_queue_%u_csum_err", i);
> - ethtool_sprintf(&p, "rx_queue_%u_alloc_failed", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_drops", i);
> + ethtool_sprintf(&data, "rx_queue_%u_csum_err", i);
> + ethtool_sprintf(&data, "rx_queue_%u_alloc_failed", i);
> }
> /* BUG_ON(p - data != IGB_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, igb_priv_flags_strings,
> - IGB_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGB_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, igb_priv_flags_strings[i]);
> break;
> }
> }
ditto
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 5b0c6f433767..7b118fb7097b 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -104,7 +104,7 @@ static const char igc_gstrings_test[][ETH_GSTRING_LEN] = {
> [TEST_LINK] = "Link test (on/offline)"
> };
>
> -#define IGC_TEST_LEN (sizeof(igc_gstrings_test) / ETH_GSTRING_LEN)
> +#define IGC_TEST_LEN ARRAY_SIZE(igc_gstrings_test)
>
> #define IGC_GLOBAL_STATS_LEN \
> (sizeof(igc_gstrings_stats) / sizeof(struct igc_stats))
> @@ -763,36 +763,38 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset,
> u8 *data)
> {
> struct igc_adapter *adapter = netdev_priv(netdev);
> - u8 *p = data;
> + const char *str;
> int i;
>
> switch (stringset) {
> case ETH_SS_TEST:
> - memcpy(data, *igc_gstrings_test,
> - IGC_TEST_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGC_TEST_LEN; i++)
> + ethtool_puts(&data, igc_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> for (i = 0; i < IGC_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, igc_gstrings_stats[i].stat_string);
> - for (i = 0; i < IGC_NETDEV_STATS_LEN; i++)
> - ethtool_puts(&p, igc_gstrings_net_stats[i].stat_string);
> + ethtool_puts(&data, igc_gstrings_stats[i].stat_string);
> + for (i = 0; i < IGC_NETDEV_STATS_LEN; i++) {
> + str = igc_gstrings_net_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < adapter->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "tx_queue_%u_restart", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_restart", i);
> }
> for (i = 0; i < adapter->num_rx_queues; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> - ethtool_sprintf(&p, "rx_queue_%u_drops", i);
> - ethtool_sprintf(&p, "rx_queue_%u_csum_err", i);
> - ethtool_sprintf(&p, "rx_queue_%u_alloc_failed", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_drops", i);
> + ethtool_sprintf(&data, "rx_queue_%u_csum_err", i);
> + ethtool_sprintf(&data, "rx_queue_%u_alloc_failed", i);
> }
> /* BUG_ON(p - data != IGC_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, igc_priv_flags_strings,
> - IGC_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IGC_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, igc_priv_flags_strings[i]);
> break;
> }
> }
ditto
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> index 9482e0cca8b7..b3b2e38c2ae6 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
> @@ -129,7 +129,7 @@ static const char ixgbe_gstrings_test[][ETH_GSTRING_LEN] = {
> "Interrupt test (offline)", "Loopback test (offline)",
> "Link test (on/offline)"
> };
> -#define IXGBE_TEST_LEN sizeof(ixgbe_gstrings_test) / ETH_GSTRING_LEN
> +#define IXGBE_TEST_LEN ARRAY_SIZE(ixgbe_gstrings_test)
>
> static const char ixgbe_priv_flags_strings[][ETH_GSTRING_LEN] = {
> #define IXGBE_PRIV_FLAGS_LEGACY_RX BIT(0)
> @@ -1409,38 +1409,40 @@ static void ixgbe_get_ethtool_stats(struct net_device *netdev,
> static void ixgbe_get_strings(struct net_device *netdev, u32 stringset,
> u8 *data)
> {
> + const char *str;
> unsigned int i;
> - u8 *p = data;
>
> switch (stringset) {
> case ETH_SS_TEST:
> for (i = 0; i < IXGBE_TEST_LEN; i++)
> - ethtool_puts(&p, ixgbe_gstrings_test[i]);
> + ethtool_puts(&data, ixgbe_gstrings_test[i]);
> break;
> case ETH_SS_STATS:
> - for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++)
> - ethtool_puts(&p, ixgbe_gstrings_stats[i].stat_string);
> + for (i = 0; i < IXGBE_GLOBAL_STATS_LEN; i++) {
> + str = ixgbe_gstrings_stats[i].stat_string;
> + ethtool_puts(&data, str);
> + }
> for (i = 0; i < netdev->num_tx_queues; i++) {
> - ethtool_sprintf(&p, "tx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "tx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "tx_queue_%u_bytes", i);
> }
> for (i = 0; i < IXGBE_NUM_RX_QUEUES; i++) {
> - ethtool_sprintf(&p, "rx_queue_%u_packets", i);
> - ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
> + ethtool_sprintf(&data, "rx_queue_%u_packets", i);
> + ethtool_sprintf(&data, "rx_queue_%u_bytes", i);
> }
> for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++) {
> - ethtool_sprintf(&p, "tx_pb_%u_pxon", i);
> - ethtool_sprintf(&p, "tx_pb_%u_pxoff", i);
> + ethtool_sprintf(&data, "tx_pb_%u_pxon", i);
> + ethtool_sprintf(&data, "tx_pb_%u_pxoff", i);
> }
> for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++) {
> - ethtool_sprintf(&p, "rx_pb_%u_pxon", i);
> - ethtool_sprintf(&p, "rx_pb_%u_pxoff", i);
> + ethtool_sprintf(&data, "rx_pb_%u_pxon", i);
> + ethtool_sprintf(&data, "rx_pb_%u_pxoff", i);
> }
> /* BUG_ON(p - data != IXGBE_STATS_LEN * ETH_GSTRING_LEN); */
> break;
> case ETH_SS_PRIV_FLAGS:
> - memcpy(data, ixgbe_priv_flags_strings,
> - IXGBE_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> + for (i = 0; i < IXGBE_PRIV_FLAGS_STR_LEN; i++)
> + ethtool_puts(&data, ixgbe_priv_flags_strings[i]);
> }
> }
>
ditto here
next prev parent reply other threads:[~2024-10-28 10:13 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-25 20:17 [PATCHv2 net-next iwl-next] net: intel: use ethtool string helpers Rosen Penev
2024-10-25 20:17 ` [Intel-wired-lan] " Rosen Penev
2024-10-28 10:12 ` Przemek Kitszel [this message]
2024-10-28 10:12 ` Przemek Kitszel
2024-10-30 22:52 ` Rosen Penev
2024-10-30 22:52 ` [Intel-wired-lan] " Rosen Penev
2024-10-31 7:45 ` Przemek Kitszel
2024-10-31 7:45 ` [Intel-wired-lan] " Przemek Kitszel
2024-10-31 21:03 ` Rosen Penev
2024-10-31 21:03 ` [Intel-wired-lan] " Rosen Penev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ca89f03e-6dc1-44fa-bfd1-aac95ede0cbe@intel.com \
--to=przemyslaw.kitszel@intel.com \
--cc=andrew+netdev@lunn.ch \
--cc=anthony.l.nguyen@intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=hawk@kernel.org \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=rosenp@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.