| Message ID | 20241014150139.927423-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: systemport: avoid build warnings due to unused I/O helpers | expand |
On Mon, Oct 14, 2024 at 06:01:38PM +0300, Vladimir Oltean wrote: > A clang-16 W=1 build emits the following (abridged): > > warning: unused function 'txchk_readl' [-Wunused-function] > BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'txchk_writel' [-Wunused-function] > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'tbuf_readl' [-Wunused-function] > BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'tbuf_writel' [-Wunused-function] > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > Annotate the functions with the __maybe_unused attribute to tell the > compiler it's fine to do dead code elimination, and suppress the > warnings. > > Also, remove the "inline" keyword from C files, since the compiler is > free anyway to inline or not. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Simon Horman <horms@kernel.org> ...
On 10/14/24 08:01, Vladimir Oltean wrote: > A clang-16 W=1 build emits the following (abridged): > > warning: unused function 'txchk_readl' [-Wunused-function] > BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'txchk_writel' [-Wunused-function] > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'tbuf_readl' [-Wunused-function] > BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > warning: unused function 'tbuf_writel' [-Wunused-function] > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > Annotate the functions with the __maybe_unused attribute to tell the > compiler it's fine to do dead code elimination, and suppress the > warnings. > > Also, remove the "inline" keyword from C files, since the compiler is > free anyway to inline or not. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> clang is adequately warning that the txchk_{read,write}l functions are not used at all, so while your patch is correct, I think we could also go with this one liner in addition, or as a replacement to your patch: diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index c9faa8540859..7cea30eac83a 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -46,7 +46,6 @@ BCM_SYSPORT_IO_MACRO(umac, SYS_PORT_UMAC_OFFSET); BCM_SYSPORT_IO_MACRO(gib, SYS_PORT_GIB_OFFSET); BCM_SYSPORT_IO_MACRO(tdma, SYS_PORT_TDMA_OFFSET); BCM_SYSPORT_IO_MACRO(rxchk, SYS_PORT_RXCHK_OFFSET); -BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); BCM_SYSPORT_IO_MACRO(rbuf, SYS_PORT_RBUF_OFFSET); BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); BCM_SYSPORT_IO_MACRO(topctrl, SYS_PORT_TOPCTRL_OFFSET);
On Mon, Oct 14, 2024 at 09:56:25AM -0700, Florian Fainelli wrote: > On 10/14/24 08:01, Vladimir Oltean wrote: > > A clang-16 W=1 build emits the following (abridged): > > > > warning: unused function 'txchk_readl' [-Wunused-function] > > BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); > > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > > > warning: unused function 'txchk_writel' [-Wunused-function] > > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > > > warning: unused function 'tbuf_readl' [-Wunused-function] > > BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); > > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > > > warning: unused function 'tbuf_writel' [-Wunused-function] > > note: expanded from macro 'BCM_SYSPORT_IO_MACRO' > > > > Annotate the functions with the __maybe_unused attribute to tell the > > compiler it's fine to do dead code elimination, and suppress the > > warnings. > > > > Also, remove the "inline" keyword from C files, since the compiler is > > free anyway to inline or not. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > clang is adequately warning that the txchk_{read,write}l functions are not > used at all, so while your patch is correct, I think we could also go with > this one liner in addition, or as a replacement to your patch: > > diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c > b/drivers/net/ethernet/broadcom/bcmsysport.c > index c9faa8540859..7cea30eac83a 100644 > --- a/drivers/net/ethernet/broadcom/bcmsysport.c > +++ b/drivers/net/ethernet/broadcom/bcmsysport.c > @@ -46,7 +46,6 @@ BCM_SYSPORT_IO_MACRO(umac, SYS_PORT_UMAC_OFFSET); > BCM_SYSPORT_IO_MACRO(gib, SYS_PORT_GIB_OFFSET); > BCM_SYSPORT_IO_MACRO(tdma, SYS_PORT_TDMA_OFFSET); > BCM_SYSPORT_IO_MACRO(rxchk, SYS_PORT_RXCHK_OFFSET); > -BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); > BCM_SYSPORT_IO_MACRO(rbuf, SYS_PORT_RBUF_OFFSET); > BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); > BCM_SYSPORT_IO_MACRO(topctrl, SYS_PORT_TOPCTRL_OFFSET); > -- > Florian As a maintainer, you know best about how much to preserve from currently unused code, in the idea that it might get used later. Should I also delete this? diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index 9332a9390f0d..113d4251a243 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -46,9 +46,7 @@ BCM_SYSPORT_IO_MACRO(umac, SYS_PORT_UMAC_OFFSET); BCM_SYSPORT_IO_MACRO(gib, SYS_PORT_GIB_OFFSET); BCM_SYSPORT_IO_MACRO(tdma, SYS_PORT_TDMA_OFFSET); BCM_SYSPORT_IO_MACRO(rxchk, SYS_PORT_RXCHK_OFFSET); -BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); BCM_SYSPORT_IO_MACRO(rbuf, SYS_PORT_RBUF_OFFSET); -BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); BCM_SYSPORT_IO_MACRO(topctrl, SYS_PORT_TOPCTRL_OFFSET); /* On SYSTEMPORT Lite, any register after RDMA_STATUS has the exact diff --git a/drivers/net/ethernet/broadcom/bcmsysport.h b/drivers/net/ethernet/broadcom/bcmsysport.h index 335cf6631db5..55d72a16efcc 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.h +++ b/drivers/net/ethernet/broadcom/bcmsysport.h @@ -168,10 +168,6 @@ struct bcm_rsb { #define RXCHK_BRCM_TAG_CID_SHIFT 16 #define RXCHK_BRCM_TAG_CID_MASK 0xff -/* TXCHCK offsets and defines */ -#define SYS_PORT_TXCHK_OFFSET 0x380 -#define TXCHK_PKT_RDY_THRESH 0x00 - /* Receive buffer offset and defines */ #define SYS_PORT_RBUF_OFFSET 0x400 @@ -202,16 +198,6 @@ struct bcm_rsb { #define RBUF_OVFL_DISC_CNTR 0x0c #define RBUF_ERR_PKT_CNTR 0x10 -/* Transmit buffer offset and defines */ -#define SYS_PORT_TBUF_OFFSET 0x600 - -#define TBUF_CONTROL 0x00 -#define TBUF_BP_EN (1 << 0) -#define TBUF_MAX_PKT_THRESH_SHIFT 1 -#define TBUF_MAX_PKT_THRESH_MASK 0x1f -#define TBUF_FULL_THRESH_SHIFT 8 -#define TBUF_FULL_THRESH_MASK 0x1f - /* UniMAC offset and defines */ #define SYS_PORT_UMAC_OFFSET 0x800 They shouldn't contribute to the generated code size anyway. Plus, __maybe_unused would allow only the readl() or the writel() function to be used, which doesn't seem to have been needed so far, though. Something I haven't thought of, until now, is that if the I/O macros were defined as static inline in a header rather than C file, the compiler wouldn't have warned. So I guess that option is also on the table if we're keeping after all.
diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c index b45ed7cd2921..7d6e2c2ee445 100644 --- a/drivers/net/ethernet/broadcom/bcmsysport.c +++ b/drivers/net/ethernet/broadcom/bcmsysport.c @@ -29,13 +29,15 @@ /* I/O accessors register helpers */ #define BCM_SYSPORT_IO_MACRO(name, offset) \ -static inline u32 name##_readl(struct bcm_sysport_priv *priv, u32 off) \ +static u32 __maybe_unused \ +name##_readl(struct bcm_sysport_priv *priv, u32 off) \ { \ u32 reg = readl_relaxed(priv->base + offset + off); \ return reg; \ } \ -static inline void name##_writel(struct bcm_sysport_priv *priv, \ - u32 val, u32 off) \ + \ +static void __maybe_unused \ +name##_writel(struct bcm_sysport_priv *priv, u32 val, u32 off) \ { \ writel_relaxed(val, priv->base + offset + off); \ } \
A clang-16 W=1 build emits the following (abridged): warning: unused function 'txchk_readl' [-Wunused-function] BCM_SYSPORT_IO_MACRO(txchk, SYS_PORT_TXCHK_OFFSET); note: expanded from macro 'BCM_SYSPORT_IO_MACRO' warning: unused function 'txchk_writel' [-Wunused-function] note: expanded from macro 'BCM_SYSPORT_IO_MACRO' warning: unused function 'tbuf_readl' [-Wunused-function] BCM_SYSPORT_IO_MACRO(tbuf, SYS_PORT_TBUF_OFFSET); note: expanded from macro 'BCM_SYSPORT_IO_MACRO' warning: unused function 'tbuf_writel' [-Wunused-function] note: expanded from macro 'BCM_SYSPORT_IO_MACRO' Annotate the functions with the __maybe_unused attribute to tell the compiler it's fine to do dead code elimination, and suppress the warnings. Also, remove the "inline" keyword from C files, since the compiler is free anyway to inline or not. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/ethernet/broadcom/bcmsysport.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)