| Message ID | 1690966823-8159-1-git-send-email-selvin.xavier@broadcom.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [for-next,v2] IB/core: Add more speed parsing in ib_get_width_and_speed() | expand |
On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > When the Ethernet driver does not provide the number of lanes > in the __ethtool_get_link_ksettings() response, the function > ib_get_width_and_speed() does not take consideration of 50G, > 100G and 200G speeds while calculating the IB width and speed. > Update the width and speed for the above netdev speeds. > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > --- > v1 - v2: > - Rebased the patch based on the latest changes in ib_get_width_and_speed > - removed the switch case and use the existing else if check > --- > drivers/infiniband/core/verbs.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > index 25367bd..41ff559 100644 > --- a/drivers/infiniband/core/verbs.c > +++ b/drivers/infiniband/core/verbs.c > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > } else if (netdev_speed <= SPEED_40000) { > *width = IB_WIDTH_4X; > *speed = IB_SPEED_FDR10; > - } else { > + } else if (netdev_speed <= SPEED_50000) { > + *width = IB_WIDTH_2X; > + *speed = IB_SPEED_EDR; > + } else if (netdev_speed <= SPEED_100000) { > *width = IB_WIDTH_4X; > *speed = IB_SPEED_EDR; > + } else if (netdev_speed <= SPEED_200000) { > + *width = IB_WIDTH_4X; > + *speed = IB_SPEED_HDR; SPEED_50000, SPEED_100000 and SPEED_200000 depends on ClassPortInfo:CapabilityMask2.is* values. For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. Thanks > + } else { > + *width = IB_WIDTH_4X; > + *speed = IB_SPEED_NDR; > } > > return; > -- > 2.5.5 >
On Thu, Aug 3, 2023 at 11:28 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > When the Ethernet driver does not provide the number of lanes > > in the __ethtool_get_link_ksettings() response, the function > > ib_get_width_and_speed() does not take consideration of 50G, > > 100G and 200G speeds while calculating the IB width and speed. > > Update the width and speed for the above netdev speeds. > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > --- > > v1 - v2: > > - Rebased the patch based on the latest changes in ib_get_width_and_speed > > - removed the switch case and use the existing else if check > > --- > > drivers/infiniband/core/verbs.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > index 25367bd..41ff559 100644 > > --- a/drivers/infiniband/core/verbs.c > > +++ b/drivers/infiniband/core/verbs.c > > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > > } else if (netdev_speed <= SPEED_40000) { > > *width = IB_WIDTH_4X; > > *speed = IB_SPEED_FDR10; > > - } else { > > + } else if (netdev_speed <= SPEED_50000) { > > + *width = IB_WIDTH_2X; > > + *speed = IB_SPEED_EDR; > > + } else if (netdev_speed <= SPEED_100000) { > > *width = IB_WIDTH_4X; > > *speed = IB_SPEED_EDR; > > + } else if (netdev_speed <= SPEED_200000) { > > + *width = IB_WIDTH_4X; > > + *speed = IB_SPEED_HDR; > > > SPEED_50000, SPEED_100000 and SPEED_200000 depends on > ClassPortInfo:CapabilityMask2.is* values. > > For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. Agree with that. This reporting can be achieved by the existing code, but the L2 driver needs to report non zero values for lanes in ethtool_ops->get_link_ksettings. Caller of this modified function gets the speed and number of lanes from ethtool_ops->get_link_ksettings. In this patch we are trying to handle the case where ethtool ops doesn't provide the lanes. We can default to some value so that we dont report wrong total link speed (width * speed) for any speed more than 40G. > > Thanks > > > + } else { > > + *width = IB_WIDTH_4X; > > + *speed = IB_SPEED_NDR; > > } > > > > return; > > -- > > 2.5.5 > > > >
On Fri, Aug 04, 2023 at 09:43:28AM +0530, Selvin Xavier wrote: > On Thu, Aug 3, 2023 at 11:28 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > > When the Ethernet driver does not provide the number of lanes > > > in the __ethtool_get_link_ksettings() response, the function > > > ib_get_width_and_speed() does not take consideration of 50G, > > > 100G and 200G speeds while calculating the IB width and speed. > > > Update the width and speed for the above netdev speeds. > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > --- > > > v1 - v2: > > > - Rebased the patch based on the latest changes in ib_get_width_and_speed > > > - removed the switch case and use the existing else if check > > > --- > > > drivers/infiniband/core/verbs.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > > index 25367bd..41ff559 100644 > > > --- a/drivers/infiniband/core/verbs.c > > > +++ b/drivers/infiniband/core/verbs.c > > > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > > > } else if (netdev_speed <= SPEED_40000) { > > > *width = IB_WIDTH_4X; > > > *speed = IB_SPEED_FDR10; > > > - } else { > > > + } else if (netdev_speed <= SPEED_50000) { > > > + *width = IB_WIDTH_2X; > > > + *speed = IB_SPEED_EDR; > > > + } else if (netdev_speed <= SPEED_100000) { > > > *width = IB_WIDTH_4X; > > > *speed = IB_SPEED_EDR; > > > + } else if (netdev_speed <= SPEED_200000) { > > > + *width = IB_WIDTH_4X; > > > + *speed = IB_SPEED_HDR; > > > > > > SPEED_50000, SPEED_100000 and SPEED_200000 depends on > > ClassPortInfo:CapabilityMask2.is* values. > > > > For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. > Agree with that. > This reporting can be achieved by the existing code, but the L2 driver > needs to report non zero values for lanes in > ethtool_ops->get_link_ksettings. > Caller of this modified function gets the speed and number of lanes > from ethtool_ops->get_link_ksettings. > > In this patch we are trying to handle the case where ethtool ops > doesn't provide the lanes. Almost all drivers don't support lanes reporting. > We can default to some value so that we > dont report wrong total link speed (width * speed) for any speed more > than 40G. > > > > Thanks > > > > > + } else { > > > + *width = IB_WIDTH_4X; > > > + *speed = IB_SPEED_NDR; > > > } > > > > > > return; > > > -- > > > 2.5.5 > > > > > > >
On Mon, Aug 7, 2023 at 7:22 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Fri, Aug 04, 2023 at 09:43:28AM +0530, Selvin Xavier wrote: > > On Thu, Aug 3, 2023 at 11:28 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > > > > When the Ethernet driver does not provide the number of lanes > > > > in the __ethtool_get_link_ksettings() response, the function > > > > ib_get_width_and_speed() does not take consideration of 50G, > > > > 100G and 200G speeds while calculating the IB width and speed. > > > > Update the width and speed for the above netdev speeds. > > > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > > --- > > > > v1 - v2: > > > > - Rebased the patch based on the latest changes in ib_get_width_and_speed > > > > - removed the switch case and use the existing else if check > > > > --- > > > > drivers/infiniband/core/verbs.c | 11 ++++++++++- > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > > > index 25367bd..41ff559 100644 > > > > --- a/drivers/infiniband/core/verbs.c > > > > +++ b/drivers/infiniband/core/verbs.c > > > > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > > > > } else if (netdev_speed <= SPEED_40000) { > > > > *width = IB_WIDTH_4X; > > > > *speed = IB_SPEED_FDR10; > > > > - } else { > > > > + } else if (netdev_speed <= SPEED_50000) { > > > > + *width = IB_WIDTH_2X; > > > > + *speed = IB_SPEED_EDR; > > > > + } else if (netdev_speed <= SPEED_100000) { > > > > *width = IB_WIDTH_4X; > > > > *speed = IB_SPEED_EDR; > > > > + } else if (netdev_speed <= SPEED_200000) { > > > > + *width = IB_WIDTH_4X; > > > > + *speed = IB_SPEED_HDR; > > > > > > > > > SPEED_50000, SPEED_100000 and SPEED_200000 depends on > > > ClassPortInfo:CapabilityMask2.is* values. > > > > > > For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. > > Agree with that. > > This reporting can be achieved by the existing code, but the L2 driver > > needs to report non zero values for lanes in > > ethtool_ops->get_link_ksettings. > > Caller of this modified function gets the speed and number of lanes > > from ethtool_ops->get_link_ksettings. > > > > In this patch we are trying to handle the case where ethtool ops > > doesn't provide the lanes. > > Almost all drivers don't support lanes reporting. Agreed. But this patch will at least correct the overall speed reporting. So it's an improvement from the current behavior. If you still feel that this is not needed, you can drop the patch. We will have to modify the L2 drivers to report the lanes in that case. > > > We can default to some value so that we > > dont report wrong total link speed (width * speed) for any speed more > > than 40G. > > > > > > Thanks > > > > > > > + } else { > > > > + *width = IB_WIDTH_4X; > > > > + *speed = IB_SPEED_NDR; > > > > } > > > > > > > > return; > > > > -- > > > > 2.5.5 > > > > > > > > > > > >
On Wed, Aug 09, 2023 at 02:24:22PM +0530, Selvin Xavier wrote: > On Mon, Aug 7, 2023 at 7:22 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > On Fri, Aug 04, 2023 at 09:43:28AM +0530, Selvin Xavier wrote: > > > On Thu, Aug 3, 2023 at 11:28 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > > > > > > When the Ethernet driver does not provide the number of lanes > > > > > in the __ethtool_get_link_ksettings() response, the function > > > > > ib_get_width_and_speed() does not take consideration of 50G, > > > > > 100G and 200G speeds while calculating the IB width and speed. > > > > > Update the width and speed for the above netdev speeds. > > > > > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > > > --- > > > > > v1 - v2: > > > > > - Rebased the patch based on the latest changes in ib_get_width_and_speed > > > > > - removed the switch case and use the existing else if check > > > > > --- > > > > > drivers/infiniband/core/verbs.c | 11 ++++++++++- > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > > > > index 25367bd..41ff559 100644 > > > > > --- a/drivers/infiniband/core/verbs.c > > > > > +++ b/drivers/infiniband/core/verbs.c > > > > > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > > > > > } else if (netdev_speed <= SPEED_40000) { > > > > > *width = IB_WIDTH_4X; > > > > > *speed = IB_SPEED_FDR10; > > > > > - } else { > > > > > + } else if (netdev_speed <= SPEED_50000) { > > > > > + *width = IB_WIDTH_2X; > > > > > + *speed = IB_SPEED_EDR; > > > > > + } else if (netdev_speed <= SPEED_100000) { > > > > > *width = IB_WIDTH_4X; > > > > > *speed = IB_SPEED_EDR; > > > > > + } else if (netdev_speed <= SPEED_200000) { > > > > > + *width = IB_WIDTH_4X; > > > > > + *speed = IB_SPEED_HDR; > > > > > > > > > > > > SPEED_50000, SPEED_100000 and SPEED_200000 depends on > > > > ClassPortInfo:CapabilityMask2.is* values. > > > > > > > > For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. > > > Agree with that. > > > This reporting can be achieved by the existing code, but the L2 driver > > > needs to report non zero values for lanes in > > > ethtool_ops->get_link_ksettings. > > > Caller of this modified function gets the speed and number of lanes > > > from ethtool_ops->get_link_ksettings. > > > > > > In this patch we are trying to handle the case where ethtool ops > > > doesn't provide the lanes. > > > > Almost all drivers don't support lanes reporting. > Agreed. But this patch will at least correct the overall speed > reporting. So it's an improvement from the current behavior. > If you still feel that this is not needed, you can drop the patch. I'm ok with the implementation, just waiting to hear second opinion from Jason. Thanks
On Wed, Aug 09, 2023 at 12:06:01PM +0300, Leon Romanovsky wrote: > On Wed, Aug 09, 2023 at 02:24:22PM +0530, Selvin Xavier wrote: > > On Mon, Aug 7, 2023 at 7:22 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > On Fri, Aug 04, 2023 at 09:43:28AM +0530, Selvin Xavier wrote: > > > > On Thu, Aug 3, 2023 at 11:28 PM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > > > > > On Wed, Aug 02, 2023 at 02:00:23AM -0700, Selvin Xavier wrote: > > > > > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > > > > > > > > When the Ethernet driver does not provide the number of lanes > > > > > > in the __ethtool_get_link_ksettings() response, the function > > > > > > ib_get_width_and_speed() does not take consideration of 50G, > > > > > > 100G and 200G speeds while calculating the IB width and speed. > > > > > > Update the width and speed for the above netdev speeds. > > > > > > > > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com> > > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com> > > > > > > --- > > > > > > v1 - v2: > > > > > > - Rebased the patch based on the latest changes in ib_get_width_and_speed > > > > > > - removed the switch case and use the existing else if check > > > > > > --- > > > > > > drivers/infiniband/core/verbs.c | 11 ++++++++++- > > > > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c > > > > > > index 25367bd..41ff559 100644 > > > > > > --- a/drivers/infiniband/core/verbs.c > > > > > > +++ b/drivers/infiniband/core/verbs.c > > > > > > @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, > > > > > > } else if (netdev_speed <= SPEED_40000) { > > > > > > *width = IB_WIDTH_4X; > > > > > > *speed = IB_SPEED_FDR10; > > > > > > - } else { > > > > > > + } else if (netdev_speed <= SPEED_50000) { > > > > > > + *width = IB_WIDTH_2X; > > > > > > + *speed = IB_SPEED_EDR; > > > > > > + } else if (netdev_speed <= SPEED_100000) { > > > > > > *width = IB_WIDTH_4X; > > > > > > *speed = IB_SPEED_EDR; > > > > > > + } else if (netdev_speed <= SPEED_200000) { > > > > > > + *width = IB_WIDTH_4X; > > > > > > + *speed = IB_SPEED_HDR; > > > > > > > > > > > > > > > SPEED_50000, SPEED_100000 and SPEED_200000 depends on > > > > > ClassPortInfo:CapabilityMask2.is* values. > > > > > > > > > > For example, SPEED_50000 can b IB_WIDTH_2X/IB_SPEED_EDR and IB_WIDTH_1X/IB_SPEED_HDR. > > > > Agree with that. > > > > This reporting can be achieved by the existing code, but the L2 driver > > > > needs to report non zero values for lanes in > > > > ethtool_ops->get_link_ksettings. > > > > Caller of this modified function gets the speed and number of lanes > > > > from ethtool_ops->get_link_ksettings. > > > > > > > > In this patch we are trying to handle the case where ethtool ops > > > > doesn't provide the lanes. > > > > > > Almost all drivers don't support lanes reporting. > > Agreed. But this patch will at least correct the overall speed > > reporting. So it's an improvement from the current behavior. > > If you still feel that this is not needed, you can drop the patch. > > I'm ok with the implementation, just waiting to hear second opinion from Jason. It seems fine, I think 4x is a reasonable place to default to. Jason
On Wed, 02 Aug 2023 02:00:23 -0700, Selvin Xavier wrote: > When the Ethernet driver does not provide the number of lanes > in the __ethtool_get_link_ksettings() response, the function > ib_get_width_and_speed() does not take consideration of 50G, > 100G and 200G speeds while calculating the IB width and speed. > Update the width and speed for the above netdev speeds. > > > [...] Applied, thanks! [1/1] IB/core: Add more speed parsing in ib_get_width_and_speed() https://git.kernel.org/rdma/rdma/c/ca60fd116c7ee1 Best regards,
diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c index 25367bd..41ff559 100644 --- a/drivers/infiniband/core/verbs.c +++ b/drivers/infiniband/core/verbs.c @@ -1899,9 +1899,18 @@ static void ib_get_width_and_speed(u32 netdev_speed, u32 lanes, } else if (netdev_speed <= SPEED_40000) { *width = IB_WIDTH_4X; *speed = IB_SPEED_FDR10; - } else { + } else if (netdev_speed <= SPEED_50000) { + *width = IB_WIDTH_2X; + *speed = IB_SPEED_EDR; + } else if (netdev_speed <= SPEED_100000) { *width = IB_WIDTH_4X; *speed = IB_SPEED_EDR; + } else if (netdev_speed <= SPEED_200000) { + *width = IB_WIDTH_4X; + *speed = IB_SPEED_HDR; + } else { + *width = IB_WIDTH_4X; + *speed = IB_SPEED_NDR; } return;