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

[Leon Romanovsky <leon@kernel.org>]

On Sun, Aug 23, 2020 at 02:27:39PM +0300, jackm wrote:
> 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?

This specific patch fixes 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.

I'll change, thanks.

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

"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

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