Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Ghazale Hosseinabadi]

Hi,

----- Original Message -----
From: hal@dev.mellanox.co.il
To: parav@mellanox.com, tbogendoerfer@suse.de, matanb@mellanox.com, leonro@mellanox.com, dledford@redhat.com, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: ghazale.hosseinabadi@oracle.com
Sent: Friday, October 27, 2017 2:30:33 PM GMT -08:00 US/Canada Pacific
Subject: Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

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 ?

When running ibstat (if transceiver is not connected in adapter):

ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid argument

Thanks,
Ghazale

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
> When running ibstat (if transceiver is not connected in adapter):
> 
> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid argument

Any output before that ? I'm trying to understand how far it gets. It
looks to me that empty rate file would be parsed as 0 and ibstat would
show that rate. ibpanic would occur if file was not found but I could be
missing something.

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Ghazale Hosseinabadi]

On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
>> When running ibstat (if transceiver is not connected in adapter):
>>
>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid argument
> Any output before that ?
no, It only prints this line.

-- Ghazale
>   I'm trying to understand how far it gets. It
> looks to me that empty rate file would be parsed as 0 and ibstat would
> show that rate. ibpanic would occur if file was not found but I could be
> missing something.
>

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

On 10/27/2017 7:04 PM, Ghazale Hosseinabadi wrote:
> 
> 
> On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
>> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
>>> When running ibstat (if transceiver is not connected in adapter):
>>>
>>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid
>>> argument
>> Any output before that ?
> no, It only prints this line.

and setting the width to 1x in the driver so the rate file is properly
populated fixes this ? I must be missing something as to what is going
on in this scenario.

sysfs.c:rate_show is inconsistent as it paves over an invalid speed
setting that to SDR but does not pave over invalid width returning
-EINVAL but this comment is in another "direction".

-- Hal

> 
> -- Ghazale
>>   I'm trying to understand how far it gets. It
>> looks to me that empty rate file would be parsed as 0 and ibstat would
>> show that rate. ibpanic would occur if file was not found but I could be
>> missing something.
>>
> 
> 

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

On 10/27/2017 7:19 PM, Hal Rosenstock wrote:
> On 10/27/2017 7:04 PM, Ghazale Hosseinabadi wrote:
>>
>>
>> On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
>>> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
>>>> When running ibstat (if transceiver is not connected in adapter):
>>>>
>>>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid
>>>> argument
>>> Any output before that ?
>> no, It only prints this line.
> 
> and setting the width to 1x in the driver so the rate file is properly
> populated fixes this ? I must be missing something as to what is going
> on in this scenario.

[off list...]
Are you using libibumad or rdma-core package ? Which version ?
What version of infiniband-diags are you using ?
Can you build from sources ?
I have patch to libibumad/rdma-core and another patch to ibstat
(infiniband-diags) which I'd like you to try. Is that possible ?

Thanks.

-- Hal

> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
> setting that to SDR but does not pave over invalid width returning
> -EINVAL but this comment is in another "direction".
> 
> -- Hal
> 
>>
>> -- Ghazale
>>>   I'm trying to understand how far it gets. It
>>> looks to me that empty rate file would be parsed as 0 and ibstat would
>>> show that rate. ibpanic would occur if file was not found but I could be
>>> missing something.
>>>
>>
>>

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Ghazale Hosseinabadi]

On 10/27/2017 05:17 PM, Hal Rosenstock wrote:
> On 10/27/2017 7:19 PM, Hal Rosenstock wrote:
>> On 10/27/2017 7:04 PM, Ghazale Hosseinabadi wrote:
>>>
>>> On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
>>>> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
>>>>> When running ibstat (if transceiver is not connected in adapter):
>>>>>
>>>>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid
>>>>> argument
>>>> Any output before that ?
>>> no, It only prints this line.
>> and setting the width to 1x in the driver so the rate file is properly
>> populated fixes this ? I must be missing something as to what is going
>> on in this scenario.
> [off list...]
> Are you using libibumad or rdma-core package ?
rdma-core
>   Which version ?
rdma-core-13-25
> What version of infiniband-diags are you using ?
infiniband-diags-1.6.7-1
> Can you build from sources ?
> I have patch to libibumad/rdma-core and another patch to ibstat
> (infiniband-diags) which I'd like you to try. Is that possible ?
I haven't built user-land packages myself, but I can definitely try it.
Please send me the patches and I will try to build.

Thanks,
Ghazale
>
> Thanks.
>
> -- Hal
>
>> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
>> setting that to SDR but does not pave over invalid width returning
>> -EINVAL but this comment is in another "direction".
>>
>> -- Hal
>>
>>> -- Ghazale
>>>>    I'm trying to understand how far it gets. It
>>>> looks to me that empty rate file would be parsed as 0 and ibstat would
>>>> show that rate. ibpanic would occur if file was not found but I could be
>>>> missing something.
>>>>
>>>

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Ghazale Hosseinabadi]

On 10/27/2017 04:19 PM, Hal Rosenstock wrote:
> On 10/27/2017 7:04 PM, Ghazale Hosseinabadi wrote:
>>
>> On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
>>> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
>>>> When running ibstat (if transceiver is not connected in adapter):
>>>>
>>>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid
>>>> argument
>>> Any output before that ?
>> no, It only prints this line.
> and setting the width to 1x in the driver so the rate file is properly
> populated fixes this ?
Yes, because a value is written in 
/sys/class/infiniband/mlx5_X/ports/1/rate
>   I must be missing something as to what is going
> on in this scenario.
Without this bug fix, file /sys/class/infiniband/mlx5_X/ports/1/rate is 
empty, which results in ibpanic.

-- Ghazale
>
> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
> setting that to SDR but does not pave over invalid width returning
> -EINVAL but this comment is in another "direction".
>
> -- Hal
>
>> -- Ghazale
>>>    I'm trying to understand how far it gets. It
>>> looks to me that empty rate file would be parsed as 0 and ibstat would
>>> show that rate. ibpanic would occur if file was not found but I could be
>>> missing something.
>>>
>>

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Thomas Bogendoerfer]

On Fri, 27 Oct 2017 19:19:05 -0400
Hal Rosenstock <hal@dev.mellanox.co.il> wrote:

> On 10/27/2017 7:04 PM, Ghazale Hosseinabadi wrote:
> > 
> > 
> > On 10/27/2017 03:52 PM, Hal Rosenstock wrote:
> >> On 10/27/2017 5:54 PM, Ghazale Hosseinabadi wrote:
> >>> When running ibstat (if transceiver is not connected in adapter):
> >>>
> >>> ibpanic: [7851] main: stat of IB device 'mlx5_1' failed: Invalid
> >>> argument
> >> Any output before that ?
> > no, It only prints this line.

rate_show() generates an EINVAL, that's propably what ibstat/*libs confuses.

> and setting the width to 1x in the driver so the rate file is properly
> populated fixes this ? 

yes, because then rate_show() will not return EINVAL.

> I must be missing something as to what is going on in this scenario.
> 
> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
> setting that to SDR but does not pave over invalid width returning
> -EINVAL but this comment is in another "direction".

I thought about going in the other direction by making the speed/width unknown case
somehow visible in rate_show(). When I checked all other drivers only mlx5 stand out
so I decided to only change mlx5. But I make a change to let rate_show() print out, 
that is currently has no rate for the port.

Thomas.

-- 
Thomas Bogendoerfer <tbogendoerfer@suse.de>

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

On 10/28/2017 6:42 AM, Thomas Bogendoerfer wrote:
>> I must be missing something as to what is going on in this scenario.

I think I see what's going on now. The -EINVAL in kernel sysfs/rate_show
causes error to either open or read of file. I had checked that an empty
file is parsed correctly but didn't consider an error on open or read.

>> sysfs.c:rate_show is inconsistent as it paves over an invalid speed
>> setting that to SDR but does not pave over invalid width returning
>> -EINVAL but this comment is in another "direction".
> I thought about going in the other direction by making the speed/width unknown case
> somehow visible in rate_show(). When I checked all other drivers only mlx5 stand out
> so I decided to only change mlx5. But I make a change to let rate_show() print out, 
> that is currently has no rate for the port.

I wasn't proposing to make that visible in rate_show() but rather
pointing out the inconsistency in changing unknown speeds to SDR but
unknown widths are error. There were comments on not having to set
"random" numbers for speed/width. If so, shouldn't these be handled
consistently here ? Would setting -EINVAL when not one of recognized
speeds cause an issue (rather than setting to SDR) ?

IMO a more future proof implementation is not to have error for
ib_width_enum_to_int but have unknown widths default to 1x similar to
how unknown speeds default to SDR:

include/rdma/ib_verbs.h:
static inline int ib_width_enum_to_int(enum ib_port_width width)
{
        switch (width) {
        case IB_WIDTH_1X:  return  1;
        case IB_WIDTH_4X:  return  4;
        case IB_WIDTH_8X:  return  8;
        case IB_WIDTH_12X: return 12;
        default:          return 1;
        }
}

For example, 2x link widths have been added to IBA spec.

This is alternative approach to libibumad/ibstat patches for invalid
link width as it's not set when QSFP is not plugged into port.

-- Hal

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Ghazale Hosseinabadi]

Hi,

----- Original Message -----
From: hal@dev.mellanox.co.il
To: parav@mellanox.com, tbogendoerfer@suse.de, matanb@mellanox.com, leonro@mellanox.com, dledford@redhat.com, linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: ghazale.hosseinabadi@oracle.com
Sent: Friday, October 27, 2017 1:18:50 PM GMT -08:00 US/Canada Pacific
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 ?

There is a rate file, but it is empty.

Thanks,
Ghazale

-- Hal

[PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Thomas Bogendoerfer]

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

RE: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Parav Pandit]

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

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

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

RE: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Parav Pandit]

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

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Hal Rosenstock]

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 ?

Re: [PATCH] IB/mlx5: give back valid speed/width even without plugged in SFP module

[Leon Romanovsky]

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

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

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