[openib-general] [PATCH] RFC: AMSO1100 iWARP Driver

[Roland Dreier]

Tom, thanks for posting this.  After thinking this over, I really
think the amso1100 driver belongs upstream.  If Linux is shipping
ARCnet and Sound Blaster CD-ROM support, then you've got a long wait
until your card is obsolete enough to forget about.  And having a real
iWARP driver just makes things a lot easier to justify and understand,
although I would still like to get buy-in from people like NetEffect
and Chelsio.

Anyway, some easy comments from a quick skim:

 > +/*
 > + * WARNING: If you change this file, also bump CC_IVN_BASE
 > + * in common/include/clustercore/cc_ivn.h.
 > + */

Uh, where's clustercore/cc_ivn.h?

 > +typedef enum {
 >  ...
 > +} cc_event_id_t;

 > +typedef enum {
 >  ...
 > +} cc_resource_indicator_t;

typedefs that create foo_t are strongly deprecated in the kernel.
Just do

enum cc_event_id {
 ...
};

and use "enum cc_event_id" everywhere.

 > +	switch (mq_index) {
 > +		case (0):

no need for parentheses here.  and can the magic (0), (1), (2) be
given names that say what they mean?

 > +	struct c2_mq_shared volatile *shared;

volatile declarations are almost always a bug... use proper locking or
memory barriers to say what you mean instead.

 > +	/*
 > +	 * Now read back shared->armed to make the PCI
 > +	 * write synchronous.  This is necessary for
 > +	 * correct cq notification semantics.
 > +	 */
 > +	{
 > +		volatile char c;
 > +		c  = shared->armed;
 > +	}

If you're reading across PCI you should be using readb().

 > +	qp->adapter_handle = reply->qp_handle;
 > +	qp->state = IB_QPS_RESET;
 > +        qp->send_sgl_depth = qp_attrs->cap.max_send_sge;
 > +        qp->rdma_write_sgl_depth = qp_attrs->cap.max_send_sge;
 > +        qp->recv_sgl_depth = qp_attrs->cap.max_recv_sge;

whitespace damage alert

 > +#define assert(expr)                                                  \
 > +    if(!(expr)) {                                                     \
 > +        printk(KERN_ERR PFX "Assertion failed! %s, %s, %s, line %d\n",\
 > +               #expr, __FILE__, __FUNCTION__, __LINE__);              \
 > +    }

probably just use BUG_ON() -- then you get a traceback too.

 > +struct c2_adapter_pci_regs {
 > +    char reg_magic[8];
 > +    u32  version;
 > +    u32  ivn;
 > +    u32  pci_window_size;
 > +    u32  q0_q_size;

Indent with tabs not 4 spaces (lots of other places too)

 > +static inline u32 c2_read32(const void __iomem *addr)
 > +{
 > +	return readl(addr);
 > +}

Any reason for not using readl() directly (and similarly for all the
other c2_readxx c2_writexx funcs)?

 - R.