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

[PATCH net-next 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.

---
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: 5829614a7b3b2cc9820efb2d29a205c00d748fcf

[PATCH net-next 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.

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 b2b619aa2310..8d49462170a3 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -649,9 +649,9 @@ static int gve_adminq_create_rx_queue(struct gve_priv *priv, u32 queue_index)
 			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 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.

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 bd7632eed776..31563eeb0a41 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -89,42 +89,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 1/2] gve: Avoid unnecessary use of comma operator

[Dan Carpenter]

On Fri, May 03, 2024 at 09:31:26PM +0100, Simon Horman wrote:
> Although it does not seem to have any untoward side-effects,
> the use of ';' to separate to assignments seems more appropriate than ','.
> 

Huh.  Interesting.  I wrote a check for that in Smatch.  The only place
where it would matter would be in an if statement.

	if (foo)
		frob1(),
	frob2();

It's unexpected that frob2() is included in the if statement.  But I
was never able to find any of these actual bugs so I gave up.

regards,
dan carpenter

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

[Larysa Zaremba]

On Fri, May 03, 2024 at 09:31:25PM +0100, Simon Horman 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.
> 
> No change in run time behaviour is intended.
> Each patch is compile tested only.
>

Both patches look fine to me.

Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
 
> ---
> 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: 5829614a7b3b2cc9820efb2d29a205c00d748fcf
> 
> 

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

[Shailend Chand]

On Fri, May 3, 2024 at 1:31 PM Simon Horman <horms@kernel.org> 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.
>
> No change in run time behaviour is intended.
> Each patch is compile tested only.
>
> ---
> Simon Horman (2):
>       gve: Avoid unnecessary use of comma operator
>       gve: Use ethtool_sprintf/puts() to fill stats strings

Reviewed-by: Shailend Chand <shailend@google.com>
Thanks!

>
>  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: 5829614a7b3b2cc9820efb2d29a205c00d748fcf
>

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

[Jakub Kicinski]

On Fri, 03 May 2024 21:31:25 +0100 Simon Horman wrote:
> 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.

Looks like it conflicts now, please rebase
-- 
pw-bot: cr

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

[Simon Horman]

On Tue, May 07, 2024 at 03:28:46PM -0700, Jakub Kicinski wrote:
> On Fri, 03 May 2024 21:31:25 +0100 Simon Horman wrote:
> > 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.
> 
> Looks like it conflicts now, please rebase

Thanks, I'll rebase and send a v2.