* Re: [PATCH v5] vdpa/mlx5: Avoid losing link state updates
[not found] <20230417110343.138319-1-elic@nvidia.com>
@ 2023-04-17 11:27 ` Michael S. Tsirkin
2023-04-17 11:27 ` Michael S. Tsirkin
2023-04-17 11:41 ` Michael S. Tsirkin
2 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2023-04-17 11:27 UTC (permalink / raw)
To: Eli Cohen; +Cc: parav, virtualization, eperezma
On Mon, Apr 17, 2023 at 02:03:43PM +0300, Eli Cohen 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.
>
> Modify the code so link state notifier is registered after DRIVER_OK was
> negotiated and carry the registration only if
> VIRTIO_NET_F_STATUS was negotiated. Unregister the notifier when the
> device is reset.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on feature bits")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Message-Id: <20230404073347.40847-1-elic@nvidia.com>
What is this? Should not be there.
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 203 +++++++++++++++++-------------
> 1 file changed, 114 insertions(+), 89 deletions(-)
> v4 -> v5:
> Rebase over latest linux tree.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 195963b82b63..97a16f7eb894 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2298,6 +2298,113 @@ 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_carrier(struct work_struct *work)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> + struct mlx5_vdpa_dev *mvdev;
> + struct mlx5_vdpa_net *ndev;
> +
> + 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);
> +
> + if (ndev->config_cb.callback)
> + ndev->config_cb.callback(ndev->config_cb.private);
> +
> + kfree(wqent);
> +}
> +
> +static int queue_link_work(struct mlx5_vdpa_net *ndev)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> +
> + wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> + if (!wqent)
> + return -ENOMEM;
> +
> + wqent->mvdev = &ndev->mvdev;
> + INIT_WORK(&wqent->work, update_carrier);
> + queue_work(ndev->mvdev.wq, &wqent->work);
> + return 0;
> +}
> +
> +static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> +{
> + struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> + struct mlx5_eqe *eqe = param;
> + int ret = NOTIFY_DONE;
> +
> + if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> + switch (eqe->sub_type) {
> + case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> + case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> + if (queue_link_work(ndev))
> + return NOTIFY_DONE;
> +
> + ret = NOTIFY_OK;
> + break;
> + default:
> + return NOTIFY_DONE;
> + }
> + return ret;
> + }
> + return ret;
> +}
> +
> +static void register_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> + return;
> +
> + ndev->nb.notifier_call = event_handler;
> + mlx5_notifier_register(ndev->mvdev.mdev, &ndev->nb);
> + ndev->nb_registered = true;
> + queue_link_work(ndev);
> +}
> +
> +static void unregister_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!ndev->nb_registered)
> + return;
> +
> + ndev->nb_registered = false;
> + mlx5_notifier_unregister(ndev->mvdev.mdev, &ndev->nb);
> + if (ndev->mvdev.wq)
> + flush_workqueue(ndev->mvdev.wq);
> +}
> +
> static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> @@ -2567,10 +2674,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> mlx5_vdpa_warn(mvdev, "failed to setup control VQ vring\n");
> goto err_setup;
> }
> + register_link_notifier(ndev);
> err = setup_driver(mvdev);
> if (err) {
> mlx5_vdpa_warn(mvdev, "failed to setup driver\n");
> - goto err_setup;
> + goto err_driver;
> }
> } else {
> mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> @@ -2582,6 +2690,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> up_write(&ndev->reslock);
> return;
>
> +err_driver:
> + unregister_link_notifier(ndev);
> err_setup:
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> @@ -2607,6 +2717,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "performing device reset\n");
>
> down_write(&ndev->reslock);
> + unregister_link_notifier(ndev);
> teardown_driver(ndev);
> clear_vqs_ready(ndev);
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2861,9 +2972,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "suspending device\n");
>
> down_write(&ndev->reslock);
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - flush_workqueue(ndev->mvdev.wq);
> + unregister_link_notifier(ndev);
> for (i = 0; i < ndev->cur_num_vqs; i++) {
> mvq = &ndev->vqs[i];
> suspend_vq(ndev, mvq);
> @@ -3000,84 +3109,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;
> - struct mlx5_vdpa_dev *mvdev;
> - struct mlx5_vdpa_net *ndev;
> -
> - 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);
> -
> - if (ndev->nb_registered && ndev->config_cb.callback)
> - ndev->config_cb.callback(ndev->config_cb.private);
> -
> - kfree(wqent);
> -}
> -
> -static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> -{
> - struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> - struct mlx5_eqe *eqe = param;
> - int ret = NOTIFY_DONE;
> - struct mlx5_vdpa_wq_ent *wqent;
> -
> - if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> - return NOTIFY_DONE;
> - switch (eqe->sub_type) {
> - case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> - case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> - wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> - if (!wqent)
> - return NOTIFY_DONE;
> -
> - wqent->mvdev = &ndev->mvdev;
> - INIT_WORK(&wqent->work, update_carrier);
> - queue_work(ndev->mvdev.wq, &wqent->work);
> - ret = NOTIFY_OK;
> - break;
> - default:
> - return NOTIFY_DONE;
> - }
> - return ret;
> - }
> - return ret;
> -}
> -
> static int config_func_mtu(struct mlx5_core_dev *mdev, u16 mtu)
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_nic_vport_context_in);
> @@ -3258,9 +3289,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_res2;
> }
>
> - ndev->nb.notifier_call = event_handler;
> - mlx5_notifier_register(mdev, &ndev->nb);
> - ndev->nb_registered = true;
> mvdev->vdev.mdev = &mgtdev->mgtdev;
> err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> if (err)
> @@ -3294,10 +3322,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>
> mlx5_vdpa_remove_debugfs(ndev->debugfs);
> ndev->debugfs = NULL;
> - if (ndev->nb_registered) {
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - }
> + unregister_link_notifier(ndev);
> wq = mvdev->wq;
> mvdev->wq = NULL;
> destroy_workqueue(wq);
> --
> 2.39.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v5] vdpa/mlx5: Avoid losing link state updates
[not found] <20230417110343.138319-1-elic@nvidia.com>
2023-04-17 11:27 ` [PATCH v5] vdpa/mlx5: Avoid losing link state updates Michael S. Tsirkin
@ 2023-04-17 11:27 ` Michael S. Tsirkin
2023-04-17 11:41 ` Michael S. Tsirkin
2 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2023-04-17 11:27 UTC (permalink / raw)
To: Eli Cohen; +Cc: parav, virtualization, eperezma
On Mon, Apr 17, 2023 at 02:03:43PM +0300, Eli Cohen 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.
>
> Modify the code so link state notifier is registered after DRIVER_OK was
> negotiated and carry the registration only if
> VIRTIO_NET_F_STATUS was negotiated. Unregister the notifier when the
> device is reset.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on feature bits")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Message-Id: <20230404073347.40847-1-elic@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
this should not be there either.
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 203 +++++++++++++++++-------------
> 1 file changed, 114 insertions(+), 89 deletions(-)
> v4 -> v5:
> Rebase over latest linux tree.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 195963b82b63..97a16f7eb894 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2298,6 +2298,113 @@ 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_carrier(struct work_struct *work)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> + struct mlx5_vdpa_dev *mvdev;
> + struct mlx5_vdpa_net *ndev;
> +
> + 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);
> +
> + if (ndev->config_cb.callback)
> + ndev->config_cb.callback(ndev->config_cb.private);
> +
> + kfree(wqent);
> +}
> +
> +static int queue_link_work(struct mlx5_vdpa_net *ndev)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> +
> + wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> + if (!wqent)
> + return -ENOMEM;
> +
> + wqent->mvdev = &ndev->mvdev;
> + INIT_WORK(&wqent->work, update_carrier);
> + queue_work(ndev->mvdev.wq, &wqent->work);
> + return 0;
> +}
> +
> +static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> +{
> + struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> + struct mlx5_eqe *eqe = param;
> + int ret = NOTIFY_DONE;
> +
> + if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> + switch (eqe->sub_type) {
> + case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> + case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> + if (queue_link_work(ndev))
> + return NOTIFY_DONE;
> +
> + ret = NOTIFY_OK;
> + break;
> + default:
> + return NOTIFY_DONE;
> + }
> + return ret;
> + }
> + return ret;
> +}
> +
> +static void register_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> + return;
> +
> + ndev->nb.notifier_call = event_handler;
> + mlx5_notifier_register(ndev->mvdev.mdev, &ndev->nb);
> + ndev->nb_registered = true;
> + queue_link_work(ndev);
> +}
> +
> +static void unregister_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!ndev->nb_registered)
> + return;
> +
> + ndev->nb_registered = false;
> + mlx5_notifier_unregister(ndev->mvdev.mdev, &ndev->nb);
> + if (ndev->mvdev.wq)
> + flush_workqueue(ndev->mvdev.wq);
> +}
> +
> static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> @@ -2567,10 +2674,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> mlx5_vdpa_warn(mvdev, "failed to setup control VQ vring\n");
> goto err_setup;
> }
> + register_link_notifier(ndev);
> err = setup_driver(mvdev);
> if (err) {
> mlx5_vdpa_warn(mvdev, "failed to setup driver\n");
> - goto err_setup;
> + goto err_driver;
> }
> } else {
> mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> @@ -2582,6 +2690,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> up_write(&ndev->reslock);
> return;
>
> +err_driver:
> + unregister_link_notifier(ndev);
> err_setup:
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> @@ -2607,6 +2717,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "performing device reset\n");
>
> down_write(&ndev->reslock);
> + unregister_link_notifier(ndev);
> teardown_driver(ndev);
> clear_vqs_ready(ndev);
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2861,9 +2972,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "suspending device\n");
>
> down_write(&ndev->reslock);
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - flush_workqueue(ndev->mvdev.wq);
> + unregister_link_notifier(ndev);
> for (i = 0; i < ndev->cur_num_vqs; i++) {
> mvq = &ndev->vqs[i];
> suspend_vq(ndev, mvq);
> @@ -3000,84 +3109,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;
> - struct mlx5_vdpa_dev *mvdev;
> - struct mlx5_vdpa_net *ndev;
> -
> - 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);
> -
> - if (ndev->nb_registered && ndev->config_cb.callback)
> - ndev->config_cb.callback(ndev->config_cb.private);
> -
> - kfree(wqent);
> -}
> -
> -static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> -{
> - struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> - struct mlx5_eqe *eqe = param;
> - int ret = NOTIFY_DONE;
> - struct mlx5_vdpa_wq_ent *wqent;
> -
> - if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> - return NOTIFY_DONE;
> - switch (eqe->sub_type) {
> - case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> - case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> - wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> - if (!wqent)
> - return NOTIFY_DONE;
> -
> - wqent->mvdev = &ndev->mvdev;
> - INIT_WORK(&wqent->work, update_carrier);
> - queue_work(ndev->mvdev.wq, &wqent->work);
> - ret = NOTIFY_OK;
> - break;
> - default:
> - return NOTIFY_DONE;
> - }
> - return ret;
> - }
> - return ret;
> -}
> -
> static int config_func_mtu(struct mlx5_core_dev *mdev, u16 mtu)
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_nic_vport_context_in);
> @@ -3258,9 +3289,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_res2;
> }
>
> - ndev->nb.notifier_call = event_handler;
> - mlx5_notifier_register(mdev, &ndev->nb);
> - ndev->nb_registered = true;
> mvdev->vdev.mdev = &mgtdev->mgtdev;
> err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> if (err)
> @@ -3294,10 +3322,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>
> mlx5_vdpa_remove_debugfs(ndev->debugfs);
> ndev->debugfs = NULL;
> - if (ndev->nb_registered) {
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - }
> + unregister_link_notifier(ndev);
> wq = mvdev->wq;
> mvdev->wq = NULL;
> destroy_workqueue(wq);
> --
> 2.39.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v5] vdpa/mlx5: Avoid losing link state updates
[not found] <20230417110343.138319-1-elic@nvidia.com>
2023-04-17 11:27 ` [PATCH v5] vdpa/mlx5: Avoid losing link state updates Michael S. Tsirkin
2023-04-17 11:27 ` Michael S. Tsirkin
@ 2023-04-17 11:41 ` Michael S. Tsirkin
2 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2023-04-17 11:41 UTC (permalink / raw)
To: Eli Cohen; +Cc: parav, virtualization, eperezma
And in fact this is v6, isn't it? Similar to v6 but
rebased?
On Mon, Apr 17, 2023 at 02:03:43PM +0300, Eli Cohen 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.
>
> Modify the code so link state notifier is registered after DRIVER_OK was
> negotiated and carry the registration only if
> VIRTIO_NET_F_STATUS was negotiated. Unregister the notifier when the
> device is reset.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on feature bits")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Message-Id: <20230404073347.40847-1-elic@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 203 +++++++++++++++++-------------
> 1 file changed, 114 insertions(+), 89 deletions(-)
> v4 -> v5:
> Rebase over latest linux tree.
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 195963b82b63..97a16f7eb894 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2298,6 +2298,113 @@ 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_carrier(struct work_struct *work)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> + struct mlx5_vdpa_dev *mvdev;
> + struct mlx5_vdpa_net *ndev;
> +
> + 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);
> +
> + if (ndev->config_cb.callback)
> + ndev->config_cb.callback(ndev->config_cb.private);
> +
> + kfree(wqent);
> +}
> +
> +static int queue_link_work(struct mlx5_vdpa_net *ndev)
> +{
> + struct mlx5_vdpa_wq_ent *wqent;
> +
> + wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> + if (!wqent)
> + return -ENOMEM;
> +
> + wqent->mvdev = &ndev->mvdev;
> + INIT_WORK(&wqent->work, update_carrier);
> + queue_work(ndev->mvdev.wq, &wqent->work);
> + return 0;
> +}
> +
> +static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> +{
> + struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> + struct mlx5_eqe *eqe = param;
> + int ret = NOTIFY_DONE;
> +
> + if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> + switch (eqe->sub_type) {
> + case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> + case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> + if (queue_link_work(ndev))
> + return NOTIFY_DONE;
> +
> + ret = NOTIFY_OK;
> + break;
> + default:
> + return NOTIFY_DONE;
> + }
> + return ret;
> + }
> + return ret;
> +}
> +
> +static void register_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> + return;
> +
> + ndev->nb.notifier_call = event_handler;
> + mlx5_notifier_register(ndev->mvdev.mdev, &ndev->nb);
> + ndev->nb_registered = true;
> + queue_link_work(ndev);
> +}
> +
> +static void unregister_link_notifier(struct mlx5_vdpa_net *ndev)
> +{
> + if (!ndev->nb_registered)
> + return;
> +
> + ndev->nb_registered = false;
> + mlx5_notifier_unregister(ndev->mvdev.mdev, &ndev->nb);
> + if (ndev->mvdev.wq)
> + flush_workqueue(ndev->mvdev.wq);
> +}
> +
> static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> {
> struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> @@ -2567,10 +2674,11 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> mlx5_vdpa_warn(mvdev, "failed to setup control VQ vring\n");
> goto err_setup;
> }
> + register_link_notifier(ndev);
> err = setup_driver(mvdev);
> if (err) {
> mlx5_vdpa_warn(mvdev, "failed to setup driver\n");
> - goto err_setup;
> + goto err_driver;
> }
> } else {
> mlx5_vdpa_warn(mvdev, "did not expect DRIVER_OK to be cleared\n");
> @@ -2582,6 +2690,8 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> up_write(&ndev->reslock);
> return;
>
> +err_driver:
> + unregister_link_notifier(ndev);
> err_setup:
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> ndev->mvdev.status |= VIRTIO_CONFIG_S_FAILED;
> @@ -2607,6 +2717,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "performing device reset\n");
>
> down_write(&ndev->reslock);
> + unregister_link_notifier(ndev);
> teardown_driver(ndev);
> clear_vqs_ready(ndev);
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> @@ -2861,9 +2972,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> mlx5_vdpa_info(mvdev, "suspending device\n");
>
> down_write(&ndev->reslock);
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - flush_workqueue(ndev->mvdev.wq);
> + unregister_link_notifier(ndev);
> for (i = 0; i < ndev->cur_num_vqs; i++) {
> mvq = &ndev->vqs[i];
> suspend_vq(ndev, mvq);
> @@ -3000,84 +3109,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;
> - struct mlx5_vdpa_dev *mvdev;
> - struct mlx5_vdpa_net *ndev;
> -
> - 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);
> -
> - if (ndev->nb_registered && ndev->config_cb.callback)
> - ndev->config_cb.callback(ndev->config_cb.private);
> -
> - kfree(wqent);
> -}
> -
> -static int event_handler(struct notifier_block *nb, unsigned long event, void *param)
> -{
> - struct mlx5_vdpa_net *ndev = container_of(nb, struct mlx5_vdpa_net, nb);
> - struct mlx5_eqe *eqe = param;
> - int ret = NOTIFY_DONE;
> - struct mlx5_vdpa_wq_ent *wqent;
> -
> - if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> - if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> - return NOTIFY_DONE;
> - switch (eqe->sub_type) {
> - case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> - case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> - wqent = kzalloc(sizeof(*wqent), GFP_ATOMIC);
> - if (!wqent)
> - return NOTIFY_DONE;
> -
> - wqent->mvdev = &ndev->mvdev;
> - INIT_WORK(&wqent->work, update_carrier);
> - queue_work(ndev->mvdev.wq, &wqent->work);
> - ret = NOTIFY_OK;
> - break;
> - default:
> - return NOTIFY_DONE;
> - }
> - return ret;
> - }
> - return ret;
> -}
> -
> static int config_func_mtu(struct mlx5_core_dev *mdev, u16 mtu)
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_nic_vport_context_in);
> @@ -3258,9 +3289,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_res2;
> }
>
> - ndev->nb.notifier_call = event_handler;
> - mlx5_notifier_register(mdev, &ndev->nb);
> - ndev->nb_registered = true;
> mvdev->vdev.mdev = &mgtdev->mgtdev;
> err = _vdpa_register_device(&mvdev->vdev, max_vqs + 1);
> if (err)
> @@ -3294,10 +3322,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>
> mlx5_vdpa_remove_debugfs(ndev->debugfs);
> ndev->debugfs = NULL;
> - if (ndev->nb_registered) {
> - ndev->nb_registered = false;
> - mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> - }
> + unregister_link_notifier(ndev);
> wq = mvdev->wq;
> mvdev->wq = NULL;
> destroy_workqueue(wq);
> --
> 2.39.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 3+ messages in thread