| Message ID | 6358f849b5a1a3727879f4f2f5c855e3a5b95ab4.1428930614.git.varun@chelsio.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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@obsidianresearch.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. - Some of the commit message are short, or non existant (ie #4) - Generally, no need for 'static inline' in a .c file, the compiler knows what to do. Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Acked-by: Steve Wise <swise@opengridcomputing.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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@obsidianresearch.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. #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); > - Some of the commit message are short, or non existant (ie #4) > - Generally, no need for 'static inline' in a .c file, the compiler knows > what to do. > > Regards, > Jason > Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@obsidianresearch.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 linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h b/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h index 5e53327..210f04a 100644 --- a/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h +++ b/drivers/infiniband/hw/cxgb4/t4fw_ri_api.h @@ -818,15 +818,6 @@ struct cpl_pass_accept_req { #define SYN_INTF_V(x) ((x) << SYN_INTF_S) #define SYN_INTF_G(x) (((x) >> SYN_INTF_S) & SYN_INTF_M) -struct ulptx_idata { - __be32 cmd_more; - __be32 len; -}; - -#define ULPTX_NSGE_S 0 -#define ULPTX_NSGE_M 0xFFFF -#define ULPTX_NSGE_V(x) ((x) << ULPTX_NSGE_S) - #define RX_DACK_MODE_S 29 #define RX_DACK_MODE_M 0x3 #define RX_DACK_MODE_V(x) ((x) << RX_DACK_MODE_S) diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h index 30a2f56..7e2137d 100644 --- a/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_msg.h @@ -1054,6 +1054,11 @@ struct ulptx_sgl { struct ulptx_sge_pair sge[0]; }; +struct ulptx_idata { + __be32 cmd_more; + __be32 len; +}; + #define ULPTX_NSGE_S 0 #define ULPTX_NSGE_V(x) ((x) << ULPTX_NSGE_S) diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.h b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.h index 1096026..342263b 100644 --- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.h +++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.h @@ -23,11 +23,6 @@ #define CXGB4I_TX_HEADER_LEN \ (sizeof(struct fw_ofld_tx_data_wr) + sizeof(struct sge_opaque_hdr)) -struct ulptx_idata { - __be32 cmd_more; - __be32 len; -}; - struct cpl_rx_data_ddp { union opcode_tid ot; __be16 urg;
define struct ulptx_idata in common header file t4_msg.h to remove duplicate definitions. Signed-off-by: Varun Prakash <varun@chelsio.com> --- drivers/infiniband/hw/cxgb4/t4fw_ri_api.h | 9 --------- drivers/net/ethernet/chelsio/cxgb4/t4_msg.h | 5 +++++ drivers/scsi/cxgbi/cxgb4i/cxgb4i.h | 5 ----- 3 files changed, 5 insertions(+), 14 deletions(-)