<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=US-ASCII">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: [PATCH] client reregister </TITLE>
</HEAD>
<BODY>
<BR>
<BR>
<P><FONT SIZE=2>> -----Original Message-----</FONT>
<BR><FONT SIZE=2>> From: Fab Tillier [<A HREF="mailto:ftillier@silverstorm.com">mailto:ftillier@silverstorm.com</A>] </FONT>
<BR><FONT SIZE=2>> Sent: Monday, September 12, 2005 11:00 PM</FONT>
<BR><FONT SIZE=2>> To: 'Yossi Leybovich'</FONT>
<BR><FONT SIZE=2>> Cc: openib-windows@openib.org</FONT>
<BR><FONT SIZE=2>> Subject: RE: [PATCH] client reregister </FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > From: Yossi Leybovich [<A HREF="mailto:sleybo@mellanox.co.il">mailto:sleybo@mellanox.co.il</A>]</FONT>
<BR><FONT SIZE=2>> > Sent: Monday, September 12, 2005 10:20 AM</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Fab</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > Attach is patch for supporting client reregister event in the HCA , </FONT>
<BR><FONT SIZE=2>> > PnP manager and in the IPoIB I followed your recommendation to </FONT>
<BR><FONT SIZE=2>> > implement reregistration just in the level of IPoIB till </FONT>
<BR><FONT SIZE=2>> you will have </FONT>
<BR><FONT SIZE=2>> > the full design to the mcast/service_record mngr in the AL</FONT>
<BR><FONT SIZE=2>> ></FONT>
<BR><FONT SIZE=2>> > Main changes:</FONT>
<BR><FONT SIZE=2>> > 1. for the HCA async event I add the support to parse the </FONT>
<BR><FONT SIZE=2>> event, but I </FONT>
<BR><FONT SIZE=2>> > didn't have the chance to check that it actually work (I encounter </FONT>
<BR><FONT SIZE=2>> > problem to get async events from the HCA , I don't get the </FONT>
<BR><FONT SIZE=2>> cq async </FONT>
<BR><FONT SIZE=2>> > events as well. I will debug that later thus week) The code </FONT>
<BR><FONT SIZE=2>> just add </FONT>
<BR><FONT SIZE=2>> > defines and cases and I think t safe to add it.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> This code looks fine to me. If you want, we can split this </FONT>
<BR><FONT SIZE=2>> patch into three parts - adding reregister support to IBAL, </FONT>
<BR><FONT SIZE=2>> to the HCA driver, and to IPoIB. This might help keep the </FONT>
<BR><FONT SIZE=2>> discussions more focused. It's up to you, though.</FONT>
</P>
<P><FONT SIZE=2>I don't really mind , of course the important part is of the IPoIB.</FONT>
</P>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > 2. In PnP manager I trapped the port_info and set flag to </FONT>
<BR><FONT SIZE=2>> signal that </FONT>
<BR><FONT SIZE=2>> > client reregister was set by the SM .</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Is there any difference in client behavior between SM_CHANGE </FONT>
<BR><FONT SIZE=2>> and CLIENT_REREGISTER? If not, can we use SM_CHANGE for </FONT>
<BR><FONT SIZE=2>> both? In either case, the SM has lost all information about </FONT>
<BR><FONT SIZE=2>> the end node. Is the only difference that the SM LID didn't </FONT>
<BR><FONT SIZE=2>> change? If so, rather than introducing an internal field in </FONT>
<BR><FONT SIZE=2>> a public structure, why not set the SM LID to zero in the PnP </FONT>
<BR><FONT SIZE=2>> manager's cached attributes? This would cause the poll </FONT>
<BR><FONT SIZE=2>> operation to detect the change as an SM_CHANGE, thus </FONT>
<BR><FONT SIZE=2>> collapsing the two events into one.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>You right the what really matter is that the SM lost all privios records. </FONT>
<BR><FONT SIZE=2>So you can remove the new field and set the LID to zero</FONT>
</P>
<P><FONT SIZE=2>> > 3. In the IPoIB after getting the PNP_CLIENT_REREGISTER </FONT>
<BR><FONT SIZE=2>> event I remove </FONT>
<BR><FONT SIZE=2>> > the bcast entry and issue get_bcast again. After the IpoIB join the </FONT>
<BR><FONT SIZE=2>> > bcast group it register the IP address and refresh the </FONT>
<BR><FONT SIZE=2>> mcast array (as </FONT>
<BR><FONT SIZE=2>> > it do in the PNP_ACTIVE).</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Since neither the mcast or service registrations where </FONT>
<BR><FONT SIZE=2>> flushed, refresh_mcast and reg_addrs will do nothing. We </FONT>
<BR><FONT SIZE=2>> need to add support in IBAL to reregister these given an </FONT>
<BR><FONT SIZE=2>> existing object handle, or add code to IPoIB to first </FONT>
<BR><FONT SIZE=2>> deregister and then reregister. I would prefer to add this to IBAL.</FONT>
</P>
<P><FONT SIZE=2>OK . I will set the adaper->hang to TRUE as you suggets.</FONT>
<BR><FONT SIZE=2>That will solve the problem.</FONT>
<BR><FONT SIZE=2>After we will implement the reregistration in IBAL we can remove this.</FONT>
</P>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > Of course that wont be needed after we will implement the </FONT>
<BR><FONT SIZE=2>> > mcast/service_records reregistration mngr in the AL (BTW </FONT>
<BR><FONT SIZE=2>> did you find </FONT>
<BR><FONT SIZE=2>> > the time to put some initial design draft)</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> An alternative to these changes in IPoIB is to do something </FONT>
<BR><FONT SIZE=2>> similar to what the Linux driver does - down the interface </FONT>
<BR><FONT SIZE=2>> and then up it again. The easiest way to do this is to punt </FONT>
<BR><FONT SIZE=2>> on NDIS and set the adapter's hung flag. This flag will </FONT>
<BR><FONT SIZE=2>> cause NDIS to reset the adapter, which will re-register </FONT>
<BR><FONT SIZE=2>> everything properly.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> This is probably the simplest solution, albeit a little </FONT>
<BR><FONT SIZE=2>> crude. It will get the work done, though, and we can refine it later.</FONT>
</P>
<P><FONT SIZE=2>OK.</FONT>
</P>
<BR>
<BR>
<BR>
<P><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > Singned-off-by:Yossi Leybovich (sleybo@mellanox.co.il)</FONT>
<BR><FONT SIZE=2>> > Index: core/al/al_init.c </FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- core/al/al_init.c (revision 355)</FONT>
<BR><FONT SIZE=2>> > +++ core/al/al_init.c (working copy)</FONT>
<BR><FONT SIZE=2>> > @@ -39,7 +39,7 @@</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > -uint32_t g_al_dbg_lvl = AL_DBG_ERROR;</FONT>
<BR><FONT SIZE=2>> > +uint32_t g_al_dbg_lvl = </FONT>
<BR><FONT SIZE=2>> AL_DBG_ERROR |</FONT>
<BR><FONT SIZE=2>> > AL_DBG_SMI;</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> The debug level should stay as AL_DBG_ERROR in SVN. Note </FONT>
<BR><FONT SIZE=2>> that you can set the debug level via the registry, as a key </FONT>
<BR><FONT SIZE=2>> under the ibal service parameters - this value will be set </FONT>
<BR><FONT SIZE=2>> when the driver loads. This might make it easier to test </FONT>
<BR><FONT SIZE=2>> without having to recompile when changing debug levels. You </FONT>
<BR><FONT SIZE=2>> can also change the debug level via the kernel debugger.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>Sorry ,my mistake I didn't intend to send it in the patch .</FONT>
</P>
<BR>
<P><FONT SIZE=2>> (p_port_info->subnet_timeout & 0x80)</FONT>
<BR><FONT SIZE=2>> > + {</FONT>
<BR><FONT SIZE=2>> > + </FONT>
<BR><FONT SIZE=2>> AL_TRACE(AL_DBG_PNP,(" setting </FONT>
<BR><FONT SIZE=2>> > + the</FONT>
<BR><FONT SIZE=2>> > client reregister event \n"));</FONT>
<BR><FONT SIZE=2>> > + </FONT>
<BR><FONT SIZE=2>> > + p_spl_qp_svc->obj.p_ci_ca->p_pnp_attr-</FONT>
<BR><FONT SIZE=2>> > >p_port_attr->client_reregistration = 1;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Access to the port attributes needs to be synchronized with </FONT>
<BR><FONT SIZE=2>> the PnP manager through a call to ci_ca_lock_attr and </FONT>
<BR><FONT SIZE=2>> ci_ca_unlock_attr. Can we just set the SM LID to zero in the </FONT>
<BR><FONT SIZE=2>> PnP manager's cached attributes? This would result in </FONT>
<BR><FONT SIZE=2>> existing clients potentially seeing a SM LID of zero, but </FONT>
<BR><FONT SIZE=2>> they would receive a SM_CHANGE event pretty quickly updating </FONT>
<BR><FONT SIZE=2>> them with the new value. This would avoid adding an internal </FONT>
<BR><FONT SIZE=2>> flag to a public structure. Of course, this would only work </FONT>
<BR><FONT SIZE=2>> if we reuse the SM_CHANGE event rather than define a new one.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>As said above I will use the SM change event.</FONT>
</P>
<P><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > Index: hw/mt23108/kernel/hca_verbs.c </FONT>
<BR><FONT SIZE=2>> > ===================================================================</FONT>
<BR><FONT SIZE=2>> > --- hw/mt23108/kernel/hca_verbs.c (revision 355)</FONT>
<BR><FONT SIZE=2>> > +++ hw/mt23108/kernel/hca_verbs.c (working copy)</FONT>
<BR><FONT SIZE=2>> > @@ -414,6 +414,10 @@</FONT>
<BR><FONT SIZE=2>> > hca_attr_mask |= HCA_ATTR_IS_VENDOR_CLS_SUP;</FONT>
<BR><FONT SIZE=2>> > hca_attr.is_vendor_cls_sup = </FONT>
<BR><FONT SIZE=2>> (MT_bool)p_port_attr->cap.vend;</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > + if (modca_cmd & IB_CA_MOD_IS_CLIENT_REREGISTER_SUPPORTED) {</FONT>
<BR><FONT SIZE=2>> > + hca_attr_mask |= </FONT>
<BR><FONT SIZE=2>> HCA_ATTR_IS_CLIENT_REREGISTRATION_SUP;</FONT>
<BR><FONT SIZE=2>> > + hca_attr.is_client_reregister_sup= </FONT>
<BR><FONT SIZE=2>> > + (MT_bool)p_port_attr-</FONT>
<BR><FONT SIZE=2>> > >cap.client_reregister;</FONT>
<BR><FONT SIZE=2>> > + }</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Do we want to let users modify this? It seems that making </FONT>
<BR><FONT SIZE=2>> such a change would have global effects on unsuspecting </FONT>
<BR><FONT SIZE=2>> clients. I would rather not support disabling the client </FONT>
<BR><FONT SIZE=2>> reregister flag as I don't see it providing any value except </FONT>
<BR><FONT SIZE=2>> potentially for DoS type of activities. This falls into a </FONT>
<BR><FONT SIZE=2>> larger port capability management issue that we should tackle </FONT>
<BR><FONT SIZE=2>> at some point.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>This is bigger issue, do we want to let the user change Q_KEY violation SNMP support etc. ?</FONT>
<BR><FONT SIZE=2>I think that like Gen2 we should remove modify CA from user level and allow it only in kernel level</FONT>
<BR><FONT SIZE=2>(In Gen2 Modify CA change only the sys image GUID and in kernel one can change the cap mask by modify_port )</FONT>
<BR><FONT SIZE=2>Still this code is in low level driver and should support the set/reset of this value.</FONT>
</P>
<P><FONT SIZE=2>If we do that we need to supply hack to set the flag of IsSM from user.</FONT>
</P>
<P><FONT SIZE=2> </FONT>
<BR><FONT SIZE=2>> > @@ -7177,6 +7177,7 @@</FONT>
<BR><FONT SIZE=2>> > IB_AE_WQ_ACCESS_ERROR,</FONT>
<BR><FONT SIZE=2>> > IB_AE_PORT_ACTIVE,</FONT>
<BR><FONT SIZE=2>> > IB_AE_PORT_DOWN,</FONT>
<BR><FONT SIZE=2>> > + IB_AE_CLIENT_REREGISTER,</FONT>
<BR><FONT SIZE=2>> > IB_AE_UNKNOWN /* ALWAYS LAST ENUM VALUE */</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> This event should be trapped in al_ci_ca_shared.c, in the </FONT>
<BR><FONT SIZE=2>> function ci_ca_process_event_cb and processed like other port </FONT>
<BR><FONT SIZE=2>> events (~line 227).</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > } ib_async_event_t;</FONT>
<BR><FONT SIZE=2>> > @@ -9279,6 +9288,10 @@</FONT>
<BR><FONT SIZE=2>> > *</FONT>
<BR><FONT SIZE=2>> > * IB_CA_MOD_SYSTEM_IMAGE_GUID</FONT>
<BR><FONT SIZE=2>> > * Used to modify the system image GUID for the port.</FONT>
<BR><FONT SIZE=2>> > +*</FONT>
<BR><FONT SIZE=2>> > +* IB_CA_MOD_IS_CLIENT_REREGISTER_SUPPORTED</FONT>
<BR><FONT SIZE=2>> > +* Used to modify the system image GUID for the port.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Minor nit - the comment is wrong. Goes away if we disallow </FONT>
<BR><FONT SIZE=2>> changing the client reregister support bit.</FONT>
</P>
<P><FONT SIZE=2>As I wrote above , we should leave it the same as the other capabilities.</FONT>
<BR><FONT SIZE=2>Need to change the comment.</FONT>
<BR><FONT SIZE=2>> > }</FONT>
<BR><FONT SIZE=2>> > @@ -855,7 +876,20 @@</FONT>
<BR><FONT SIZE=2>> > case IB_PNP_PORT_REMOVE:</FONT>
<BR><FONT SIZE=2>> > ipoib_resume_oids( p_adapter );</FONT>
<BR><FONT SIZE=2>> > break;</FONT>
<BR><FONT SIZE=2>> > + case IB_PNP_CLIENT_REREGISTER:</FONT>
<BR><FONT SIZE=2>> > </FONT>
<BR><FONT SIZE=2>> > + /* Join all programmed multicast groups. */</FONT>
<BR><FONT SIZE=2>> > + ipoib_refresh_mcast(p_adapter, </FONT>
<BR><FONT SIZE=2>> p_adapter->mcast_array, </FONT>
<BR><FONT SIZE=2>> > +p_adapter->mcast_array_size);</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> This won't re-join the groups because they will already have </FONT>
<BR><FONT SIZE=2>> an endpoint. You either have to flush those endpoints just </FONT>
<BR><FONT SIZE=2>> like you did with the broadcast group, or we need to find a </FONT>
<BR><FONT SIZE=2>> way to re-register the existing groups. Flushing the </FONT>
<BR><FONT SIZE=2>> endpoints will cause packets to be dropped.</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Should we be reporting a cable disconnected event from the </FONT>
<BR><FONT SIZE=2>> time we detect the SM has changed to the time everything has </FONT>
<BR><FONT SIZE=2>> been restored?</FONT>
</P>
<P><FONT SIZE=2>I will use your suggestion to signal the adapterr as hang.</FONT>
</P>
<BR>
</BODY>
</HTML>