On Sun, Aug 23, 2020 at 02:27:39PM +0300, jackm wrote:
> On Sun, 23 Aug 2020 09:17:54 +0300
> Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> > From: Mark Bloch <markb@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Cc: Long Li <longli@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> > Reviewed-by: Maor Gottlieb <maorg@xxxxxxxxxx>
> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > ---
> > 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
>
