| Message ID | 20171027123011.10454-1-tbogendoerfer@suse.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
> -----Original Message----- > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > owner@vger.kernel.org] On Behalf Of Thomas Bogendoerfer > Sent: Friday, October 27, 2017 7:30 AM > To: Matan Barak <matanb@mellanox.com>; Leon Romanovsky > <leonro@mellanox.com>; Doug Ledford <dledford@redhat.com>; linux- > rdma@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH] IB/mlx5: give back valid speed/width even without plugged in > SFP module > > If there is no SFP module plugged into a port of mlx5 cards 'cat > /sys/class/infniband/mlx5_X/ports/1/rate' returns Invalid argument. > This causes tools like 'ibstat' to malfunction. This change adjusts mlx5 to all > other RoCE/iWarp drivers, which always return valid speed/width. > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > drivers/infiniband/hw/mlx5/main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx5/main.c > b/drivers/infiniband/hw/mlx5/main.c > index 260f8be1d0ed..4388618e3434 100644 > --- a/drivers/infiniband/hw/mlx5/main.c > +++ b/drivers/infiniband/hw/mlx5/main.c > @@ -246,7 +246,10 @@ static int translate_eth_proto_oper(u32 > eth_proto_oper, u8 *active_speed, > *active_speed = IB_SPEED_EDR; > break; > default: > - return -EINVAL; > + /* Unknown */ > + *active_width = IB_WIDTH_1X; > + *active_speed = IB_SPEED_SDR; > + break; > } > > return 0; > -- > 2.12.3 Similar issue was reported by Ghazale in offline email and she also provided similar patch. I added her in this mail thread. Please add below reported-by tag if you find it appropriate. Reported-by: Ghazale Hosseinabadi <ghazale.hosseinabadi@oracle.com> Thanks for the short term fix. However I believe that ibstat tool should be enhanced to report unknown port speed instead of expecting drivers to supply some random number like this. Similar tools such as ethtool does report unknown port speed as unknown like below output which doesn't have SFP. ethtool ens2f0 <....> Speed: Unknown! Duplex: Unknown! (255) <....> Link detected: no -- 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 Fri, Oct 27, 2017 at 02:30:11PM +0200, Thomas Bogendoerfer wrote: > If there is no SFP module plugged into a port of mlx5 cards > 'cat /sys/class/infniband/mlx5_X/ports/1/rate' returns Invalid argument. > This causes tools like 'ibstat' to malfunction. This change adjusts mlx5 > to all other RoCE/iWarp drivers, which always return valid speed/width. Like Parav, I have mixed feelings about such change. It returns EINVAL if nothing is connected and it is right thing to do. It is hard to call "valid speed/width" for unconnected port. I would like to have ibstat and other drivers fixed instead of converting mlx5 to be wrong. Proposed change breaks existing scripts. Thanks > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > drivers/infiniband/hw/mlx5/main.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c > index 260f8be1d0ed..4388618e3434 100644 > --- a/drivers/infiniband/hw/mlx5/main.c > +++ b/drivers/infiniband/hw/mlx5/main.c > @@ -246,7 +246,10 @@ static int translate_eth_proto_oper(u32 eth_proto_oper, u8 *active_speed, > *active_speed = IB_SPEED_EDR; > break; > default: > - return -EINVAL; > + /* Unknown */ > + *active_width = IB_WIDTH_1X; > + *active_speed = IB_SPEED_SDR; > + break; > } > > return 0; > -- > 2.12.3 > > -- > 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 10/27/2017 2:32 PM, Parav Pandit wrote:
> However I believe that ibstat tool should be enhanced to report unknown port speed instead of expecting drivers to supply some random number like this.
ibstat gets the rate from libibumad via /sys/class/infiniband/<IB device>/ports/<port number>/rate file which is supposed to be populated by the driver. Is there no rate file in this error case ?
-- Hal
--
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
> -----Original Message----- > From: Hal Rosenstock [mailto:hal@dev.mellanox.co.il] > Sent: Friday, October 27, 2017 3:19 PM > To: Parav Pandit <parav@mellanox.com>; Thomas Bogendoerfer > <tbogendoerfer@suse.de>; Matan Barak <matanb@mellanox.com>; Leon > Romanovsky <leonro@mellanox.com>; Doug Ledford <dledford@redhat.com>; > linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: Ghazale Hosseinabadi <ghazale.hosseinabadi@oracle.com> > Subject: Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged > in SFP module > > On 10/27/2017 2:32 PM, Parav Pandit wrote: > > However I believe that ibstat tool should be enhanced to report unknown port > speed instead of expecting drivers to supply some random number like this. > > ibstat gets the rate from libibumad via /sys/class/infiniband/<IB > device>/ports/<port number>/rate file which is supposed to be populated by the > driver. Is there no rate file in this error case ? > <...>/<port_num>/rate file exist. rate_show() has invalid active_width as expected due to nonexistence of SFP. So sysfs call return invalid value. We don't have invalid_active_width defined right now. So ibstat and other applications should not crash on such valid errors.
On 10/27/2017 4:33 PM, Parav Pandit wrote: > > >> -----Original Message----- >> From: Hal Rosenstock [mailto:hal@dev.mellanox.co.il] >> Sent: Friday, October 27, 2017 3:19 PM >> To: Parav Pandit <parav@mellanox.com>; Thomas Bogendoerfer >> <tbogendoerfer@suse.de>; Matan Barak <matanb@mellanox.com>; Leon >> Romanovsky <leonro@mellanox.com>; Doug Ledford <dledford@redhat.com>; >> linux-rdma@vger.kernel.org; linux-kernel@vger.kernel.org >> Cc: Ghazale Hosseinabadi <ghazale.hosseinabadi@oracle.com> >> Subject: Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged >> in SFP module >> >> On 10/27/2017 2:32 PM, Parav Pandit wrote: >>> However I believe that ibstat tool should be enhanced to report unknown port >> speed instead of expecting drivers to supply some random number like this. >> >> ibstat gets the rate from libibumad via /sys/class/infiniband/<IB >> device>/ports/<port number>/rate file which is supposed to be populated by the >> driver. Is there no rate file in this error case ? >> > <...>/<port_num>/rate file exist. > > rate_show() has invalid active_width as expected due to nonexistence of SFP. > So sysfs call return invalid value. > We don't have invalid_active_width defined right now. > So ibstat and other applications should not crash on such valid errors. Agreed. I haven't seen ibstat crash reported though. Can someone provide the crash details ? -- 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
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 260f8be1d0ed..4388618e3434 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -246,7 +246,10 @@ static int translate_eth_proto_oper(u32 eth_proto_oper, u8 *active_speed, *active_speed = IB_SPEED_EDR; break; default: - return -EINVAL; + /* Unknown */ + *active_width = IB_WIDTH_1X; + *active_speed = IB_SPEED_SDR; + break; } return 0;
If there is no SFP module plugged into a port of mlx5 cards 'cat /sys/class/infniband/mlx5_X/ports/1/rate' returns Invalid argument. This causes tools like 'ibstat' to malfunction. This change adjusts mlx5 to all other RoCE/iWarp drivers, which always return valid speed/width. Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> --- drivers/infiniband/hw/mlx5/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)