[ofw] [IPoIB][patch 0/3] Adding support to dell guid and user-define mask for guid generation

Alex Naslednikov xalex at mellanox.co.il
Mon Sep 22 08:54:11 PDT 2008


This patch was revisited according to your comments - sending it again.
I want to summarize question and comments here:
 
Q1. "Ideally these two would have been independent patches..."
A1. Done
Q2. //The mask is 8 bit and can't contain more than 6 non-zero bits
This comment does not match the definition of guid_mask in the
ipoib_params_t structure (above). Why not more than 6 non-zero bits?
(a question regards why GUID mask should obviously contains exact 6
non-zero bits)
 
A2. Each bit in guid_mask (8) stands for the appropriate byte in GUID (8
bytes). MAC has only 6 bytes length and so is the mask
 
Q3. "Need a space before...", "square brackets..." etc.
(comments regards bad spacing)
A3. Done
 
Q4. Why generation of DELL guid differs from other, say, DELL function ?

A4. Done, totally revisited, see patch 3/3 
 
Q5. > if( p_guid[0] == 0 ) 

> {

> if( p_guid[1] == 0x02 && p_guid[2] == 0xc9 )

.......

>+ else if( p_guid[1] == 0x18 && p_guid[2] == 0x8b )

Why not check the first byte here? The function below will assert that
it's zero.

A5. Please, see again the whole function. The first byte should be zero
for all these types; otherwise, we continue to the default flow.


Q6. IB_INVALID_GUID_MASK - why have we introduce new type ?
A6. See the code again 
AA6. GUID mask can't be fully validated during the parameter getting
stage.
Consider this example: guid_mask == 0xEF. It is less than 0xFC , but it
contains seven non-zero digits In order to check this during parameter
stage, we should pass once again through the calculation implemented in
ipoib_mac_from_general_guid

Q7. > +#define MAC_ADDR_SIZE 6
Unnecessary. Please see ip_packet.h @ line 67 struct _mac_addr is using
HW_ADDR_LEN for it's addr[] size.
A7. In our stack, this variable defined as static const variable only in
SDP module. In order to preserve the encapsulation principle, we
redefined it IPoIB module

Q8. Synchronization between ib_types, ib_types_extended and
opensm/ib_types
A8. I posted a comment on this several month ago. 
Ib_types_extended is included by opensm/ib_types (see #ifdef WIN), and
the rest of opensm/ib_types is out of the scope.
Thus, it sufficient to update only 2 files: ib_types and
ib_types_extended.
However, each change that will be inserted also into opensm/ib_types
will help with linux opensm merging

 




More information about the ofw mailing list