[ofw] IBAL CM ignores responder resources of the passive side

[Leonid Keller]

I'd say, it's not "broken" applications, but applications, that do not care what Responder Resources should be.
And if an application in a third company works with previous drivers on 1000 computers and now they get new drivers and install them on 1000 computers and suddenly the application fails ...

I'm going to add in our code the following lines:

 ASSERT(p_cm_rep->resp_res);
 if (p_cm_rep->resp_res)
 	p_cep->resp_res = p_cm_rep->resp_res;

You may think of leaving in the OFED code only the last line.


> -----Original Message-----
> From: Fab Tillier [mailto:ftillier at microsoft.com]
> Sent: Thursday, June 23, 2011 2:53 AM
> To: Leonid Keller; Hefty, Sean
> Cc: ofw_list
> Subject: RE: IBAL CM ignores responder resources of the passive side
> 
> I'd just let existing broken apps get fixed and fix it in the CM
> properly.  That is, do the straight assignment, not the conditional
> assignment.
> 
> Cheers,
> -Fab
> 
> -----Original Message-----
> From: Leonid Keller [mailto:leonid at mellanox.co.il]
> Sent: Wednesday, June 22, 2011 10:03 AM
> To: Hefty, Sean; Fab Tillier
> Cc: ofw_list
> Subject: RE: IBAL CM ignores responder resources of the passive side
> 
> I'm afraid that just
> 
> p_cep->resp_res = p_cm_rep->resp_res;
> 
> can break some existing applications, which do not fill p_cm_rep-
> >resp_res and still work.
> Maybe
> 
> if (p_cm_rep->resp_res)
> 	p_cep->resp_res = p_cm_rep->resp_res;
> 
> > -----Original Message-----
> > From: Hefty, Sean [mailto:sean.hefty at intel.com]
> > Sent: Wednesday, June 22, 2011 7:04 PM
> > To: Leonid Keller; Fab Tillier
> > Cc: ofw_list
> > Subject: RE: IBAL CM ignores responder resources of the passive side
> >
> > without looking at more of the code, it looks like a bug
> >
> >
> > > __save_user_rep(
> > >
> > >                 IN
> > > cep_agent_t* const
> p_port_cep,
> > >
> > >                 IN
> > > kcep_t* const
> > p_cep,
> > >
> > >                 IN                           const     iba_cm_rep*
> > const
> > > p_cm_rep,
> > >
> > >                 IN
> > > uint8_t
> > > rnr_nak_timeout )
> > >
> > > {
> > >
> > >                 AL_ENTER( AL_DBG_CM );
> > >
> > >
> > >
> > >                 p_cep->local_qpn = p_cm_rep->qpn;
> > >
> > >                 p_cep->rq_psn = p_cm_rep->starting_psn;
> > >
> > >                 p_cep->init_depth = p_cm_rep->init_depth;
> > >
> > >
> > >
> >
> >
> > I think we can just always do the assignment:
> >
> > 	p_cep->resp_res = p_cm_rep->resp_res;
> >
> > here.  Is the p_cep->resp_res the value that actually goes into the
> > REP?
> >
> >
> > >                 ci_ca_lock_attr( p_port_cep->h_ca->obj.p_ci_ca );
> > >
> > >                 /* Check the CA's responder resource max and trim
> if
> > > necessary. */
> > >
> > >                 if( p_port_cep->h_ca->obj.p_ci_ca->p_pnp_attr-
> > > >max_qp_resp_res <
> >
> >
> > whine: The max_qp_resp_res isn't going to dynamically change.  We
> > should use a saved value somewhere that doesn't require serializing
> all
> > CM users for this check.
> >
> > - Sean