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

[Hal Rosenstock]

On Mon, 2005-03-21 at 10:57, Roland Dreier wrote: 
> 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?

Is the same true for agent.c/agent_priv.h (and agent_priv.h should be
eliminated) ?

>  > +	/* 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).

Similar for agent.c and mad.c.

>  > +	/* Unmap PCI */
>  > +	dma_unmap_single(mad_agent->device->dma_device,
> 
> Inaccurate and not helpful comment again.

Similar for agent.c and mad.c.

-- Hal