[ofa-general] Re: [PATCH 5/6 v2] fix pkey change handling and remove the cahce

[Hal Rosenstock]

Hi Yosef,

On Mon, 2007-05-07 at 10:18, Yosef Etigin wrote:
> Michael S. Tsirkin wrote:
> >>@@ -1865,6 +1863,15 @@ static void ib_mad_recv_done_handler(str
> >> 	recv->header.recv_wc.recv_buf.mad = &recv->mad.mad;
> >> 	recv->header.recv_wc.recv_buf.grh = &recv->grh;
> >> 
> >>+	/* update our lmc cache with port info smps */
> >>+	if ((recv->mad.mad.mad_hdr.mgmt_class == IB_MGMT_CLASS_SUBN_LID_ROUTED ||
> >>+	     recv->mad.mad.mad_hdr.mgmt_class == IB_MGMT_CLASS_SUBN_DIRECTED_ROUTE)
> >>+	    && (recv->mad.mad.mad_hdr.attr_id == IB_SMP_ATTR_PORT_INFO)
> >>+		&& (recv->mad.mad.mad_hdr.method == IB_MGMT_METHOD_SET))
> >>+	{
> >>+		atomic_set(&port_priv->port_lmc, recv->mad.smp.data[34] & 0x7);
> >>+	}
> >>+
> >> 	if (atomic_read(&qp_info->snoop_count))
> >> 		snoop_recv(qp_info, &recv->header.recv_wc, IB_MAD_SNOOP_RECVS);
> >> 
> > 
> > 
> > Why is this an atomic?
> 
> I thought there might be a race between this and where we read the lmc (rcv_has_same_gid)
> 
> > The comment does not seem to tell us anything useful. Remove it?
> > These 8 lines seem to violate coding style rules in at least 3 different ways::)
> > 
> 	if ((recv->mad.mad.mad_hdr.mgmt_class == IB_MGMT_CLASS_SUBN_LID_ROUTED ||
> 		 recv->mad.mad.mad_hdr.mgmt_class == IB_MGMT_CLASS_SUBN_DIRECTED_ROUTE)
> 		&& (recv->mad.mad.mad_hdr.attr_id == IB_SMP_ATTR_PORT_INFO)
> 		&& (recv->mad.mad.mad_hdr.method == IB_MGMT_METHOD_SET))
> 		atomic_set(&port_priv->port_lmc, recv->mad.smp.data[34] & 0x7);

Should at least a #define be used for smp.data[34} if not a struct so it
is clearer what is going on here ?

I haven't yet had a chance to look at the rest of the patch.

-- Hal

> is that better?
> _______________________________________________
> general mailing list
> general at lists.openfabrics.org
> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> 
> To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general