Re: [PATCH for-next 12/27] IB/core: Change port_attr.sm_lid from 16 to 32 bits

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3725 bytes --]

On Tue, Aug 08, 2017 at 09:49:17AM -0700, Don Hiatt wrote:
>
>
> On 8/8/2017 9:35 AM, 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > >
> > > > > > 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > Reviewed-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > Signed-off-by: Don Hiatt <don.hiatt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > Signed-off-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > > > ---
> > > > > >   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.)
> >
> > Ira
> >
> Hi Leon,
>
> I had already created a function to do the cast in the 'IB/core: Change
> wc.slid from 16 to 32 bits' patch
> but I did not apply it here. Sorry about that.  My plan is to rename the
> functions to ib_lid_{spu16,be16}
> and introduce them in this patch with a comment in the git log that these
> functions exist. I will also add
> a comment stating why we aren't losing any information.

These functions should be introduced before "casting patches".

Thanks

>
> Thank you.
>
> don
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]