[openib-general] iWARP Branch Proposal

[Roland Dreier]

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.

- 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?

- 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)?

  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?

- 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().