netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] gve: DQO: Suppress unused var warnings
@ 2021-07-23 23:19 Bailey Forrest
  2021-09-27  9:58 ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Bailey Forrest @ 2021-07-23 23:19 UTC (permalink / raw)
  To: Bailey Forrest, David S . Miller, Jakub Kicinski, Arnd Bergmann; +Cc: netdev

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 <bcf@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-07-23 23:19 [PATCH net] gve: DQO: Suppress unused var warnings Bailey Forrest
@ 2021-09-27  9:58 ` Arnd Bergmann
  2021-09-27 20:21   ` Bailey Forrest
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2021-09-27  9:58 UTC (permalink / raw)
  To: Bailey Forrest; +Cc: David S . Miller, Jakub Kicinski, Networking

On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest <bcf@google.com> 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 <bcf@google.com>

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/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-09-27  9:58 ` Arnd Bergmann
@ 2021-09-27 20:21   ` Bailey Forrest
  2021-09-27 23:21     ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Bailey Forrest @ 2021-09-27 20:21 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: David S . Miller, Jakub Kicinski, Networking

Apologies, resending as text

On Mon, Sep 27, 2021 at 2:59 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest <bcf@google.com> 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 <bcf@google.com>
>
> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-09-27 20:21   ` Bailey Forrest
@ 2021-09-27 23:21     ` Jakub Kicinski
  2021-09-27 23:59       ` Bailey Forrest
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-09-27 23:21 UTC (permalink / raw)
  To: Bailey Forrest; +Cc: Arnd Bergmann, David S . Miller, Networking

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 <arnd@kernel.org> wrote:
> >
> > On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest <bcf@google.com> 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 <bcf@google.com>  
> >
> > 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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-09-27 23:21     ` Jakub Kicinski
@ 2021-09-27 23:59       ` Bailey Forrest
  2021-09-28 14:15         ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Bailey Forrest @ 2021-09-27 23:59 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: Arnd Bergmann, David S . Miller, Networking

On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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 <arnd@kernel.org> wrote:
> > >
> > > On Sat, Jul 24, 2021 at 1:19 AM Bailey Forrest <bcf@google.com> 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 <bcf@google.com>
> > >
> > > 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 */

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-09-27 23:59       ` Bailey Forrest
@ 2021-09-28 14:15         ` Arnd Bergmann
  2021-09-28 20:04           ` Bailey Forrest
  0 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2021-09-28 14:15 UTC (permalink / raw)
  To: Bailey Forrest; +Cc: Jakub Kicinski, David S . Miller, Networking

On Tue, Sep 28, 2021 at 2:00 AM Bailey Forrest <bcf@google.com> wrote:
> On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > 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:

> 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.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net] gve: DQO: Suppress unused var warnings
  2021-09-28 14:15         ` Arnd Bergmann
@ 2021-09-28 20:04           ` Bailey Forrest
  0 siblings, 0 replies; 7+ messages in thread
From: Bailey Forrest @ 2021-09-28 20:04 UTC (permalink / raw)
  To: Arnd Bergmann; +Cc: Jakub Kicinski, David S . Miller, Networking

On Tue, Sep 28, 2021 at 7:15 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Tue, Sep 28, 2021 at 2:00 AM Bailey Forrest <bcf@google.com> wrote:
> > On Mon, Sep 27, 2021 at 4:21 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > 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:
>
> > 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.

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-28 20:04 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 23:19 [PATCH net] gve: DQO: Suppress unused var warnings Bailey Forrest
2021-09-27  9:58 ` Arnd Bergmann
2021-09-27 20:21   ` Bailey Forrest
2021-09-27 23:21     ` Jakub Kicinski
2021-09-27 23:59       ` Bailey Forrest
2021-09-28 14:15         ` Arnd Bergmann
2021-09-28 20:04           ` Bailey Forrest

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).