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

[Steve Wise]

Roland Dreier wrote:
>  > -	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.
>
>   

Sure.

>  > +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.
>
>   


I'll change this to QP_FATAL then.


> 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)
>
>   


Ok.


>  > +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.
>   


Agreed.


>  > +		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.
>   

In looking at the l2t_send code, it doesn't free on failure, so I 
believe this was a memory leak in the existing error path.

> 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?
>
>   

Maybe.

Divy, what do you think?


Steve.


>  - R.
>