Re: [PATCH for-next V3 01/11] IB/core: Add gid_type to gid attribute

[Leon Romanovsky]

On Wed, Dec 23, 2015 at 02:56:47PM +0200, Matan Barak wrote:
> In order to support multiple GID types, we need to store the gid_type
> with each GID. This is also aligned with the RoCE v2 annex "RoCEv2 PORT
> GID table entries shall have a "GID type" attribute that denotes the L3
> Address type". The currently supported GID is IB_GID_TYPE_IB which is
> also RoCE v1 GID type.
> 
> This implies that gid_type should be added to roce_gid_table meta-data.
> 
> Signed-off-by: Matan Barak 
> ---
>  drivers/infiniband/core/cache.c   |  144 +++-
>  drivers/infiniband/core/cm.c  |2 +-
>  drivers/infiniband/core/cma.c |3 +-
>  drivers/infiniband/core/core_priv.h   |4 +
>  drivers/infiniband/core/device.c  |9 ++-
>  drivers/infiniband/core/multicast.c   |2 +-
>  drivers/infiniband/core/roce_gid_mgmt.c   |   60 ++--
>  drivers/infiniband/core/sa_query.c|5 +-
>  drivers/infiniband/core/uverbs_marshall.c |1 +
>  drivers/infiniband/core/verbs.c   |1 +
>  include/rdma/ib_cache.h   |4 +
>  include/rdma/ib_sa.h  |1 +
>  include/rdma/ib_verbs.h   |   11 ++-
>  13 files changed, 185 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 097e9df..566fd8f 100644
> --- a/drivers/infiniband/core/cache.c
> +++ b/drivers/infiniband/core/cache.c
> @@ -64,6 +64,7 @@ enum gid_attr_find_mask {
>   GID_ATTR_FIND_MASK_GID  = 1UL << 0,
>   GID_ATTR_FIND_MASK_NETDEV   = 1UL << 1,
>   GID_ATTR_FIND_MASK_DEFAULT  = 1UL << 2,
> + GID_ATTR_FIND_MASK_GID_TYPE = 1UL << 3,
>  };
>  
>  enum gid_table_entry_props {
> @@ -125,6 +126,19 @@ static void dispatch_gid_change_event(struct ib_device 
> *ib_dev, u8 port)
>   }
>  }
>  
> +static const char * const gid_type_str[] = {
   ^^ ^^
IMHO, The white spaces can be a little bit confusing to understand.

> + [IB_GID_TYPE_IB]= "IB/RoCE v1",
> +};
> +
> +const char *ib_cache_gid_type_str(enum ib_gid_type gid_type)
> +{
> + if (gid_type < ARRAY_SIZE(gid_type_str) && gid_type_str[gid_type])
Why do you need to check second condition?
> + return gid_type_str[gid_type];
> +
> + return "Invalid GID type";
> +}
> +EXPORT_SYMBOL(ib_cache_gid_type_str);
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next V3 01/11] IB/core: Add gid_type to gid attribute

[Sagi Grimberg]

+static const char * const gid_type_str[] = {

^^ ^^
IMHO, The white spaces can be a little bit confusing to understand.



Pay attention to the double const - I think it's more clear that way.


I agree.




+ [IB_GID_TYPE_IB]= "IB/RoCE v1",
+};
+
+const char *ib_cache_gid_type_str(enum ib_gid_type gid_type)
+{
+ if (gid_type < ARRAY_SIZE(gid_type_str) && gid_type_str[gid_type])

Why do you need to check second condition?


If we want to leave a gap for an invalid GID type, we could do that.
Anyway, we could remove this as an incremental future patch if that's
really important.


Leave it in, better safe than sorry...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next V3 01/11] IB/core: Add gid_type to gid attribute

[Matan Barak]

On Wed, Dec 23, 2015 at 3:40 PM, Leon Romanovsky  wrote:
> On Wed, Dec 23, 2015 at 02:56:47PM +0200, Matan Barak wrote:
>> In order to support multiple GID types, we need to store the gid_type
>> with each GID. This is also aligned with the RoCE v2 annex "RoCEv2 PORT
>> GID table entries shall have a "GID type" attribute that denotes the L3
>> Address type". The currently supported GID is IB_GID_TYPE_IB which is
>> also RoCE v1 GID type.
>>
>> This implies that gid_type should be added to roce_gid_table meta-data.
>>
>> Signed-off-by: Matan Barak 
>> ---
>>  drivers/infiniband/core/cache.c   |  144 
>> +++-
>>  drivers/infiniband/core/cm.c  |2 +-
>>  drivers/infiniband/core/cma.c |3 +-
>>  drivers/infiniband/core/core_priv.h   |4 +
>>  drivers/infiniband/core/device.c  |9 ++-
>>  drivers/infiniband/core/multicast.c   |2 +-
>>  drivers/infiniband/core/roce_gid_mgmt.c   |   60 ++--
>>  drivers/infiniband/core/sa_query.c|5 +-
>>  drivers/infiniband/core/uverbs_marshall.c |1 +
>>  drivers/infiniband/core/verbs.c   |1 +
>>  include/rdma/ib_cache.h   |4 +
>>  include/rdma/ib_sa.h  |1 +
>>  include/rdma/ib_verbs.h   |   11 ++-
>>  13 files changed, 185 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cache.c 
>> b/drivers/infiniband/core/cache.c
>> index 097e9df..566fd8f 100644
>> --- a/drivers/infiniband/core/cache.c
>> +++ b/drivers/infiniband/core/cache.c
>> @@ -64,6 +64,7 @@ enum gid_attr_find_mask {
>>   GID_ATTR_FIND_MASK_GID  = 1UL << 0,
>>   GID_ATTR_FIND_MASK_NETDEV   = 1UL << 1,
>>   GID_ATTR_FIND_MASK_DEFAULT  = 1UL << 2,
>> + GID_ATTR_FIND_MASK_GID_TYPE = 1UL << 3,
>>  };
>>
>>  enum gid_table_entry_props {
>> @@ -125,6 +126,19 @@ static void dispatch_gid_change_event(struct ib_device 
>> *ib_dev, u8 port)
>>   }
>>  }
>>
>> +static const char * const gid_type_str[] = {
>^^ ^^
> IMHO, The white spaces can be a little bit confusing to understand.
>

Pay attention to the double const - I think it's more clear that way.

>> + [IB_GID_TYPE_IB]= "IB/RoCE v1",
>> +};
>> +
>> +const char *ib_cache_gid_type_str(enum ib_gid_type gid_type)
>> +{
>> + if (gid_type < ARRAY_SIZE(gid_type_str) && gid_type_str[gid_type])
> Why do you need to check second condition?

If we want to leave a gap for an invalid GID type, we could do that.
Anyway, we could remove this as an incremental future patch if that's
really important.

>> + return gid_type_str[gid_type];
>> +
>> + return "Invalid GID type";
>> +}
>> +EXPORT_SYMBOL(ib_cache_gid_type_str);
>> +
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html