netdev - Re: [PATCH net-next v2 1/4] cxgb4/iw_cxgb4/cxgb4i: remove duplicate definitions

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: <20150415161234.GB4653@obsidianresearch.com>
Date:	Wed, 15 Apr 2015 10:12:34 -0600
From:	Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:	Varun Prakash <varun@...lsio.com>
Cc:	netdev@...r.kernel.org, linux-scsi@...r.kernel.org,
	linux-rdma@...r.kernel.org, davem@...emloft.net,
	JBottomley@...allels.com, roland@...estorage.com,
	leedom@...lsio.com, anish@...lsio.com, hariprasad@...lsio.com,
	swise@...ngridcomputing.com, kxie@...lsio.com,
	praveenm@...lsio.com, kumaras@...lsio.com
Subject: Re: [PATCH net-next v2 1/4] cxgb4/iw_cxgb4/cxgb4i: remove duplicate
 definitions

On Wed, Apr 15, 2015 at 06:34:27PM +0530, Varun Prakash wrote:
> On Mon, Apr 13, 2015 at 10:38:08AM -0600, Jason Gunthorpe wrote:
> > On Mon, Apr 13, 2015 at 07:34:23PM +0530, Varun Prakash wrote:
> > > define struct ulptx_idata in common header file t4_msg.h
> > > to remove duplicate definitions.
> > 
> > The Infiniband side of this patch looks OK.
> > 
> > Reviewed-By: Jason Gunthorpe <jgunthorpe@...idianresearch.com>
> > 
> > Just some random thoughts on the other patches:
> >  - Try and use 'if (IS_ENABLED(CONFIG_XX))' over #ifdef 
> >    to improve compile test coverage. This would drop a fair number
> >    of ifdefs.
> 
> FCoE specific structures and functions are defined only if 
> CONFIG_CHELSIO_T4_FCOE is enabled.

I saw a number of places that could be switched, you should look
through.

> #ifdef CONFIG_CHELSIO_T4_FCOE
> void cxgb_fcoe_init_ddp(struct adapter *adap)
> {
> 	...
> }
> #endif
> 
> If CONFIG_CHELSIO_T4_FCOE is disabled then following code will
> result in build error "implicit declaration of function cxgb_fcoe_init_ddp"
>
> if (IS_ENABLED(CONFIG_CHELSIO_T4_FCOE))
> 	cxgb_fcoe_init_ddp(adap);

Commonly the ifdef would be moved to the function header and a static
inline cxgb_fcoe_init_ddp(..) {} declared for the ifndef case. For the
case of static function calls, just drop the ifdefing entirely. The
compiler will silently drop unused static functions. Think carefully
about ifdefing out structure members - I don't think saving a few bytes of
heap in a driver like cxgb4 is worthwhile.

I also just noticed that

drivers/net/ethernet/chelsio/cxgb4/cxgb4_fcoe.c

Is wrapped in a whole file #ifdef CONFIG_CHELSIO_T4_FCOE - please use
the makefile instead for this.

Jason
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html