On Tue, Aug 08, 2017 at 04:35:47PM +0000, Weiny, Ira wrote:
> >
> > On Sun, Aug 06, 2017 at 11:22:17AM +0300, Leon Romanovsky wrote:
> > > On Sun, Aug 06, 2017 at 11:18:57AM +0300, Leon Romanovsky wrote:
> > > > On Fri, Aug 04, 2017 at 01:53:21PM -0700, Dennis Dalessandro wrote:
> > > > > From: Dasaratharaman Chandramouli
> > > > > <dasaratharaman.chandramouli@xxxxxxxxx>
> > > > >
> > > > > sm_lid field in struct ib_port_attr is increased to 32 bits. This
> > > > > enables core components to use larger LIDs if needed.
> > > > > The user ABI is unchanged and return 16 bit values when queried.
> > > > >
> > > > > Signed-off-by: Dasaratharaman Chandramouli
> > > > > <dasaratharaman.chandramouli@xxxxxxxxx>
> > > > > Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > > > > Signed-off-by: Don Hiatt <don.hiatt@xxxxxxxxx>
> > > > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxx>
> > > > > ---
> > > > > drivers/infiniband/core/uverbs_cmd.c | 8 +++++---
> > > > > include/rdma/ib_verbs.h | 2 +-
> > > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/core/uverbs_cmd.c
> > > > > b/drivers/infiniband/core/uverbs_cmd.c
> > > > > index 7ef74b0..38dce45 100644
> > > > > --- a/drivers/infiniband/core/uverbs_cmd.c
> > > > > +++ b/drivers/infiniband/core/uverbs_cmd.c
> > > > > @@ -275,11 +275,13 @@ ssize_t ib_uverbs_query_port(struct
> > ib_uverbs_file *file,
> > > > > resp.bad_pkey_cntr = attr.bad_pkey_cntr;
> > > > > resp.qkey_viol_cntr = attr.qkey_viol_cntr;
> > > > > resp.pkey_tbl_len = attr.pkey_tbl_len;
> > > > > - resp.sm_lid = attr.sm_lid;
> > > > > - if (rdma_cap_opa_ah(ib_dev, cmd.port_num))
> > > > > + if (rdma_cap_opa_ah(ib_dev, cmd.port_num)) {
> > > > > resp.lid = OPA_TO_IB_UCAST_LID(attr.lid);
> > > > > - else
> > > > > + resp.sm_lid = OPA_TO_IB_UCAST_LID(attr.sm_lid);
> > > > > + } else {
> > > > > resp.lid = (u16)attr.lid;
> > > > > + resp.sm_lid = (u16)attr.sm_lid;
> > > >
> > > > I see that lid is already has casting from u32 to u16 and now it is sm_lid.
> > > > Do we have more elegant way to achieve that? And comment for future
> > > > developers can be good too.
> > >
> > > I see now, you changed lid in patch 11. The same comment there.
> >
> > To be clear on that point: NAK on *current* implementation.
> >
> > At least, it should be done in separate function, with comment explains why
> > are you doing and why it is safe to cast from higher value to lower value and
> > proper checks that you are not loosing information when you are casting.
> >
>
> A comment is reasonable. However, I'm not sure a separate function is needed. With the current interface there is no way to pass all the information through.
>
> Software which requires the use of these values will need to know to get them from other sources (OPA MAD queries for the SM LID for example.)
The function, check and warning are intended to catch possible buggy
flows and user errors. It is a bit optimistic way of thinking that all
software uses SMs.
>
> Ira
>
Attachment:
signature.asc
Description: PGP signature
