[openib-general] [PATCH 2 of 3] mad: large RMPP support
Roland Dreier
rdreier at cisco.com
Tue Feb 7 10:18:39 PST 2006
> + rmpp_mad = (struct ib_rmpp_mad *)seg_buf->mad;
Trivial, but I prefer a space after cast operators.
> +static struct ib_umad_packet *alloc_packet(void)
> +{
> + struct ib_umad_packet *packet;
> + int length = sizeof *packet + sizeof(struct ib_mad);
> +
> + packet = kzalloc(length, GFP_KERNEL);
> + if (!packet) {
> + printk(KERN_ERR "alloc_packet: mem alloc failed for length %d\n",
> + length);
> + return NULL;
> + }
> + INIT_LIST_HEAD(&packet->seg_list);
> + return packet;
> +}
This seems a little too big for what it's actually doing. Also, do we
really need to print a kernel error when the system is out of memory?
I would just write this as:
static struct ib_umad_packet *alloc_packet(void)
{
struct ib_umad_packet *packet;
packet = kzalloc(length, GFP_KERNEL);
if (!packet)
return NULL;
INIT_LIST_HEAD(&packet->seg_list);
return packet;
}
Also the chunk that deletes the old definition of alloc_packet() seems
to be in the next patch, so the tree won't compile without both
patches (which will be a pain from someone doing git bisect).
> + /* User buffer too small. Return first RMPP segment (which
> + * includes RMPP message length).
> + */
Trivial again but please use the comment format
/*
* foo
*/
in other words put the opening "/*" on a line by itself.
> + /* multipacket RMPP MAD message. Copy remainder of message.
> + * Note that last segment may have a shorter payload.
> + */
same here
- R.
More information about the general
mailing list