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.
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
