> -----Original Message----- > From: Hefty, Sean [mailto:sean.hefty@xxxxxxxxx] > Sent: Wednesday, April 12, 2017 9:05 AM > 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 > > > > I can change the type check into a function if you feel its open > > coded. > > > One of the benefits of adding type to ah_attr is exactly the above > > code. > > > The caller doesnt have to call into the core (rdma_cap_eth_ah) to > > know what > > > 'protocol' its working with. That information is contained in the > > ah_attr itself. > > > > > I was referring to have helper function like rdma_cap_eth_ah, but for > > ah_attr_type as, is_ah_attr_eth(ah_attr). > > > > I think it will be more useful, as there is more code in grh > > processing coming up, that needs to diverge based on ib vs roce. > > And there are few places we have that comparison anyway below. It just > > keep code neat. > > FWIW, I don't see a benefit to converting a simple type check of an enum > into a function. The type allows its use in a switch statement, rather than > needing to use if - else-if - else-if. > Ok. Make sense when we have more ah types than just two, it will be useful. > > While we are at it, I was thinking since iWarp doesn't use AH, we > > better name ah->type as ROCE, to avoid this confusion happening again? > > I agree, though I'm still not sure v1 and v2 need all the same fields or use > them in the same way. Conceptually, a GRH in v2 doesn't make sense to me. > (I should go read the spec.) But in any case v1/v2 separation could come as > follow on changes. > Yes. That can be follow on patch. At minimum naming _roce is fine. > - Sean -- 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
