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:[EMAIL PROTECTED] > Sent: Monday, August 08, 2005 8:52 AM > To: Tom Tucker > Cc: [email protected] > 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(). _______________________________________________ openib-general mailing list [email protected] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
