[openib-general] Proposed device enumeration & async event APIs

[Fab Tillier]

> From: Roland Dreier [mailto:roland at topspin.com]
> Sent: Friday, September 10, 2004 1:35 PM
> 
> OK, here is my proposal for how to handle device enumeration and async
> events.  I actually have all of this coded and working on my branch;
> I'll post the full diff shortly.  However I want to pull out the API
> changes so we can discuss them more easily.

Thanks, this makes it a lot easier to comment on (at least for me).

> 
> First of all, here is how a kernel client finds out about the devices
> in the system:
> 
> 	struct ib_client {
> 		void (*add)   (struct ib_device *);
> 		void (*remove)(struct ib_device *);
> 
> 		struct list_head list;
> 	};
> 
> 	int ib_register_client  (struct ib_client *client);
> 	int ib_unregister_client(struct ib_client *client);
> 
> When a client calls ib_register_client, the add method is called
> for each of the devices that already exist.  Conversely, on
> unregister, remove  is called for all the remaining devices.  When
> a new device is added, add methods are be called in the order the
> clients registered; when a new device is removed, remove methods are
> called in the opposite order.  This allows initialization and cleanup
> to happen properly (for example, IPoIB knows that the MAD layer will
> initialize before it and clean up after it).

This sounds sane.  Are existing device notifications invoked from the thread
context of the ib_register_client function?  In other words, does the
ib_register_client function return before or after the client has receive
notifications of existing events?  Does ib_unregister_client synchronize
with callback delivery?  Does ib_unregister_client send "pretend" removal
events?

The advantage of having ib_unregister_client send these fake events is it
allows clients to have their state driven entirely by these callbacks.  This
would imply that the remove events get sent in the context of the caller.

> 
> For unaffiliated events, my API is as follows:
> 
> 	struct ib_event_handler {
> 		struct ib_device *device;
> 		void            (*handler)(struct ib_event_handler *, struct
> ib_event *);
> 		struct list_head  list;
> 	};
> 
> 	int ib_register_event_handler  (struct ib_device *device,
> 					struct ib_event_handler
*event_handler);

Why does ib_register_event_handler take a device as input?  Is this device
the same as event_handler.device?  Why not just use the event handler's
device instead?

> 	int ib_unregister_event_handler(struct ib_event_handler
> *event_handler);
> 	void ib_dispatch_event(struct ib_event *event);
> 
> This is pretty simple: everyone that wants to know about unaffiliated
> (ie not relating to a QP or CQ) events registers a struct
> ib_event_handler.  The callback doesn't take a context parameter
> because I'm assuming the struct ib_event_handler will be embedded in
> the client's context and used with container_of (this is similar to
> <linux/notifier.h>).

I think assuming the handler is embedded is sane.

> 
> Finally, I added event_handler members to struct ib_cq and struct
> ib_qp and added support for setting them on creation:
> 
> 	struct ib_cq {
> 		struct ib_device *device;
> 		ib_comp_handler   comp_handler;
> 		void             (*event_handler)(struct ib_event *, void
*);
> 		void *            context;
> 		int               cqe;
> 		atomic_t          usecnt; /* count number of work queues */
> 	};
> 
> 	struct ib_cq *ib_create_cq(struct ib_device *device,
> 				   ib_comp_handler comp_handler,
> 				   void (*event_handler)(struct ib_event *,
void
> *),
> 				   void *cq_context, int cqe);
> 
> 	struct ib_qp {
> 		struct ib_device       *device;
> 		struct ib_pd	       *pd;
> 		struct ib_cq	       *send_cq;
> 		struct ib_cq	       *recv_cq;
> 		struct ib_srq	       *srq;
> 		void                  (*event_handler)(struct ib_event *,
void
> *);
> 		void		       *qp_context;
> 		u32			qp_num;
> 	};
> 
> 	struct ib_qp_init_attr {
> 		void                  (*event_handler)(struct ib_event *,
void
> *);
> 		void		       *qp_context;
> 		struct ib_cq	       *send_cq;
> 		struct ib_cq	       *recv_cq;
> 		struct ib_srq	       *srq;
> 		struct ib_qp_cap	cap;
> 		enum ib_sig_type	sq_sig_type;
> 		enum ib_sig_type	rq_sig_type;
> 		enum ib_qp_type		qp_type;
> 		u8			port_num; /* special QP types only
*/
> 	};
> 
> These do get passed the context to match what we did with the
> comp_handler member of struct ib_cq.

Looks sane to me too.

- Fab