netdev - Re: [PATCH net-next] net: systemport: avoid build warnings due to unused I/O helpers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241014174056.sxcgikf4hhw2olp7@skbuf>
Date: Mon, 14 Oct 2024 20:40:56 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: netdev@...r.kernel.org,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: systemport: avoid build warnings due to
 unused I/O helpers

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