Skip to contentSkip to conversation

IB/core: Set invalid active widths to 1X when port is not active

PATCH Applied Honggang LI 3 participants 10 messages Mar 15, 2018 – Mar 19, 2018

Subsystems (2)
InfiniBand Mellanox MLX5 IB driver
lore
Share & tools
lore

Honggang LI

author
#0 lore raw

cover IB/core: Set invalid active widths to 1X when port is not active

From: Honggang Li <[email protected]>

Before the mlx5 RoCE port negotiate the active rate and width with remote side, the mlx5_ib driver set them to zero.

The tool 'ibstat' of 'infiniband-diags' package will panic as it read invalid width from the sys file. Set the active widths to lowest valid vaule as what ib_core module did for invalid rates.

Honggang Li (2):
IB/core: Set speed string to SDR for invalid active rates IB/core: Set width to 1X for invalid active widths when port is down

 drivers/infiniband/core/sysfs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
Signature · 1 line
2.14.GIT
author
#1 lore raw

PATCH 1/2 IB/core: Set speed string to SDR for invalid active rates

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Now, the active_width and active_speed are zeros if the RoCE port is in DOWN state. The speed string should be set to " SDR" instead of a blank string when active_speed is zero.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 1 +
 1 file changed, 1 insertion(+)
drivers/infiniband/core/sysfs.c +1 -0
index 8ae1308eecc7..cf36ff1f0068 100644
@@ -273,6 +273,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	case IB_SPEED_SDR:
 	default:		/* default to SDR for invalid rates */
+		speed = " SDR";
 		rate = 25;
 		break;
 	}
Signature · 1 line
2.14.GIT
Applied
#2 lore raw

Re: [PATCH 1/2] IB/core: Set speed string to SDR for invalid active rates

Quoted 7 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Quoted 7 lines

Now, the active_width and active_speed are zeros if the RoCE port is in DOWN state. The speed string should be set to " SDR" instead of a blank string when active_speed is zero.

Signed-off-by Honggang Li <[email protected]>
 drivers/infiniband/core/sysfs.c | 1 +
 1 file changed, 1 insertion(+)

Applied to for-next, thanks

Jason

author
#3 lore raw

PATCH 2/2 IB/core: Set width to 1X for invalid active widths when port is down

In reply to Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +11 -4
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)
+			width = 1; /* default to 1X for invalid widths */
+		else
+			return -EINVAL;
+	}
+
+	rate *= width;
 
 	return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
 		       rate / 10, rate % 10 ? ".5" : "",
-		       ib_width_enum_to_int(attr.active_width), speed);
+		       width, speed);
 }
 
 static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
Signature · 1 line
2.14.GIT
#4 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

		if (attr.state != IB_PORT_DOWN)

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed there rather than here ?

-- Hal

Quoted 15 lines
drivers/infiniband/core/sysfs.c +7 -1
+			width = 1; /* default to 1X for invalid widths */
+		else
+			return -EINVAL;
+	}
+
+	rate *= width;
 
 	return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
 		       rate / 10, rate % 10 ? ".5" : "",
-		       ib_width_enum_to_int(attr.active_width), speed);
+		       width, speed);
 }
 
 static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
author
#5 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

Quoted 43 lines · Hal Rosenstock
Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

Quoted 1 line
		if (attr.state != IB_PORT_DOWN)

OK, thanks for correct this.

Quoted 4 lines

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed

mlx5_ib, commit f1b65df5a232 changed the default active width and rate.

Quoted 1 line

there rather than here ?

Yes, I think so, as only mlx5 RoCE impacted by this issue. So, fix the driver is also acceptable.

#6 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

In reply to Hal Rosenstock
Quoted 43 lines · Hal Rosenstock
Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

Quoted 7 lines
		if (attr.state != IB_PORT_DOWN)

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed there rather than here ?

I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:

        switch (eth_proto_oper) {
...
        default:
                return -EINVAL;
        }

        return 0;

and change default case to:

		*active_width = IB_WIDTH_1X;

?

Quoted 17 lines

-- Hal

Quoted 15 lines
drivers/infiniband/core/sysfs.c +7 -1
+			width = 1; /* default to 1X for invalid widths */
+		else
+			return -EINVAL;
+	}
+
+	rate *= width;
 
 	return sprintf(buf, "%d%s Gb/sec (%dX%s)\n",
 		       rate / 10, rate % 10 ? ".5" : "",
-		       ib_width_enum_to_int(attr.active_width), speed);
+		       width, speed);
 }
 
 static ssize_t phys_state_show(struct ib_port *p, struct port_attribute *unused,
author
#7 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

Quoted 67 lines · Hal Rosenstock
Quoted 43 lines · Hal Rosenstock
Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

Quoted 7 lines
		if (attr.state != IB_PORT_DOWN)

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed there rather than here ?

I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:

        switch (eth_proto_oper) {
...
        default:
                return -EINVAL;
        }

        return 0;
Quoted 4 lines

and change default case to:

		*active_width = IB_WIDTH_1X;

?

I suggest to restore previous behavior before apply f1b65df5a232.

drivers/infiniband/hw/mlx5/main.c +3 -0
index 033b6af90de9..0d73d2772d9b 100644
@@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
 	if (err)
 		goto out;
 
+        props->active_width     = IB_WIDTH_4X;
+        props->active_speed     = IB_SPEED_QDR;
+
 	translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
 				 &props->active_width);
#8 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

Quoted 80 lines · Honggang LI
Quoted 67 lines · Hal Rosenstock
Quoted 43 lines · Hal Rosenstock
Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

Quoted 7 lines
		if (attr.state != IB_PORT_DOWN)

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed there rather than here ?

I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:

        switch (eth_proto_oper) {
...
        default:
                return -EINVAL;
        }

        return 0;
Quoted 4 lines

and change default case to:

		*active_width = IB_WIDTH_1X;

?

I suggest to restore previous behavior before apply f1b65df5a232.

drivers/infiniband/hw/mlx5/main.c +0 -0
index 033b6af90de9..0d73d2772d9b 100644

drivers/infiniband/hw/mlx5/main.c static int mlx5_query_port_roce(struct ib_device *device, u8 port_num, · line 396

Quoted 9 lines
drivers/infiniband/hw/mlx5/main.c +3 -0
@@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
 	if (err)
 		goto out;
 
+        props->active_width     = IB_WIDTH_4X;
+        props->active_speed     = IB_SPEED_QDR;
+
 	translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
 				 &props->active_width);

Sure; makes sense that it should preserve the original behavior for this case.

Thanks.

-- Hal

author
#9 lore raw

Re: [PATCH 2/2] IB/core: Set width to 1X for invalid active widths when port is down

Quoted 84 lines · Hal Rosenstock
Quoted 83 lines · Honggang LI
Quoted 67 lines · Hal Rosenstock
Quoted 43 lines · Hal Rosenstock
Quoted 30 lines · Honggang LI

From: Honggang Li <[email protected]>

commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

When the RoCE port is down, the RoCE port did not negotiate the active width with remote side. The active width is zero. If run ibstat to require the port status, ibstat will panic as it read invalid width from sys file.

Signed-off-by Honggang Li <[email protected]>
---
 drivers/infiniband/core/sysfs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
drivers/infiniband/core/sysfs.c +1 -0
index cf36ff1f0068..722e4571f4d2 100644
@@ -240,6 +240,7 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 	struct ib_port_attr attr;
 	char *speed = "";
 	int rate;		/* in deci-Gb/sec */
+	int width;
 	ssize_t ret;
 
 	ret = ib_query_port(p->ibdev, p->port_num, &attr);

drivers/infiniband/core/sysfs.c static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused, · line 284

Quoted 9 lines
drivers/infiniband/core/sysfs.c +3 -3
@@ -278,13 +279,19 @@ static ssize_t rate_show(struct ib_port *p, struct port_attribute *unused,
 		break;
 	}
 
-	rate *= ib_width_enum_to_int(attr.active_width);
-	if (rate < 0)
-		return -EINVAL;
+	width = ib_width_enum_to_int(attr.active_width);
+	if (width < 0) {
+		if (attr.state != IB_PORT_ACTIVE)

Link width is valid in any PortState other than Down so I think that this check should be:

Quoted 7 lines
		if (attr.state != IB_PORT_DOWN)

However, I don't think overriding width should be needed for this case and just returning -EINVAL should be fine regardless of port state. AFAIK it's the driver responsibility to populate acceptable defaults for such parameters. What driver(s) have this issue ? Shouldn't it be fixed there rather than here ?

I just noticed that you reference commit f1b65df5a232 ("IB/mlx5: Add support for active_width and active_speed in RoCE"). Before this patch applied, the mlx5_ib driver set default active_width and active_speed to IB_WIDTH_4X and IB_SPEED_QDR.

Should the fix be to hw/mlx5/main.c:translate_eth_proto_oper which now has:

        switch (eth_proto_oper) {
...
        default:
                return -EINVAL;
        }

        return 0;
Quoted 4 lines

and change default case to:

		*active_width = IB_WIDTH_1X;

?

I suggest to restore previous behavior before apply f1b65df5a232.

drivers/infiniband/hw/mlx5/main.c +0 -0
index 033b6af90de9..0d73d2772d9b 100644
@@ -388,6 +388,9 @@ static int mlx5_query_port_roce(struct ib_device *device, u8 port_num,
 	if (err)
 		goto out;
Quoted 9 lines

drivers/infiniband/hw/mlx5/main.c static int mlx5_query_port_roce(struct ib_device *device, u8 port_num, · line 396

Quoted 6 lines
drivers/infiniband/hw/mlx5/main.c +3 -0
+        props->active_width     = IB_WIDTH_4X;
+        props->active_speed     = IB_SPEED_QDR;
+
 	translate_eth_proto_oper(eth_prot_oper, &props->active_speed,
 				 &props->active_width);

Sure; makes sense that it should preserve the original behavior for this case.

[PATCH] IB/mlx5: Set the default active rate and width to QDR and 4X

This new patch had been sent to upstream mailing list for review.

thanks