Re: [PATCH rdma-rc] RDMA/mlx4: Read pkey table length instead of hardcoded value

[jackm <jackm@dev.mellanox.co.il>]

On Sun, 23 Aug 2020 09:17:54 +0300
Leon Romanovsky <leon@kernel.org> wrote:

> From: Mark Bloch <markb@mellanox.com>
> 
> 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.

> 
> Cc: <stable@vger.kernel.org>
> Cc: Long Li <longli@microsoft.com>
> 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 <markb@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> ---
>  drivers/infiniband/hw/mlx4/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c
> 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
> @@ -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;

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.

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