Re: [PATCHv2 net-next iwl-next] net: intel: use ethtool string helpers

Przemek Kitszel <przemyslaw.kitszel@xxxxxxxxx> · Thu, 31 Oct 2024 08:45:49 +0100

On 10/30/24 23:52, Rosen Penev wrote:
On Mon, Oct 28, 2024 at 3:13 AM Przemek Kitszel
<przemyslaw.kitszel@xxxxxxxxx> wrote:

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@xxxxxxxxx>
---
   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.
well, since I'm cleaning some of this code up, might as well get rid
of variables. That was suggested to me with other similar patches.

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>

Actually the function should be taking a double pointer here I think
in case something gets called after it in the main function.
I mean that both @p and @data are (u8 *).
I'm fine getting rid of tmp var, and updating the originally passed
argument is fine. But you could achieve it by just changing param name.

BTW I guess it was @p to fit into 80 chars more easily ;)







