On Mon, 2007-05-07 at 11:12, Yosef Etigin wrote:
> Hal Rosenstock wrote:
> > 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 ?
> >
>
> you mean something like:
> #define LMC_FROM_PORT_INFO(data) ( ( (char*)(data) )[34] & 0x07 ) ?
Yes, something along those lines at a minimum.
-- Hal
> > I haven't yet had a chance to look at the rest of the patch.
> >
> > -- Hal
> >
> >
> >>is that better?
> >>_______________________________________________
> >>general mailing list
> >>[email protected]
> >>http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
> >>
> >>To unsubscribe, please visit
> >>http://openib.org/mailman/listinfo/openib-general
> >
> >
>
_______________________________________________
general mailing list
[email protected]
http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
