[openib-general] [PATCH 4/7] AMSO1100 Provider Interface REPOST

[Sean Hefty]

> +struct c2_mr {
> +	struct ib_mr ibmr;
> +	struct c2_pd *pd;
> +};

ib_mr references the ib_pd, which can get you to the c2_pd.  You may be able to 
eliminate the c2_mr structure.

> +struct c2_ah {
> +	struct ib_ah ibah;
> +};

Can we eliminate c2_ah?  I don't have a strong preference here though.

> +int c2_llp_service_destroy(struct iw_cm_id *cm_id)

Are the functions in this file exported, or could they be static?

{snip}

> +	/*
> +	 * reference the request struct.  dereferenced in the int handler.
> +	 */
> +	vq_req_get(c2dev, vq_req);
> +
> +	/*
> +	 * Send WR to adapter
> +	 */
> +	err = vq_send_wr(c2dev, (union c2wr *) & wr);
> +	if (err) {
> +		vq_req_put(c2dev, vq_req);
> +		goto bail0;
> +	}
> +
> +	/*
> +	 * Wait for reply from adapter
> +	 */
> +	err = vq_wait_for_reply(c2dev, vq_req);
> +	if (err)
> +		goto bail0;
> +
> +	/*
> +	 * Process reply 
> +	 */
> +	reply = (struct c2wr_cr_reject_rep *) (unsigned long) vq_req->reply_msg;
> +	if (!reply) {
> +		err = -ENOMEM;
> +		goto bail0;
> +	}
> +	err = c2_errno(reply);

The basic code fragment above is repeated a couple of times.

> +static int c2_connect(struct iw_cm_id *cm_id, const void *pdata, u8 pdata_len)
> +static int c2_disconnect(struct iw_cm_id *cm_id, int abrupt)
> +static int c2_accept(struct iw_cm_id *cm_id, const void *pdata, u8 pdata_len)
> +static int c2_reject(struct iw_cm_id *cm_id, const void *pdata, u8 pdata_len)
> +static int c2_service_create(struct iw_cm_id *cm_id, int backlog)
> +static int c2_service_destroy(struct iw_cm_id *cm_id)

These are almost just wrappers around other calls.  I'm not familiar with the 
driver architecture at this point, but could these be merged with the underlying 
calls?

> +int c2_pd_alloc(struct c2_dev *dev, int privileged, struct c2_pd *pd)
> +void c2_pd_free(struct c2_dev *dev, struct c2_pd *pd)
> +int __devinit c2_init_pd_table(struct c2_dev *dev)
> +void __devexit c2_cleanup_pd_table(struct c2_dev *dev)

Same comment as previous.

> +static int
> +move_sgl(struct c2_data_addr * dst, struct ib_sge *src, int count, u32 * p_len,
> +	 u8 * actual_count)
> +{
> +	u32 tot = 0;		/* running total */
> +	u8 acount = 0;		/* running total non-0 len sge's */
> +
> +	while (count > 0) {
> +		/*
> +		 * If the addition of this SGE causes the
> +		 * total SGL length to exceed 2^32-1, then
> +		 * fail-n-bail.
> +		 *
> +		 * If the current total plus the next element length
> +		 * wraps, then it will go negative and be less than the
> +		 * current total...
> +		 */
> +		if ((tot + src->length) < tot) {
> +			return -EINVAL;
> +		}
> +		/*
> +		 * Bug: 1456 (as well as 1498 & 1643)
> +		 * Skip over any sge's supplied with len=0
> +		 */
> +		if (src->length) {
> +			tot += src->length;
> +			dst->stag = cpu_to_be32(src->lkey);
> +			dst->to = cpu_to_be64(src->addr);
> +			dst->length = cpu_to_be32(src->length);
> +			dst++;
> +			acount++;
> +		}
> +		src++;
> +		count--;
> +	}
> +
> +	if (acount == 0) {
> +		/*
> +		 * Bug: 1476 (as well as 1498, 1456 and 1643)

nit: the bug numbers don't make much sense to me.

> +		 * Setup the SGL in the WR to make it easier for the RNIC.
> +		 * This way, the FW doesn't have to deal with special cases.
> +		 * Setting length=0 should be sufficient.
> +		 */
> +		dst->stag = 0;
> +		dst->to = 0;
> +		dst->length = 0;
> +	}
> +
> +	*p_len = tot;
> +	*actual_count = acount;
> +	return 0;
> +}

> +/*
> + * Function: c2_activity (private function)
> + *
> + * Description: 
> + * Post an mq index to the host->adapter activity fifo.
> + *
> + * IN: 
> + * c2dev	- ptr to c2dev structure
> + * mq_index	- mq index to post
> + * shared	- value most recently written to shared 
> + *
> + * OUT: 
> + *
> + * Return: 
> + * none
> + */

Probably want to use docbook format for defining functions here and elsewhere.

- Sean