Re: commit ab118da4c10a70b8437f5c90ab77adae1835963e causes ib_srpt to fail connections served by target LIO

[Max Gurtovoy <maxg@xxxxxxxxxxxx>]

On 3/14/2020 11:30 PM, Laurence Oberman wrote:
> Hello Bart, Leon and Max

Hi Laurence,

thanks for the great analysis and the fast response !!

> Max had reached out to me to test a new set of patches for SRQ.
> I had not tested upstream ib_srpt on an LIO target for quite a while,
> only ib_srp client tests had been run of late.
> During a baseline test before applying Max's patches it was apparent
> that something had broken ib_srpt connections within LIO target since
> 5.5.
>
> Note thet ib_srp client connectivity with the commit functions fine,
> it's just the target that breaks with this commit.
>
> After a long bisect this is the commit that seems to break it.
> While it's not directly code in ib_srpt, its code in mlx5 vport
> ethernet connectivity that then breaks ib_srpt connectivity over mlx5
> IB RDMA with LIO.

I was able to connect in loopback and also from remote initiator with 
this commit.

So I'm not sure that this commit is broken.

I used Bart's scripts to configure the target and to connect to it in 
loopback (after some modifications for the updated kernel/sysfs/configfs 
interface).

I did see an issue to connect from remote initiator, but after reloading 
openibd in the initiator side I was able to connect.

So I suspect you had the same issue - that also should be debugged.

> I will let Leon and others decide but reverting the below commit allows
> SRP connectivity to an LIO target to work again.

I added prints to "mlx5_core_modify_hca_vport_context" function and 
found that we don't call it in "pure" mlx5 mode with PFs.

Maybe you can try it too...

I was able to check my patches on my system and I'll send them soon.

Thanks again Laurence and Bart.

> Max, I will test your new patches once we have a decision on this.
>
> Client
> Linux ibclient.lab.eng.bos.redhat.com 5.6.0-rc5+ #1 SMP Thu Mar 12
> 16:58:19 EDT 2020 x86_64 x86_64 x86_64 GNU/Linux
>
> Server with reverted commit
> Linux fedstorage.bos.redhat.com 5.6.0-rc5+ #1 SMP Sat Mar 14 16:39:35
> EDT 2020 x86_64 x86_64 x86_64 GNU/Linux
>
> commit ab118da4c10a70b8437f5c90ab77adae1835963e
> Author: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> Date:   Wed Nov 13 12:03:47 2019 +0200
>
>      net/mlx5: Don't write read-only fields in MODIFY_HCA_VPORT_CONTEXT
> command
>      
>      The MODIFY_HCA_VPORT_CONTEXT uses field_selector to mask fields
> needed
>      to be written, other fields are required to be zero according to
> the
>      HW specification. The supported fields are controlled by bitfield
>      and limited to vport state, node and port GUIDs.
>      
>      Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>      Signed-off-by: Saeed Mahameed <saeedm@xxxxxxxxxxxx>
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> b/drivers/net/ethernet/mellanox/mlx5
> index 30f7848..1faac31f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> @@ -1064,26 +1064,13 @@ int mlx5_core_modify_hca_vport_context(struct
> mlx5_core_dev *dev,
>   
>          ctx = MLX5_ADDR_OF(modify_hca_vport_context_in, in,
> hca_vport_context);
>          MLX5_SET(hca_vport_context, ctx, field_select, req-
> > field_select);
> -       MLX5_SET(hca_vport_context, ctx, sm_virt_aware, req-
> > sm_virt_aware);
> -       MLX5_SET(hca_vport_context, ctx, has_smi, req->has_smi);
> -       MLX5_SET(hca_vport_context, ctx, has_raw, req->has_raw);
> -       MLX5_SET(hca_vport_context, ctx, vport_state_policy, req-
> > policy);
> -       MLX5_SET(hca_vport_context, ctx, port_physical_state, req-
> > phys_state);
> -       MLX5_SET(hca_vport_context, ctx, vport_state, req-
> > vport_state);
> -       MLX5_SET64(hca_vport_context, ctx, port_guid, req->port_guid);
> -       MLX5_SET64(hca_vport_context, ctx, node_guid, req->node_guid);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask1, req->cap_mask1);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask1_field_select, req-
> > cap_mask1_perm);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask2, req->cap_mask2);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask2_field_select, req-
> > cap_mask2_perm);
> -       MLX5_SET(hca_vport_context, ctx, lid, req->lid);
> -       MLX5_SET(hca_vport_context, ctx, init_type_reply, req-
> > init_type_reply);
> -       MLX5_SET(hca_vport_context, ctx, lmc, req->lmc);
> -       MLX5_SET(hca_vport_context, ctx, subnet_timeout, req-
> > subnet_timeout);
> -       MLX5_SET(hca_vport_context, ctx, sm_lid, req->sm_lid);
> -       MLX5_SET(hca_vport_context, ctx, sm_sl, req->sm_sl);
> -       MLX5_SET(hca_vport_context, ctx, qkey_violation_counter, req-
> > qkey_violation_counter);
> -       MLX5_SET(hca_vport_context, ctx, pkey_violation_counter, req-
> > pkey_violation_counter);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_STATE_POLICY)
> +               MLX5_SET(hca_vport_context, ctx, vport_state_policy,
> +                        req->policy);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_PORT_GUID)
> +               MLX5_SET64(hca_vport_context, ctx, port_guid, req-
> > port_guid);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_NODE_GUID)
> +               MLX5_SET64(hca_vport_context, ctx, node_guid, req-
> > node_guid);
>          err = mlx5_cmd_exec(dev, in, in_sz, out, sizeof(out));
>   ex:
>          kfree(in);
>