[ofa-general] Re: [PATCH 2.6.30] RDMA/cxgb3: Handle EEH events for active connections.
Divy Le Ray
divy at chelsio.com
Fri Feb 20 13:27:28 PST 2009
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.
>
> > +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?
>
Hi Roland,
l2t_send() is used on connection setup/teardown path for iWARP, but is
the data path of the iSCSI offload module.
Cheers,
Divy
More information about the general
mailing list