| Message ID | 1482734458-32131-2-git-send-email-Ram.Amrani@cavium.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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@cavium.com> > --- > 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@mellanox.com>
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> 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. :)
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,
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@cavium.com> --- 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(-)