| Message ID | 20210723231957.1113800-1-bcf@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net] gve: DQO: Suppress unused var warnings | expand |
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/
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
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.
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 */
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
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.
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),
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(-)