On Tue, Mar 28, 2023 at 3:18?PM Eli Cohen <elic at nvidia.com>
wrote:>
> Current code ignores link state updates if VIRTIO_NET_F_STATUS was not
> negotiated. However, link state updates could be received before feature
> negotiation was completed , therefore causing link state events to be
> lost, possibly leaving the link state down.
>
> Add code to detect if VIRTIO_NET_F_STATUS was set and update the link
> state. We add a spinlock to serialize updated to config.status to
> maintain its integrity.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional
on feature bits")
> Signed-off-by: Eli Cohen <elic at nvidia.com>
>
> ---
> v1 -> v2:
> Remove helper function and integrate logic inline
>
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 82 ++++++++++++++++++-------------
> drivers/vdpa/mlx5/net/mlx5_vnet.h | 2 +
> 2 files changed, 51 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 0809ee8f6d38..85866ace0061 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2322,10 +2322,53 @@ static void update_cvq_info(struct mlx5_vdpa_dev
*mvdev)
> }
> }
>
> +static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16
vport)
> +{
> + u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> + u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> + int err;
> +
> + MLX5_SET(query_vport_state_in, in, opcode,
MLX5_CMD_OP_QUERY_VPORT_STATE);
> + MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> + MLX5_SET(query_vport_state_in, in, vport_number, vport);
> + if (vport)
> + MLX5_SET(query_vport_state_in, in, other_vport, 1);
> +
> + err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> + if (err)
> + return 0;
> +
> + return MLX5_GET(query_vport_state_out, out, state);
> +}
> +
> +static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> +{
> + if (query_vport_state(mvdev->mdev,
MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) => + VPORT_STATE_UP)
> + return true;
> +
> + return false;
> +}
> +
> +static void update_link_state(struct mlx5_vdpa_dev *mvdev)
> +{
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + bool up;
> +
> + up = get_link_state(mvdev);
> + spin_lock(&ndev->status_lock);
> + if (up)
> + ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
VIRTIO_NET_S_LINK_UP);
> + else
> + ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
~VIRTIO_NET_S_LINK_UP);
> + spin_unlock(&ndev->status_lock);
> +}
> +
> static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64
features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> + u64 old;
> int err;
>
> print_features(mvdev, features, true);
> @@ -2334,7 +2377,11 @@ static int mlx5_vdpa_set_driver_features(struct
vdpa_device *vdev, u64 features)
> if (err)
> return err;
>
> + old = ndev->mvdev.actual_features;
> ndev->mvdev.actual_features = features &
ndev->mvdev.mlx_features;
> + if (~old & ndev->mvdev.actual_features &
BIT_ULL(VIRTIO_NET_F_STATUS))
> + update_link_state(mvdev);
Can we use workqueue to avoid introducing the spinlock here?
> +
> if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev,
ndev->config.max_virtqueue_pairs);
> else
> @@ -3019,34 +3066,6 @@ struct mlx5_vdpa_mgmtdev {
> struct mlx5_vdpa_net *ndev;
> };
>
> -static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16
vport)
> -{
> - u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> - u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> - int err;
> -
> - MLX5_SET(query_vport_state_in, in, opcode,
MLX5_CMD_OP_QUERY_VPORT_STATE);
> - MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> - MLX5_SET(query_vport_state_in, in, vport_number, vport);
> - if (vport)
> - MLX5_SET(query_vport_state_in, in, other_vport, 1);
> -
> - err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> - if (err)
> - return 0;
> -
> - return MLX5_GET(query_vport_state_out, out, state);
> -}
> -
> -static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> -{
> - if (query_vport_state(mvdev->mdev,
MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) => - VPORT_STATE_UP)
> - return true;
> -
> - return false;
> -}
> -
> static void update_carrier(struct work_struct *work)
> {
> struct mlx5_vdpa_wq_ent *wqent;
> @@ -3056,11 +3075,7 @@ static void update_carrier(struct work_struct *work)
> wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> mvdev = wqent->mvdev;
> ndev = to_mlx5_vdpa_ndev(mvdev);
> - if (get_link_state(mvdev))
> - ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
VIRTIO_NET_S_LINK_UP);
> - else
> - ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
~VIRTIO_NET_S_LINK_UP);
> -
> + update_link_state(mvdev);
I still don't see how this can prevent link status from being updated
before DRIVER_OK.
It looks to me the correct fix is to only register the notfier after
DRIVER_OK is set.
Thanks
> if (ndev->nb_registered && ndev->config_cb.callback)
> ndev->config_cb.callback(ndev->config_cb.private);
>
> @@ -3198,6 +3213,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev
*v_mdev, const char *name,
>
> init_mvqs(ndev);
> init_rwsem(&ndev->reslock);
> + spin_lock_init(&ndev->status_lock);
> config = &ndev->config;
>
> if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.h
b/drivers/vdpa/mlx5/net/mlx5_vnet.h
> index c90a89e1de4d..3666bbaa8822 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.h
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.h
> @@ -50,6 +50,8 @@ struct mlx5_vdpa_net {
> struct mlx5_vdpa_wq_ent cvq_ent;
> struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
> struct dentry *debugfs;
> + /* serialize link status updates */
> + spinlock_t status_lock;
> };
>
> struct mlx5_vdpa_counter {
> --
> 2.38.1
>