[openib-general] InfiniPath driver announcement

[Robert Walsh]

Some comments on the patch:

> +	/* Don't let userspace make us allocate a huge buffer */
> +	if (cmd.ne > 256)
> +		return -ENOMEM;
> +

Is this necessary?  Won't the following fail with ENOMEM anyway if
cmd.ne is too big:

>  	wc = kmalloc(cmd.ne * sizeof *wc, GFP_KERNEL);
>  	if (!wc)
>  		return -ENOMEM;

Same here:

> +	/* Don't let userspace make us allocate a huge buffer */
> +	if (cmd.wqe_size > 4096)
> +		return -ENOMEM;
> +
> +	user_wr = kmalloc(cmd.wqe_size, GFP_KERNEL);
> +	if (!user_wr)
> +		return -ENOMEM;
> +

What meaning do those numbers have, exactly?  i.e. the 4096 number
above?

> +	if (ret) {
> +		for (next = wr; next; next = next->next) {
> +			if (next == bad_wr)
> +				break;
> +			++resp.bad_wr;
> +		}
> +	}

Will this work?  If bad_wr is the first wr, then resp.bad_wr will be
zero.  The current user code (which has to change anyway) assumes 0 ==
"no bad wr" and 1 == "the first wr is the bad wr", etc.
 
> +	while (wr) {
> +		next = wr->next;
> +		kfree(wr);
> +		wr = next;
> +	}
> +
> +	kfree(user_wr);

One reason why I originally allocated one big wr area instead of a bunch
of smaller ones was to keep the cost of this down.  Is it a good idea to
be doing this with a bunch of kmallocs?


This is all I've had a chance to look at for the moment.  More later.

Regards,
 Robert.

-- 
Robert Walsh                                 Email: rjwalsh at pathscale.com
PathScale, Inc.                              Phone: +1 650 934 8117
2071 Stierlin Court, Suite 200                 Fax: +1 650 428 1969
Mountain View, CA 94043
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 481 bytes
Desc: This is a digitally signed message part
URL: <http://lists.openfabrics.org/pipermail/general/attachments/20051011/a8ff7520/attachment.sig>