> -----Original Message----- > From: Hefty, Sean [mailto:sean.hefty@xxxxxxxxx] > Sent: Tuesday, April 11, 2017 7:24 PM > To: Parav Pandit <parav@xxxxxxxxxxxx>; Chandramouli, Dasaratharaman > <dasaratharaman.chandramouli@xxxxxxxxx>; linux-rdma <linux- > rdma@xxxxxxxxxxxxxxx> > Cc: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx> > Subject: RE: [PATCH rdma-next 17/18] IB/core: Define 'ib' and 'eth' > rdma_ah_attr types > > > > > > +struct ib_ah_attr { > > > > > + struct ib_global_route grh; > > > > > + u16 dlid; > > > > > + u8 sl; > > > > > + u8 src_path_bits; > > > > > + u8 static_rate; > > > > > + u8 dmac[ETH_ALEN]; > > > > > > I might have missed something in my review. Why is dmac in the ib > > > attributes? > > > > > Yes. Is should be removed similar to removing dlid from eth_ah_attr. > > Respective if-else code will also go away with that. > > > > > > > +}; > > > > > + > > > > > +struct eth_ah_attr { > > > > > struct ib_global_route grh; > > > > > u16 dlid; > > > > Please remove it from eth, as there is no dlid for Eth. > > > > > > > > > u8 sl; > > > > > u8 src_path_bits; > > > > > u8 static_rate; > > > > > - u8 ah_flags; > > > > > - u8 port_num; > > > > > u8 dmac[ETH_ALEN]; > > > > > }; > > > > > > > > > > > > > Its better to have > > > > struct ib_ah_common_attr {} to have common fields between IB and > > ETH. > > > > And have instance of them in eth_ah_attr and ib_ah_attr structure. > > > > > > I think part of this exercise is to figure out what fields belong to > > each ah type. > > Right. > > > > > I'm not even sure why ethernet has a grh, sl, or src_path_bits. Are > > these > > Sl is applicable. Src_path_bits is IB only. > > > > > used? Are all of the fields of the grh needed? > > Yes. Most of them are common between IB/RoCE. Not sure of iWarp as AH > > attribute is common for iwarp and roce. > > Can the eth attributes be split > > > further based on roce v1 or v2? > > > > Most fields appear common between all 3 IB/RoCEv1,v2 so far to me in > > ib_global_route and are used too. > > I think this is what we have so far then: > > /* common */ > struct rdma_ah_attr { > u8 ah_flags; > u8 port_num; > union ... > }; > > struct ib_ah_attr { > struct ib_global_route grh; > u16 dlid; > u8 sl; > u8 src_path_bits; > u8 static_rate; > }; > > struct eth_ah_attr { > struct ib_global_route grh; > u8 sl; > u8 static_rate; > u8 dmac[ETH_ALEN]; > }; > > struct opa_ah_attr { > struct ib_global_route grh; > u32 dlid; > u8 sl; > u8 src_path_bits; > u8 static_rate; > }; > > So, the grh, sl, and static_rate are candidates to move directly into > rdma_ah_attr. IMO, it's arguable if that's better, but I don't have a strong > opinion either way. I don't know if iwarp uses ah attributes. Grep in cxgb4 driver for ah refers only to c4iw_ah_create which returns -ENOSYS With that I think we should keep grh, sl, static rate in rdma_ah_attr. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
