[openib-general] iWARP Branch Proposal

[Tom Tucker]

Roland:

Awesome list! Thanks for the quick and thorough review. 

I'll add it to a TODO file in the branch and we'll keep track of our
progress against these items.  

Answers to the questions follow...

TomT

> -----Original Message-----
> From: Roland Dreier [mailto:rolandd at cisco.com]
> Sent: Monday, August 08, 2005 8:52 AM
> To: Tom Tucker
> Cc: openib-general at openib.org
> Subject: Re: [openib-general] iWARP Branch Proposal
> 
> Tom, thanks for posting your device driver so quickly.  I realize that
> this is work-in-progress, and with that in mind I skimmed through the
> code and came up with some suggestions for your to-do list:
> 
> - Kconfig: replace 'mthca' with 'ams1100' in help text :)
> 
> - Replace all // comments with /* */.
> 
> - Why are members of cc_pci_regs_t and cc_adapter_pci_regs_t volatile?
>   Volatile declarations are almost inevitably buggy.  It's better to
>   use ordered accessors (readl(), writel(), etc) or insert explicit
>   memory barriers.
[Tom] will do.
> 
> - Get rid of most typedefs -- for example
> 
> 	typedef enum {
> 		...
> 	} foo_t;
> 
>   should become
> 
> 	enum foo {
>         	...
> 	};
> 
> - Can cc_byteorder.h be eliminated?  Most of the wrappers are
>   definitely superfluous.  Can the WR byte order ever change?  ie are
>   the cpu_to_wrXX() functions actually a useful abstraction?
[Tom] no, but the code was written when this was still in flux...
> 
> - Most of cc_common.h can be removed -- eg CC_SLEEP, CC_PRINT are
useless
> 
> - In ccil_api.h, rather than "#ifdef CC_LITTLE_ENDIAN" just use
>   __constant_cpu_to_b32 or whatever.
> 
>   Most of the "PACKED" attributes are unnecessary (since the
>   structures are already aligned).  They'll lead to horrible code with
>   gcc on ia64 etc.
> 
> - In general, all the #ifdef X86_64 in the code looks like portability
> bugs.
>   The best solution is just to make the code 32/64 clean, but at least
>   we need to replace the test with #ifdef CONFIG_COMPAT
> 
> - ccilnet_dbg.c is an awful lot of code just for debugging.
> 
> - Get rid of compatibility with Linux 2.4 -- everywhere there's a test
>   of LINUX_VERSION_CODE, just take the 2.6 code.
> 
> - cc_mq_common.c: BUMP is pretty inefficient, does a divide every time
> 
> - cc_qp_common.c: cc_memcpy8 corrupts FPU state, is it really needed?
>   it's never called.  Why is it declared in cc_mq_common.c?
>   memcpy4 similarly corrupts state.  If it's fixed to save CR0 and do
>   clts, is it really faster than a normal memcpy (considering it also
>   disables IRQs)?
> 
[Tom] This file was shared between the user and kernel portions of our
standalone code. It will get incorporated into the devccil_qp.c file.

>   This is all utterly non-portably anyway -- there needs to be a
>   standard fallback for PPC64, IA64 etc.
> 
> - Why is cc_queue.h needed?  What is <linux/list.h> missing?
[Tom] nada, but the code was originally intended to be OS agnostic.
> 
> - cc_types.h: get rid of NULL, TRUE, FALSE defines, cc_bool_t, etc.
>   PTR_TO_CTX, CC_PTR_TO_64, etc seem busted on 64-bit archs.
>   forget the macros, just cast to/from (unsigned long).
> 
> - devccil_adapter.c: adapter_list seems to be a latent bug -- why does
>   the driver need to keep a global list of adapters?
> 
> - devccil.c: What is 'Static' -- why not 'static'??
>   Probably all this code gets replaced by existing uverbs anyway.
> 
> - devccil_lock.h: Get rid of lock wrappers, they're just
>   obfuscation. And it seems CCTHREADSAFE won't ever be defined??
>   The driver should always be threadsafe.
> 
> - devccil_mem.c: ccil_big_malloc is horrible -- need a way to use
>   non-contig memory.
> 
> - devccil_srq.c: CCERR_NOT_IMPLEMENTED stubs not needed -- the
>   existing midlayer will return -ENOSYS for unimplemented functions
>   (ie NULL pointers in ib_device method table).
> 
> - devccil_var.h: Defining your own version of PAGE_SHIFT etc. is just
>   obfuscation; <linux/kernel.h> probably has all rounding functions
>   you need.  Custom ASSERT() can probably just be replaced with
>   standard BUG_ON().