Re: [PATCH v2 03/11] IB/core: Change ah_attr.dlid from 16 to 32 bits

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Thu, Dec 08, 2016 at 09:12:54PM +0000, Hefty, Sean wrote:
> Example:
> 
> > -		if (rmpp_recv->slid == ah_attr.dlid) {
> > +		if (rmpp_recv->slid == (u16)ah_attr.dlid) {
> 
> >  struct ib_ah_attr {
> >  	struct ib_global_route	grh;
> > -	u16			dlid;
> > +	u32			dlid;
> 
> There are a bunch of places where the lid is simply cast back to a
> u16.  How is the code supposed to determine if a u16 cast is a safe
> operation or not?
> 
> I don?t like the overloading of the term 'lid' to sometimes mean a
> 16-bit value and other times a 32-bit value, with no way of
> distinguishing which one we have.  Maybe we need to call out a
> version (lidv2 = 32-bits) or carry the size.  I don't have a strong
> idea here.  But when looking at this patch, lid is re-defined to
> u32, then nearly everywhere cast back to u16, which looks
> questionable.

Yes, I also don't like this.

I would be much happier to see a proper tagged union introduced for
storing the unpacked AH and have that replace all of these hacky
places. That would help with rocee and iwarp too..

struct rdma_ah_data {
    enum rdma_ah_kind kind;
    union
    {
        struct ib_ah_attr ib;
        struct opa_ah_attr opa;
        struct rocev1_ah_attr rocev1;
        struct rocev2_ah_attr rocev2;
        struct iwarp_ah_attr iwarp;
    };
};

We really need to start tagging our data properly or we are going to
have a hard time maintaining this.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html