[PATCH net-next v2 0/2] gve: Minor cleanups

[PATCH net-next v2 0/2] gve: Minor cleanups

[Simon Horman]

Hi,

This short patchset provides two minor cleanups for the gve driver.

These were found by tooling as mentioned in each patch,
and otherwise by inspection.

No change in run time behaviour is intended.
Each patch is compile tested only.

---
Changes in v2:
- Collected Reviewed-by tags, thanks!
- Rebased
- Link to v1: https://lore.kernel.org/r/20240503-gve-comma-v1-0-b50f965694ef@kernel.org

---
Simon Horman (2):
      gve: Avoid unnecessary use of comma operator
      gve: Use ethtool_sprintf/puts() to fill stats strings

 drivers/net/ethernet/google/gve/gve_adminq.c  |  4 +--
 drivers/net/ethernet/google/gve/gve_ethtool.c | 42 +++++++++++----------------
 2 files changed, 19 insertions(+), 27 deletions(-)

base-commit: 09ca994072fd8ae99c763db2450222365dfe8fdf

[PATCH net-next v2 1/2] gve: Avoid unnecessary use of comma operator

[Simon Horman]

Although it does not seem to have any untoward side-effects,
the use of ';' to separate to assignments seems more appropriate than ','.

Flagged by clang-18 -Wcomma

No functional change intended.
Compile tested only.

Reviewed-by: Shailend Chand <shailend@google.com>
Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/google/gve/gve_adminq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 3df8243680d9..8ca0def176ef 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -650,9 +650,9 @@ static void gve_adminq_get_create_rx_queue_cmd(struct gve_priv *priv,
 			GVE_RAW_ADDRESSING_QPL_ID : rx->data.qpl->id;
 
 		cmd->create_rx_queue.rx_desc_ring_addr =
-			cpu_to_be64(rx->desc.bus),
+			cpu_to_be64(rx->desc.bus);
 		cmd->create_rx_queue.rx_data_ring_addr =
-			cpu_to_be64(rx->data.data_bus),
+			cpu_to_be64(rx->data.data_bus);
 		cmd->create_rx_queue.index = cpu_to_be32(queue_index);
 		cmd->create_rx_queue.queue_page_list_id = cpu_to_be32(qpl_id);
 		cmd->create_rx_queue.packet_buffer_size = cpu_to_be16(rx->packet_buffer_size);

-- 
2.43.0

[PATCH net-next v2 2/2] gve: Use ethtool_sprintf/puts() to fill stats strings

[Simon Horman]

Make use of standard helpers to simplify filling in stats strings.

The first two ethtool_puts() changes address the following fortification
warnings flagged by W=1 builds with clang-18. (The last ethtool_puts
change does not because the warning relates to writing beyond the first
element of an array, and gve_gstrings_priv_flags only has one element.)

.../fortify-string.h:562:4: warning: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
  562 |                         __read_overflow2_field(q_size_field, size);
      |                         ^
.../fortify-string.h:562:4: warning: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]

Likewise, the same changes resolve the same problems flagged by Smatch.

.../gve_ethtool.c:100 gve_get_strings() error: __builtin_memcpy() '*gve_gstrings_main_stats' too small (32 vs 576)
.../gve_ethtool.c:120 gve_get_strings() error: __builtin_memcpy() '*gve_gstrings_adminq_stats' too small (32 vs 512)

Compile tested only.

Reviewed-by: Shailend Chand <shailend@google.com>
Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
Signed-off-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/google/gve/gve_ethtool.c | 42 +++++++++++----------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 156b7e128b53..fe1741d482b4 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -90,42 +90,34 @@ static const char gve_gstrings_priv_flags[][ETH_GSTRING_LEN] = {
 static void gve_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 {
 	struct gve_priv *priv = netdev_priv(netdev);
-	char *s = (char *)data;
+	u8 *s = (char *)data;
 	int num_tx_queues;
 	int i, j;
 
 	num_tx_queues = gve_num_tx_queues(priv);
 	switch (stringset) {
 	case ETH_SS_STATS:
-		memcpy(s, *gve_gstrings_main_stats,
-		       sizeof(gve_gstrings_main_stats));
-		s += sizeof(gve_gstrings_main_stats);
-
-		for (i = 0; i < priv->rx_cfg.num_queues; i++) {
-			for (j = 0; j < NUM_GVE_RX_CNTS; j++) {
-				snprintf(s, ETH_GSTRING_LEN,
-					 gve_gstrings_rx_stats[j], i);
-				s += ETH_GSTRING_LEN;
-			}
-		}
+		for (i = 0; i < ARRAY_SIZE(gve_gstrings_main_stats); i++)
+			ethtool_puts(&s, gve_gstrings_main_stats[i]);
 
-		for (i = 0; i < num_tx_queues; i++) {
-			for (j = 0; j < NUM_GVE_TX_CNTS; j++) {
-				snprintf(s, ETH_GSTRING_LEN,
-					 gve_gstrings_tx_stats[j], i);
-				s += ETH_GSTRING_LEN;
-			}
-		}
+		for (i = 0; i < priv->rx_cfg.num_queues; i++)
+			for (j = 0; j < NUM_GVE_RX_CNTS; j++)
+				ethtool_sprintf(&s, gve_gstrings_rx_stats[j],
+						i);
+
+		for (i = 0; i < num_tx_queues; i++)
+			for (j = 0; j < NUM_GVE_TX_CNTS; j++)
+				ethtool_sprintf(&s, gve_gstrings_tx_stats[j],
+						i);
+
+		for (i = 0; i < ARRAY_SIZE(gve_gstrings_adminq_stats); i++)
+			ethtool_puts(&s, gve_gstrings_adminq_stats[i]);
 
-		memcpy(s, *gve_gstrings_adminq_stats,
-		       sizeof(gve_gstrings_adminq_stats));
-		s += sizeof(gve_gstrings_adminq_stats);
 		break;
 
 	case ETH_SS_PRIV_FLAGS:
-		memcpy(s, *gve_gstrings_priv_flags,
-		       sizeof(gve_gstrings_priv_flags));
-		s += sizeof(gve_gstrings_priv_flags);
+		for (i = 0; i < ARRAY_SIZE(gve_gstrings_priv_flags); i++)
+			ethtool_puts(&s, gve_gstrings_priv_flags[i]);
 		break;
 
 	default:

-- 
2.43.0

Re: [PATCH net-next v2 2/2] gve: Use ethtool_sprintf/puts() to fill stats strings

[Justin Stitt]

Hi, 

On Wed, May 08, 2024 at 09:32:20AM +0100, Simon Horman wrote:
> Make use of standard helpers to simplify filling in stats strings.
> 
> The first two ethtool_puts() changes address the following fortification
> warnings flagged by W=1 builds with clang-18. (The last ethtool_puts
> change does not because the warning relates to writing beyond the first
> element of an array, and gve_gstrings_priv_flags only has one element.)
> 
> .../fortify-string.h:562:4: warning: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
>   562 |                         __read_overflow2_field(q_size_field, size);
>       |                         ^
> .../fortify-string.h:562:4: warning: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> 
> Likewise, the same changes resolve the same problems flagged by Smatch.
> 
> .../gve_ethtool.c:100 gve_get_strings() error: __builtin_memcpy() '*gve_gstrings_main_stats' too small (32 vs 576)
> .../gve_ethtool.c:120 gve_get_strings() error: __builtin_memcpy() '*gve_gstrings_adminq_stats' too small (32 vs 512)
> 
> Compile tested only.
> 
> Reviewed-by: Shailend Chand <shailend@google.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Signed-off-by: Simon Horman <horms@kernel.org>

This patch looks good and follows similar replacements [1] I've made in
the past.

Acked-by: Justin Stitt <justinstitt@google.com>

> ---
>  drivers/net/ethernet/google/gve/gve_ethtool.c | 42 +++++++++++----------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
> index 156b7e128b53..fe1741d482b4 100644
>

[1]: https://lore.kernel.org/all/?q=f%3A%22Justin+stitt%22+AND+dfb%3A%22ethtool_puts%22

Thanks
Justin

Re: [PATCH net-next v2 0/2] gve: Minor cleanups

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 08 May 2024 09:32:18 +0100 you wrote:
> Hi,
> 
> This short patchset provides two minor cleanups for the gve driver.
> 
> These were found by tooling as mentioned in each patch,
> and otherwise by inspection.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/2] gve: Avoid unnecessary use of comma operator
    https://git.kernel.org/netdev/net-next/c/ebb8308eac84
  - [net-next,v2,2/2] gve: Use ethtool_sprintf/puts() to fill stats strings
    https://git.kernel.org/netdev/net-next/c/ba8bcb012b7d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html