[ofa-general] Re: [PATCH 2.6.30] RDMA/cxgb3: Handle EEH events for active connections.

[Roland Dreier]

 > -	return (cxgb3_ofld_send(rdev_p->t3cdev_p, skb));
 > +	return (iwch_cxgb3_ofld_send(rdev_p->t3cdev_p, skb));

minor but the parens around the function call are totally unnecessary.
If we're touching the line anyway may as well leave them off.

 > +static int iwch_post_qp_fatal(int id, void *p, void *data)
 > +{
 > +	struct ib_event event;
 > +	struct iwch_qp *qhp = p;
 > +
 > +	event.event = IB_EVENT_DEVICE_FATAL;
 > +	event.device = qhp->ibqp.device;
 > +	event.element.qp = &qhp->ibqp;
 > +	BUG_ON(qhp->rhp != data);
 > +	BUG_ON(qhp->wq.qpid != id);
 > +	if (qhp->ibqp.event_handler) {
 > +		PDBG("%s posting DEVICE_FATAL for qpid %u\n",
 > +			__func__, qhp->wq.qpid);
 > +		(*qhp->ibqp.event_handler)(&event, qhp->ibqp.qp_context);

This doesn't match the IB driver behavior (or the IB spec) -- the
DEVICE_FATAL event is unaffiliated and delivered for the adapter as a
whole.  QP events are supposed to be for events connected to a single
QP, not the whole adapter failing.

BTW I don't think you need the * here, do you?  Would be easier to read
to just call it like

	qhp->ibqp.event_handler(&event, qhp->ibqp.qp_context)

 > +int iwch_l2t_send(struct t3cdev *tdev, struct sk_buff *skb, struct l2t_entry *l2e)
 > +{
 > +	int	error=0;
 > +	struct cxio_rdev *rdev;
 > +
 > +	rdev = (struct cxio_rdev *)tdev->ulp;
 > +	if (rdev->flags) {

Might be nice to wrap this rdev->flags test up in a trivial inline
function (eg iwch_eeh_set() or something like that) in case other things
get put into those flags later.

 > +		kfree_skb(skb);
 > +		return -EIO;
 > +	}
 > +	error = l2t_send(tdev, skb, l2e);
 > +	if (error)
 > +		kfree_skb(skb);
 > +	return error;
 > +}

The kfree_skb() calls here change behavior -- eg you have the change:

 > -	l2t_send(ep->com.tdev, skb, ep->l2t);
 > -	return 0;
 > +	return iwch_l2t_send(ep->com.tdev, skb, ep->l2t);

and now if l2t_send() returns an error the skb is freed, where before it
wasn't.

Also I'm wondering why you want these wrappers in iw_cxgb3 -- would it
not make more sense for the cxgb3 l2t_send() to check the eeh state and
always behave appropriately?  Or is it more complicated than that?

 - R.