Re: [Patch v3] vdpa/mlx5: Avoid losing link state updates
On Mon, Apr 3, 2023 at 2:47 PM Eli Cohen wrote:
>
>
> On 03/04/2023 8:01, Jason Wang wrote:
> > On Sun, Apr 2, 2023 at 10:15 PM 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 only when
> >> VIRTIO_NET_F_STATUS flips from 0 to 1 and unregister it on driver reset
> >> or suspend.
> >>
> >> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on
> >> feature bits")
> >> Signed-off-by: Eli Cohen
> >> ---
> >> v2 -> v3
> >> Only register the link event notifier when VIRTIO_NET_F_STATUS is
> >> negotiated.
> >>
> >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 200 +-
> >> 1 file changed, 112 insertions(+), 88 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> index 317cef9b7813..9b1432e22540 100644
> >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >> @@ -2322,10 +2322,115 @@ 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->nb_registered && ndev->config_cb.callback)
> > It looks to me nb_registered is accessed without synchronization. Or
> > we don't even need to check that if we do:
> >
> > unregister();
> > flush_workqueue();
> >
> > which has been done in unregister_link_notifier().
> >
> >> + 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)
> >> +{
> >> + ndev->nb.notifier_call = event_handler;
> >> + mlx5_notifier_register(ndev->mvdev.mdev, &ndev->nb);
> >> + ndev->nb
Re: [Patch v3] vdpa/mlx5: Avoid losing link state updates
On Sun, Apr 2, 2023 at 10:15 PM 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 only when
> VIRTIO_NET_F_STATUS flips from 0 to 1 and unregister it on driver reset
> or suspend.
>
> Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on
> feature bits")
> Signed-off-by: Eli Cohen
> ---
> v2 -> v3
> Only register the link event notifier when VIRTIO_NET_F_STATUS is
> negotiated.
>
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 200 +-
> 1 file changed, 112 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 317cef9b7813..9b1432e22540 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2322,10 +2322,115 @@ 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->nb_registered && ndev->config_cb.callback)
It looks to me nb_registered is accessed without synchronization. Or
we don't even need to check that if we do:
unregister();
flush_workqueue();
which has been done in unregister_link_notifier().
> + 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)
> +{
> + 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)
Under which case could we hit mvdev.wq = NULL?
(We call unregister_link_notifier() before setting mq to NULL during
device del).
> + flush_workqueue(ndev->mvde
