Skip to contentSkip to conversation

RDMA/mlx4: Read pkey table length instead of hardcoded value

PATCH In review Leon Romanovsky 2 participants 4 messages Aug 23, 2020

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

Leon Romanovsky

author
#0 lore raw

PATCH 1/1 RDMA/mlx4: Read pkey table length instead of hardcoded value

From: Mark Bloch <[email protected]>

The driver shouldn't assume that a pkey table is available, this can happen if RoCE isn't supported by the device.

Use the pkey table length reported by the device. This together with the cited commit from Jack caused a regression where mlx4 devices without RoCE aren't created.

Cc <[email protected]>
Cc Long Li <[email protected]>
Fixes 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference in pkey cache")
Fixes fa417f7b520e ("IB/mlx4: Add support for IBoE")
Signed-off-by Mark Bloch <[email protected]>
Reviewed-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx4/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/infiniband/hw/mlx4/main.c +2 -1
index 5e7910a517da..bd4f975e7f9a 100644
@@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device *ibdev, u8 port,
 	props->ip_gids = true;
 	props->gid_tbl_len	= mdev->dev->caps.gid_table_len[port];
 	props->max_msg_sz	= mdev->dev->caps.max_msg_sz;
-	props->pkey_tbl_len	= 1;
+	if (mdev->dev->caps.pkey_table_len[port])
+		props->pkey_tbl_len = 1;
 	props->max_mtu		= IB_MTU_4096;
 	props->max_vl_num	= 2;
 	props->state		= IB_PORT_DOWN;
Signature · 1 line
2.26.2
#1 lore raw
Quoted 8 lines · On Sun, 23 Aug 2020 09:17:54 +0300 Leon Romanovsky <[email protected]> wrote:

From: Mark Bloch <[email protected]>

The driver shouldn't assume that a pkey table is available, this can happen if RoCE isn't supported by the device.

Use the pkey table length reported by the device. This together with the cited commit from Jack caused a regression where mlx4 devices without RoCE aren't created.

I don't understand. Do you mean that WITH this patch there is a regression, or do you mean that this patch FIXES the regression?

If this patch fixes the regression, I suggest the following replacement text for the last paragraph:

If the pkey_table is not available (which is the case when RoCE is not supported), the cited commit caused a regression where mlx4_devices without RoCE are not created.

Fix this by returning a pkey table length of zero in procedure eth_link_query_port() if the pkey-table length reported by the device is zero.

Quoted 21 lines
Cc <[email protected]>
Cc Long Li <[email protected]>
Fixes 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference

in pkey cache") Fixes: fa417f7b520e ("IB/mlx4: Add support for IBoE")

Signed-off-by Mark Bloch <[email protected]>
Reviewed-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx4/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/infiniband/hw/mlx4/main.c index 5e7910a517da..bd4f975e7f9a 100644 --- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c

(quoted diff) +0 -0
@@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device
 *ibdev, u8 port, props->ip_gids = true;
 	props->gid_tbl_len	=

mdev->dev->caps.gid_table_len[port]; props->max_msg_sz =
mdev->dev->caps.max_msg_sz;

drivers/infiniband/hw/mlx4/main.c static int eth_link_query_port(struct ib_device *ibdev, u8 port, · line 787

Quoted 1 line
drivers/infiniband/hw/mlx4/main.c +0 -1
-	props->pkey_tbl_len	= 1;

I don't like depending on the caller to provide a zeroed-out props structure.
I think it is better to do:
props->pkey_tbl_len = mdev->dev->caps.pkey_table_len[port] ? 1 : 0 ;
so that the pkey_table_len value is set no matter what.

drivers/infiniband/hw/mlx4/main.c static int eth_link_query_port(struct ib_device *ibdev, u8 port, · line 791

Quoted 5 lines
drivers/infiniband/hw/mlx4/main.c +2 -0
+	if (mdev->dev->caps.pkey_table_len[port])
+		props->pkey_tbl_len = 1;
 	props->max_mtu		= IB_MTU_4096;
 	props->max_vl_num	= 2;
 	props->state		= IB_PORT_DOWN;

-Jack

author
#2 lore raw
Quoted 8 lines · jackm
Quoted 5 lines · On Sun, 23 Aug 2020 09:17:54 +0300 Leon Romanovsky <[email protected]> wrote:

From: Mark Bloch <[email protected]>

The driver shouldn't assume that a pkey table is available, this can happen if RoCE isn't supported by the device.

Quoted 6 lines
Quoted 3 lines

Use the pkey table length reported by the device. This together with the cited commit from Jack caused a regression where mlx4 devices without RoCE aren't created.

I don't understand. Do you mean that WITH this patch there is a regression, or do you mean that this patch FIXES the regression?

This specific patch fixes regression.

Quoted 11 lines

If this patch fixes the regression, I suggest the following replacement text for the last paragraph:

If the pkey_table is not available (which is the case when RoCE is not supported), the cited commit caused a regression where mlx4_devices without RoCE are not created.

Fix this by returning a pkey table length of zero in procedure eth_link_query_port() if the pkey-table length reported by the device is zero.

I'll change, thanks.

Quoted 27 lines
Quoted 22 lines
Cc <[email protected]>
Cc Long Li <[email protected]>
Fixes 1901b91f9982 ("IB/core: Fix potential NULL pointer dereference

in pkey cache") Fixes: fa417f7b520e ("IB/mlx4: Add support for IBoE")

Signed-off-by Mark Bloch <[email protected]>
Reviewed-by Maor Gottlieb <[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
 drivers/infiniband/hw/mlx4/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
(quoted diff) +0 -0

b/drivers/infiniband/hw/mlx4/main.c index 5e7910a517da..bd4f975e7f9a 100644 --- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c

(quoted diff) +0 -0
@@ -784,7 +784,8 @@ static int eth_link_query_port(struct ib_device
 *ibdev, u8 port, props->ip_gids = true;
 	props->gid_tbl_len	=

mdev->dev->caps.gid_table_len[port]; props->max_msg_sz =
mdev->dev->caps.max_msg_sz;

drivers/infiniband/hw/mlx4/main.c static int eth_link_query_port(struct ib_device *ibdev, u8 port, · line 787

Quoted 1 line
drivers/infiniband/hw/mlx4/main.c +0 -1
-	props->pkey_tbl_len	= 1;

I don't like depending on the caller to provide a zeroed-out props structure.
I think it is better to do:

Quoted 2 lines
   props->pkey_tbl_len = mdev->dev->caps.pkey_table_len[port] ? 1 : 0 ;

so that the pkey_table_len value is set no matter what.

"props" are cleared by definition of IB/core to make sure that drivers doesn't return junk in ->query_port() for the fields that are not assigned. This is why I removed redundant assignment to 0.

Thanks

Quoted 9 lines

drivers/infiniband/hw/mlx4/main.c static int eth_link_query_port(struct ib_device *ibdev, u8 port, · line 791

Quoted 6 lines
drivers/infiniband/hw/mlx4/main.c +2 -0
+	if (mdev->dev->caps.pkey_table_len[port])
+		props->pkey_tbl_len = 1;
 	props->max_mtu		= IB_MTU_4096;
 	props->max_vl_num	= 2;
 	props->state		= IB_PORT_DOWN;

-Jack

#3 lore raw
Quoted 5 lines · On Sun, 23 Aug 2020 15:33:42 +0300 Leon Romanovsky <[email protected]> wrote:

"props" are cleared by definition of IB/core to make sure that drivers doesn't return junk in ->query_port() for the fields that are not assigned. This is why I removed redundant assignment to 0.

Thanks

OK, got it. I remove my objection. No RoCE support basically means that the pkey_tbl_len field should be treated as an unassigned field (which will remain zero, like all the other unassigned fields).

-Jack