[PATCH 0/2] IB/core: introduce ib_mtu_int_to_enum

[PATCH 0/2] IB/core: introduce ib_mtu_int_to_enum

[Ram Amrani]

As the functionality to convert the MTU from a number to enum_ib_mtu
is ubiquitous, define a dedicated function and remove the duplicated
code (first patch).
Note that the patch doesn't include ocrdma, qib and hfi1 that check
for an exact match of the int mtu and return a different default value
(or error). I leave it to them to adapt to the new function, if at all.

The second patch fixes a qedr bug using the newly introduced function.

Ram Amrani (2):
  IB/core: add the function ib_mtu_int_to_enum
  qedr: fix MTU returned from QP query

 drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 +----------
 drivers/infiniband/hw/cxgb4/provider.c      | 11 +----------
 drivers/infiniband/hw/i40iw/i40iw_verbs.c   | 11 +----------
 drivers/infiniband/hw/nes/nes_verbs.c       | 12 +-----------
 drivers/infiniband/hw/qedr/verbs.c          |  2 +-
 include/rdma/ib_verbs.h                     | 15 +++++++++++++++
 6 files changed, 20 insertions(+), 42 deletions(-)

-- 
1.8.3.1

--
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

[PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Ram Amrani]

As the functionality to convert the MTU from a number to enum_ib_mtu
is ubiquitous, define a dedicated function and remove the duplicated
code.

Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 +----------
 drivers/infiniband/hw/cxgb4/provider.c      | 11 +----------
 drivers/infiniband/hw/i40iw/i40iw_verbs.c   | 11 +----------
 drivers/infiniband/hw/nes/nes_verbs.c       | 12 +-----------
 include/rdma/ib_verbs.h                     | 14 ++++++++++++++
 5 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb3/iwch_provider.c b/drivers/infiniband/hw/cxgb3/iwch_provider.c
index cba57bb..ebb74a2 100644
--- a/drivers/infiniband/hw/cxgb3/iwch_provider.c
+++ b/drivers/infiniband/hw/cxgb3/iwch_provider.c
@@ -1134,16 +1134,7 @@ static int iwch_query_port(struct ib_device *ibdev,
 
 	memset(props, 0, sizeof(struct ib_port_attr));
 	props->max_mtu = IB_MTU_4096;
-	if (netdev->mtu >= 4096)
-		props->active_mtu = IB_MTU_4096;
-	else if (netdev->mtu >= 2048)
-		props->active_mtu = IB_MTU_2048;
-	else if (netdev->mtu >= 1024)
-		props->active_mtu = IB_MTU_1024;
-	else if (netdev->mtu >= 512)
-		props->active_mtu = IB_MTU_512;
-	else
-		props->active_mtu = IB_MTU_256;
+	props->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
 
 	if (!netif_carrier_ok(netdev))
 		props->state = IB_PORT_DOWN;
diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
index 645e606..8456fe3 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -359,16 +359,7 @@ static int c4iw_query_port(struct ib_device *ibdev, u8 port,
 
 	memset(props, 0, sizeof(struct ib_port_attr));
 	props->max_mtu = IB_MTU_4096;
-	if (netdev->mtu >= 4096)
-		props->active_mtu = IB_MTU_4096;
-	else if (netdev->mtu >= 2048)
-		props->active_mtu = IB_MTU_2048;
-	else if (netdev->mtu >= 1024)
-		props->active_mtu = IB_MTU_1024;
-	else if (netdev->mtu >= 512)
-		props->active_mtu = IB_MTU_512;
-	else
-		props->active_mtu = IB_MTU_256;
+	props->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
 
 	if (!netif_carrier_ok(netdev))
 		props->state = IB_PORT_DOWN;
diff --git a/drivers/infiniband/hw/i40iw/i40iw_verbs.c b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
index 6329c97..0f7c5bf 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_verbs.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_verbs.c
@@ -99,16 +99,7 @@ static int i40iw_query_port(struct ib_device *ibdev,
 	memset(props, 0, sizeof(*props));
 
 	props->max_mtu = IB_MTU_4096;
-	if (netdev->mtu >= 4096)
-		props->active_mtu = IB_MTU_4096;
-	else if (netdev->mtu >= 2048)
-		props->active_mtu = IB_MTU_2048;
-	else if (netdev->mtu >= 1024)
-		props->active_mtu = IB_MTU_1024;
-	else if (netdev->mtu >= 512)
-		props->active_mtu = IB_MTU_512;
-	else
-		props->active_mtu = IB_MTU_256;
+	props->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
 
 	props->lid = 1;
 	if (netif_carrier_ok(iwdev->netdev))
diff --git a/drivers/infiniband/hw/nes/nes_verbs.c b/drivers/infiniband/hw/nes/nes_verbs.c
index bd69125..0e23660 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -478,17 +478,7 @@ static int nes_query_port(struct ib_device *ibdev, u8 port, struct ib_port_attr
 	memset(props, 0, sizeof(*props));
 
 	props->max_mtu = IB_MTU_4096;
-
-	if (netdev->mtu  >= 4096)
-		props->active_mtu = IB_MTU_4096;
-	else if (netdev->mtu  >= 2048)
-		props->active_mtu = IB_MTU_2048;
-	else if (netdev->mtu  >= 1024)
-		props->active_mtu = IB_MTU_1024;
-	else if (netdev->mtu  >= 512)
-		props->active_mtu = IB_MTU_512;
-	else
-		props->active_mtu = IB_MTU_256;
+	props->active_mtu = ib_mtu_int_to_enum(netdev->mtu);
 
 	props->lid = 1;
 	props->lmc = 0;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 5ad43a4..e0551a1 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -352,6 +352,20 @@ static inline int ib_mtu_enum_to_int(enum ib_mtu mtu)
 	}
 }
 
+static inline enum ib_mtu ib_mtu_int_to_enum(int mtu)
+{
+	if (mtu >= 4096)
+		return IB_MTU_4096;
+	else if (mtu >= 2048)
+		return IB_MTU_2048;
+	else if (mtu >= 1024)
+		return IB_MTU_1024;
+	else if (mtu >= 512)
+		return IB_MTU_512;
+	else
+		return IB_MTU_256;
+}
+
 enum ib_port_state {
 	IB_PORT_NOP		= 0,
 	IB_PORT_DOWN		= 1,
-- 
1.8.3.1

--
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

[PATCH 2/2] qedr: fix MTU returned from QP query

[Ram Amrani]

MTU value returned from QP query should include overhead.

Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/qedr/verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c
index 0bb6303..3865d25 100644
--- a/drivers/infiniband/hw/qedr/verbs.c
+++ b/drivers/infiniband/hw/qedr/verbs.c
@@ -1930,7 +1930,7 @@ int qedr_query_qp(struct ib_qp *ibqp,
 
 	qp_attr->qp_state = qedr_get_ibqp_state(params.state);
 	qp_attr->cur_qp_state = qedr_get_ibqp_state(params.state);
-	qp_attr->path_mtu = iboe_get_mtu(params.mtu);
+	qp_attr->path_mtu = ib_mtu_int_to_enum(params.mtu);
 	qp_attr->path_mig_state = IB_MIG_MIGRATED;
 	qp_attr->rq_psn = params.rq_psn;
 	qp_attr->sq_psn = params.sq_psn;
-- 
1.8.3.1

--
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

Re: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Leon Romanovsky]

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

On Mon, Dec 26, 2016 at 08:40:57AM +0200, Ram Amrani wrote:
> As the functionality to convert the MTU from a number to enum_ib_mtu
> is ubiquitous, define a dedicated function and remove the duplicated
> code.
>
> Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 +----------
>  drivers/infiniband/hw/cxgb4/provider.c      | 11 +----------
>  drivers/infiniband/hw/i40iw/i40iw_verbs.c   | 11 +----------
>  drivers/infiniband/hw/nes/nes_verbs.c       | 12 +-----------
>  include/rdma/ib_verbs.h                     | 14 ++++++++++++++
>  5 files changed, 18 insertions(+), 41 deletions(-)

Looks good,
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

RE: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Hefty, Sean]

> As the functionality to convert the MTU from a number to enum_ib_mtu
> is ubiquitous, define a dedicated function and remove the duplicated
> code.

Why keep mtu as an enum in the kernel?  Just make it an int like it should be.
--
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

RE: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Amrani, Ram]

> Why keep mtu as an enum in the kernel?  Just make it an int like it should
> be.

You'll still be required to convert it to/from enum when building/parsing CM messages, for example.
Also, why should it be an int if only specific values are allowed?

Ram
--
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

RE: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Hefty, Sean]

> You'll still be required to convert it to/from enum when
> building/parsing CM messages, for example.
> Also, why should it be an int if only specific values are allowed?

The enum values are IB specification specific, and are incomplete (note the lack of Ethernet mtu sizes).  Let the IB centric code deal with the conversion, and let the rest of the API be reasonable.  The IB CM uses the mtu value in the path record, which is fine.  Then maybe someday userspace can get access to the real mtu sizes as well...
--
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

RE: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Amrani, Ram]

> The enum values are IB specification specific, and are incomplete (note the
> lack of Ethernet mtu sizes).  Let the IB centric code deal with the conversion,
> and let the rest of the API be reasonable.  The IB CM uses the mtu value in
> the path record, which is fine.  Then maybe someday userspace can get
> access to the real mtu sizes as well...

I was delaying my response to let others post their opinion, which didn't happen.
This tiny patch just reduced code size, not more.
I suggest you re-open a discussion on a dedicated thread to get a wider view, or
come up with suggestion patch.

Ram

--
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

Re: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Doug Ledford]

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

On Wed, 2017-01-04 at 17:40 +0000, Hefty, Sean wrote:
> > 
> > You'll still be required to convert it to/from enum when
> > building/parsing CM messages, for example.
> > Also, why should it be an int if only specific values are allowed?
> 
> The enum values are IB specification specific, and are incomplete
> (note the lack of Ethernet mtu sizes).  Let the IB centric code deal
> with the conversion, and let the rest of the API be reasonable.  The
> IB CM uses the mtu value in the path record, which is fine.  Then
> maybe someday userspace can get access to the real mtu sizes as
> well...

Hi Sean,

While I don't disagree with your thoughts on the API change, the
original patch as it stands is a reasonable cleanup patch.  I'm not
going to condition one on the other as a result.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] IB/core: introduce ib_mtu_int_to_enum

[Doug Ledford]

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

On Mon, 2016-12-26 at 08:40 +0200, Ram Amrani wrote:
> As the functionality to convert the MTU from a number to enum_ib_mtu
> is ubiquitous, define a dedicated function and remove the duplicated
> code (first patch).
> Note that the patch doesn't include ocrdma, qib and hfi1 that check
> for an exact match of the int mtu and return a different default
> value
> (or error). I leave it to them to adapt to the new function, if at
> all.
> 
> The second patch fixes a qedr bug using the newly introduced
> function.
> 
> Ram Amrani (2):
>   IB/core: add the function ib_mtu_int_to_enum
>   qedr: fix MTU returned from QP query
> 
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 +----------
>  drivers/infiniband/hw/cxgb4/provider.c      | 11 +----------
>  drivers/infiniband/hw/i40iw/i40iw_verbs.c   | 11 +----------
>  drivers/infiniband/hw/nes/nes_verbs.c       | 12 +-----------
>  drivers/infiniband/hw/qedr/verbs.c          |  2 +-
>  include/rdma/ib_verbs.h                     | 15 +++++++++++++++
>  6 files changed, 20 insertions(+), 42 deletions(-)

Thanks, series applied.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 1/2] IB/core: add the function ib_mtu_int_to_enum

[Hefty, Sean]

> While I don't disagree with your thoughts on the API change, the
> original patch as it stands is a reasonable cleanup patch.  I'm not
> going to condition one on the other as a result.

I don't disagree with that.  :)