[PATCH net] gve: DQO: Suppress unused var warnings

Some variables become unused when `CONFIG_NEED_DMA_MAP_STATE=n`.

We only suppress when `CONFIG_NEED_DMA_MAP_STATE=n` in order to avoid
false negatives.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Bailey Forrest <redacted>
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index 05ddb6a75c38..f873321c022e 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c

@@ -87,6 +87,9 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
 		for (j = 0; j < cur_state->num_bufs; j++) {
 			struct gve_tx_dma_buf *buf = &cur_state->bufs[j];
 
+#ifndef CONFIG_NEED_DMA_MAP_STATE
+			(void)buf;  // Suppress unused variable.
+#endif
 			if (j == 0) {
 				dma_unmap_single(tx->dev,
 						 dma_unmap_addr(buf, dma),

@@ -459,9 +462,14 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 
 	struct gve_tx_pending_packet_dqo *pending_packet;
 	struct gve_tx_metadata_dqo metadata;
+	struct gve_tx_dma_buf *buf;
 	s16 completion_tag;
 	int i;
 
+#ifndef CONFIG_NEED_DMA_MAP_STATE
+	(void)buf;  // Suppress unused variable.
+#endif
+
 	pending_packet = gve_alloc_pending_packet(tx);
 	pending_packet->skb = skb;
 	pending_packet->num_bufs = 0;

@@ -493,8 +501,6 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 
 	/* Map the linear portion of skb */
 	{
-		struct gve_tx_dma_buf *buf =
-			&pending_packet->bufs[pending_packet->num_bufs];
 		u32 len = skb_headlen(skb);
 		dma_addr_t addr;
 

@@ -502,6 +508,7 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 		if (unlikely(dma_mapping_error(tx->dev, addr)))
 			goto err;
 
+		buf = &pending_packet->bufs[pending_packet->num_bufs];
 		dma_unmap_len_set(buf, len, len);
 		dma_unmap_addr_set(buf, dma, addr);
 		++pending_packet->num_bufs;

@@ -512,8 +519,6 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 	}
 
 	for (i = 0; i < shinfo->nr_frags; i++) {
-		struct gve_tx_dma_buf *buf =
-			&pending_packet->bufs[pending_packet->num_bufs];
 		const skb_frag_t *frag = &shinfo->frags[i];
 		bool is_eop = i == (shinfo->nr_frags - 1);
 		u32 len = skb_frag_size(frag);

@@ -523,6 +528,7 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 		if (unlikely(dma_mapping_error(tx->dev, addr)))
 			goto err;
 
+		buf = &pending_packet->bufs[pending_packet->num_bufs];
 		dma_unmap_len_set(buf, len, len);
 		dma_unmap_addr_set(buf, dma, addr);
 		++pending_packet->num_bufs;

@@ -555,6 +561,9 @@ static int gve_tx_add_skb_no_copy_dqo(struct gve_tx_ring *tx,
 	for (i = 0; i < pending_packet->num_bufs; i++) {
 		struct gve_tx_dma_buf *buf = &pending_packet->bufs[i];
 
+#ifndef CONFIG_NEED_DMA_MAP_STATE
+		(void)buf;  // Suppress unused variable.
+#endif
 		if (i == 0) {
 			dma_unmap_single(tx->dev, dma_unmap_addr(buf, dma),
 					 dma_unmap_len(buf, len),

-- 
2.32.0.432.gabb21c7263-goog

On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest [off-list ref] wrote:

Some variables become unused when `CONFIG_NEED_DMA_MAP_STATE=n`.

We only suppress when `CONFIG_NEED_DMA_MAP_STATE=n` in order to avoid
false negatives.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Bailey Forrest <redacted>

Hi Bailey,

I see that the warning still exists in linux-5.15-rc3 and net-next,
I'm building with my original patch[1] to get around the -Werror
warnings.

Can you resend your patch, or should I resend mine after all?

      Arnd

[1] https://lore.kernel.org/all/20210721151100.2042139-1-arnd@kernel.org/

Apologies, resending as text

On Mon, Sep 27, 2021 at 2:59 AM Arnd Bergmann [off-list ref] wrote:

On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest [off-list ref] wrote:

quoted

Some variables become unused when `CONFIG_NEED_DMA_MAP_STATE=n`.

We only suppress when `CONFIG_NEED_DMA_MAP_STATE=n` in order to avoid
false negatives.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Bailey Forrest <redacted>

Hi Bailey,

I see that the warning still exists in linux-5.15-rc3 and net-next,
I'm building with my original patch[1] to get around the -Werror
warnings.

Can you resend your patch, or should I resend mine after all?

      Arnd

[1] https://lore.kernel.org/all/20210721151100.2042139-1-arnd@kernel.org/

Hi David/Jakub,

Any thoughts on my patch? I'm open to alternative suggestions for how
to resolve this.

This patch still works and merges cleanly on HEAD.

Thanks,
Bailey

On Mon, 27 Sep 2021 13:21:30 -0700 Bailey Forrest wrote:

Apologies, resending as text

On Mon, Sep 27, 2021 at 2:59 AM Arnd Bergmann [off-list ref] wrote:

quoted

On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest [off-list ref] wrote:  

quoted

Some variables become unused when `CONFIG_NEED_DMA_MAP_STATE=n`.

We only suppress when `CONFIG_NEED_DMA_MAP_STATE=n` in order to avoid
false negatives.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Bailey Forrest <redacted>  

Hi Bailey,

I see that the warning still exists in linux-5.15-rc3 and net-next,
I'm building with my original patch[1] to get around the -Werror
warnings.

Can you resend your patch, or should I resend mine after all?

      Arnd

[1] https://lore.kernel.org/all/20210721151100.2042139-1-arnd@kernel.org/  

Hi David/Jakub,

Any thoughts on my patch? I'm open to alternative suggestions for how
to resolve this.

This patch still works and merges cleanly on HEAD.

Looks like fixing this on the wrong end, dma_unmap_len_set() 
and friends should always evaluate their arguments.

On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski [off-list ref] wrote:

On Mon, 27 Sep 2021 13:21:30 -0700 Bailey Forrest wrote:

quoted

Apologies, resending as text

On Mon, Sep 27, 2021 at 2:59 AM Arnd Bergmann [off-list ref] wrote:

quoted

On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest [off-list ref] wrote:

quoted

Some variables become unused when `CONFIG_NEED_DMA_MAP_STATE=n`.

We only suppress when `CONFIG_NEED_DMA_MAP_STATE=n` in order to avoid
false negatives.

Fixes: a57e5de476be ("gve: DQO: Add TX path")
Signed-off-by: Bailey Forrest <redacted>

Hi Bailey,

I see that the warning still exists in linux-5.15-rc3 and net-next,
I'm building with my original patch[1] to get around the -Werror
warnings.

Can you resend your patch, or should I resend mine after all?

      Arnd

[1] https://lore.kernel.org/all/20210721151100.2042139-1-arnd@kernel.org/

Hi David/Jakub,

Any thoughts on my patch? I'm open to alternative suggestions for how
to resolve this.

This patch still works and merges cleanly on HEAD.

Looks like fixing this on the wrong end, dma_unmap_len_set()
and friends should always evaluate their arguments.

That makes sense.

Arnd, if you want to fix this inside of the dma_* macros, the
following diff resolves the errors reported for this driver:

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index dca2b1355bb1..f51eee0f678e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h

@@ -590,10 +590,21 @@ static inline int dma_mmap_wc(struct device *dev,
 #else
 #define DEFINE_DMA_UNMAP_ADDR(ADDR_NAME)
 #define DEFINE_DMA_UNMAP_LEN(LEN_NAME)
-#define dma_unmap_addr(PTR, ADDR_NAME)           (0)
-#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL)  do { } while (0)
-#define dma_unmap_len(PTR, LEN_NAME)             (0)
-#define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
+
+#define dma_unmap_addr(PTR, ADDR_NAME) ({ (void)PTR; 0; })
+
+#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL) do { \
+ (void)PTR; \
+ (void)VAL; \
+} while (0)
+
+#define dma_unmap_len(PTR, LEN_NAME) ({ (void)PTR; 0; })
+
+#define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { \
+ (void)PTR; \
+ (void)VAL; \
+} while (0)
+
 #endif

 #endif /* _LINUX_DMA_MAPPING_H */

On Tue, Sep 28, 2021 at 2:00 AM Bailey Forrest [off-list ref] wrote:

On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 27 Sep 2021 13:21:30 -0700 Bailey Forrest wrote:

Looks like fixing this on the wrong end, dma_unmap_len_set()
and friends should always evaluate their arguments.

That makes sense.

Arnd, if you want to fix this inside of the dma_* macros, the
following diff resolves the errors reported for this driver:

Unfortunately, this breaks every other driver using these macros, as the
point of them is that the unmap-address is not actually defined
and not taking up space in data structure. Referencing it by name
is a compile-time bug.

I've come up with a new patch to gve that just removes the
"struct gve_tx_dma_buf" and open-codes the access everywhere,
sending that as v2 now. Feel free to take that and modify as needed
before sending on, or doing yet another patch.

        Arnd

On Tue, Sep 28, 2021 at 7:15 AM Arnd Bergmann [off-list ref] wrote:

On Tue, Sep 28, 2021 at 2:00 AM Bailey Forrest [off-list ref] wrote:

quoted

On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Mon, 27 Sep 2021 13:21:30 -0700 Bailey Forrest wrote:

Looks like fixing this on the wrong end, dma_unmap_len_set()
and friends should always evaluate their arguments.

That makes sense.

Arnd, if you want to fix this inside of the dma_* macros, the
following diff resolves the errors reported for this driver:

quoted

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index dca2b1355bb1..f51eee0f678e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h

@@ -590,10 +590,21 @@ static inline int dma_mmap_wc(struct device *dev,
 #else
 #define DEFINE_DMA_UNMAP_ADDR(ADDR_NAME)
 #define DEFINE_DMA_UNMAP_LEN(LEN_NAME)
-#define dma_unmap_addr(PTR, ADDR_NAME)           (0)
-#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL)  do { } while (0)
-#define dma_unmap_len(PTR, LEN_NAME)             (0)
-#define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
+
+#define dma_unmap_addr(PTR, ADDR_NAME) ({ (void)PTR; 0; })
+
+#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL) do { \

Unfortunately, this breaks every other driver using these macros, as the
point of them is that the unmap-address is not actually defined
and not taking up space in data structure. Referencing it by name
is a compile-time bug.

My patch works with a small modification:

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index dca2b1355bb1..04ca5467562e 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -590,10 +590,10 @@ static inline int dma_mmap_wc(struct device *dev,
 #else
 #define DEFINE_DMA_UNMAP_ADDR(ADDR_NAME)
 #define DEFINE_DMA_UNMAP_LEN(LEN_NAME)
-#define dma_unmap_addr(PTR, ADDR_NAME)           (0)
-#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL)  do { } while (0)
-#define dma_unmap_len(PTR, LEN_NAME)             (0)
-#define dma_unmap_len_set(PTR, LEN_NAME, VAL)    do { } while (0)
+#define dma_unmap_addr(PTR, ADDR_NAME) ({ (void)PTR; 0; })
+#define dma_unmap_addr_set(PTR, ADDR_NAME, VAL) do { (void)PTR; } while (0)
+#define dma_unmap_len(PTR, LEN_NAME) ({ (void)PTR; 0; })
+#define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { (void)PTR; } while (0)
 #endif

 #endif /* _LINUX_DMA_MAPPING_H */

To me, this is still the preferred solution. However, your latest
patch looked fine to me.