[openib-general] [PATCH] ping Add IB ping server agent

[Roland Dreier]

A few comments based on a quick read through of the code.

 > Index: ping_priv.h
 > ===================================================================
 > --- ping_priv.h	(revision 0)
 > +++ ping_priv.h	(revision 0)
 > +#include <linux/pci.h>
 > +
 > +#define SPFX "ib_ping: "
 > +
 > +struct ib_ping_send_wr {
 > +	struct list_head send_list;
 > +	struct ib_ah *ah;
 > +	struct ib_mad_private *mad;
 > +	DECLARE_PCI_UNMAP_ADDR(mapping)
 > +};
 > +
 > +struct ib_ping_port_private {
 > +	struct list_head port_list;
 > +	struct list_head send_posted_list;
 > +	spinlock_t send_list_lock;
 > +	int port_num;
 > +	struct ib_mad_agent *pingd_agent;     /* OpenIB Ping class */
 > +};

Is it worth having a separate include file that is only included in
one place for this small amount of declarations?

 > Index: ping.c
 > --- ping.c	(revision 0)
 > +++ ping.c	(revision 0)

 > +#include "mad_priv.h"

It doesn't seem right for a different module to be including the mad
module's private implementation details.

 > +/*
 > + * Caller must hold ib_ping_port_list_lock
 > + */
 > +static inline struct ib_ping_port_private *
 > +__ib_get_ping_port(struct ib_device *device, int port_num,
 > +		   struct ib_mad_agent *mad_agent)
 > +{
 > +	struct ib_ping_port_private *entry;
 > +
 > +	BUG_ON(!(!!device ^ !!mad_agent));  /* Exactly one MUST be (!NULL) */

Why have this complication of having a single function that can look
up by device or mad_agent?  For lookup by mad_agent it seems you
should just use mad_agent->context directly and not even call a
function to do that.

 > +
 > +	if (device) {
 > +		list_for_each_entry(entry, &ib_ping_port_list, port_list) {
 > +			if (entry->pingd_agent->device == device &&
 > +			    entry->port_num == port_num)
 > +				return entry;
 > +		}
 > +	} else {
 > +		list_for_each_entry(entry, &ib_ping_port_list, port_list) {
 > +			if (entry->pingd_agent == mad_agent)
 > +				return entry;
 > +		}
 > +	}
 > +	return NULL;
 > +}

 > +	/* PCI mapping */
 > +	gather_list.addr = dma_map_single(mad_agent->device->dma_device,

Is this comment useful?  It's pretty obvious what's going on here, and
it's not necessarily PCI mapping (the HCA could be on some other type
of bus).

 > +static void pingd_recv_handler(struct ib_mad_agent *mad_agent,
 > +			       struct ib_mad_recv_wc *mad_recv_wc)
 > +{
 > +	struct ib_ping_port_private	*port_priv;
 > +	struct ib_vendor_mad	*vend;
 > +	struct ib_mad_private *recv = container_of(mad_recv_wc,
 > +					struct ib_mad_private,
 > +					header.recv_wc);
 > +
 > +	/* Find matching MAD agent */
 > +	port_priv = ib_get_ping_port(NULL, 0, mad_agent);

just mad_agent->context as I said above.

 > +	if (!port_priv) {
 > +		kmem_cache_free(ib_mad_cache, recv);

should ib_free_recv_mad() -- there's a defined API, we should use it.

 > +		kmem_cache_free(ib_mad_cache, recv);

ditto.

 > +static void pingd_send_handler(struct ib_mad_agent *mad_agent,
 > +			       struct ib_mad_send_wc *mad_send_wc)
 > +{
 > +	struct ib_ping_port_private	*port_priv;
 > +	struct ib_ping_send_wr		*ping_send_wr;
 > +	unsigned long			flags;
 > +
 > +	/* Find matching MAD agent */
 > +	port_priv = ib_get_ping_port(NULL, 0, mad_agent);

mad_agent->context

 > +	/* Unmap PCI */
 > +	dma_unmap_single(mad_agent->device->dma_device,

Inaccurate and not helpful comment again.

 > +	/* Release allocated memory */
 > +	kmem_cache_free(ib_mad_cache, ping_send_wr->mad);

ib_free_recv_mad()

 > +int ib_ping_port_open(struct ib_device *device, int port_num)
 > +{
 > +	int ret;
 > +	struct ib_ping_port_private *port_priv;
 > +	struct ib_mad_reg_req pingd_reg_req;
 > +	unsigned long flags;
 > +
 > +	/* First, check if port already open */
 > +	port_priv = ib_get_ping_port(device, port_num, NULL);

I think you can trust the core not to call you twice for the same
device, no need to check this.

 > +	/* Obtain server MAD agent for OpenIB Ping class (GSI QP) */
 > +	port_priv->pingd_agent = ib_register_mad_agent(device, port_num,
 > +						       IB_QPT_GSI,
 > +						      &pingd_reg_req, 0,
 > +						      &pingd_send_handler,
 > +						      &pingd_recv_handler,
 > +						       NULL);

use port_priv instead of NULL for the context param.

 > +static void ib_ping_init_device(struct ib_device *device)
 > +{
 > +	int ret, num_ports, cur_port, i, ret2;

Why do you need ret and ret2?

 > +
 > +	if (device->node_type == IB_NODE_SWITCH) {
 > +		num_ports = 1;
 > +		cur_port = 0;
 > +	} else {
 > +		num_ports = device->phys_port_cnt;
 > +		cur_port = 1;
 > +	}
 > +
 > +	for (i = 0; i < num_ports; i++, cur_port++) {
 > +		ret = ib_ping_port_open(device, cur_port);
 > +		if (ret) {
 > +			printk(KERN_ERR SPFX "Couldn't open %s port %d\n",
 > +			       device->name, cur_port);
 > +			goto error_device_open;
 > +		}

why not just

		if (ib_ping_port_open(device, cur_port) {

?  You never use the value of ret for anything.

 > +	}
 > +	goto error_device_query;

Just return here -- this is the success case so don't obfuscate it.

 > +
 > +error_device_open:
 > +	while (i > 0) {
 > +		cur_port--;
 > +		ret2 = ib_ping_port_close(device, cur_port);
 > +		if (ret2) {

why not just

		if (ib_ping_port_close(device, cur_port)
			printk(...

 > +			printk(KERN_ERR PFX "Couldn't close %s port %d "
 > +			       "for ping agent\n",
 > +			       device->name, cur_port);
 > +		}
 > +		i--;
 > +	}
 > +
 > +error_device_query:

don't need this label at all.

 > +	return;
 > +}
 > +
 > +static void ib_ping_remove_device(struct ib_device *device)
 > +{
 > +	int ret = 0, i, num_ports, cur_port, ret2;

Why do you need ret or ret2?

 > +
 > +	if (device->node_type == IB_NODE_SWITCH) {
 > +		num_ports = 1;
 > +		cur_port = 0;
 > +	} else {
 > +		num_ports = device->phys_port_cnt;
 > +		cur_port = 1;
 > +	}
 > +	for (i = 0; i < num_ports; i++, cur_port++) {
 > +		ret2 = ib_ping_port_close(device, cur_port);
 > +		if (ret2) {
 > +			printk(KERN_ERR SPFX "Couldn't close %s port %d "
 > +			       "for ping agent\n",
 > +			       device->name, cur_port);
 > +			if (!ret)
 > +				ret = ret2;
 > +		}

How about just

		if (ib_ping_port_close(device, cur_port)
			printk(...

You don't care about the return value in ret2 and you don't do
anything with the value you put in ret as far as I can see.

 > +	}
 > +}
 > +
 > +static struct ib_client ping_client = {
 > +        .name   = "ping",
 > +        .add = ib_ping_init_device,
 > +        .remove = ib_ping_remove_device
 > +};
 > +
 > +static int __init ib_ping_init_module(void)
 > +{
 > +	spin_lock_init(&ib_ping_port_list_lock);
 > +	INIT_LIST_HEAD(&ib_ping_port_list);

INIT_LIST_HEAD() isn't needed if you declare your list with LIST_HEAD().

 > Index: mad.c
 > ===================================================================
 > --- mad.c	(revision 2023)
 > +++ mad.c	(working copy)
 > @@ -45,6 +45,8 @@
 >  
 >  
 >  kmem_cache_t *ib_mad_cache;
 > +EXPORT_SYMBOL(ib_mad_cache);

I don't think we should be exporting internals like this.

 - R.