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

[Tom Tucker]

Roland:

Thanks for the comments. I'll take a pass through given your review and
repost.

Thanks, again,

Tom

On Mon, 2006-01-23 at 22:07 -0800, Roland Dreier wrote:
> 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?

time-warp comment. 
> 
>  > +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 {
>  ...
> };
> 

got it.

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

yeah - I noticed this too.
> 
>  > +	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.

agreed. wmb()

> 
>  > +	/*
>  > +	 * 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().

agreed.

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

it's all over... vi vs. emacs wars.

> 
>  > +#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.
> 

yep.

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

we used to share firmware/device driver code... These could be reduced
to their Linux equivalent readl, etc....

> 
>  - R.