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

[Yosef Etigin]

Michael S. Tsirkin wrote:
>>Quoting Yosef Etigin <yosefe at voltaire.com>:
>>Subject: Re: [PATCH 5/6 v2] fix pkey change handling and remove the cahce
>>
>>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)
> 
> 
> Aren't all incoming MADs on a port handled over a single threaded WQ?
> And how would atomics help?
> 
Yes. not atomic any more.

> 
>>>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);
>>
>>is that better?
> 
> 
> Move && to the end of each line, and kill the extra () around single comparisons.
> 

ok.