On Fri, Jul 17, 2020 at 04:57:29PM +0800, Jason Wang wrote:
> >Looks like checking intialized is enough. Will fix this.
> >>>+
> >>>+static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx,
> >>>bool ready)
> >>>+{
> >>>+ struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >>>+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >>>+ struct mlx5_vdpa_virtqueue *mvq = &ndev->vqs[idx];
> >>>+ int err;
> >>>+
> >>>+ if (!mvq->ready && ready && mvq->fw_state !=
> >>>MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY) {
> >>>+ err = modify_virtqueue(ndev, mvq,
> >>>MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> >>>+ if (err) {
> >>>+ mlx5_vdpa_warn(mvdev, "failed to modify
> >>>virtqueue(%d)\n", err);
> >>>+ return;
> >>>+ }
> >>>+ }
> >>
> >>I wonder what's the reason of changing vq state on the hardware
> >>here. I think we can defer it to set_status().
> >>
> >I can defer this to set status.
> >
> >I just wonder if it is possible that the core vdpa driver may call this
> >function with ready equals false and after some time call it with ready
> >equals true.
>
>
> Good point, so I think we can keep the logic. But looks like the
> code can not work if ready equals false since it only tries to
> modify vq state to RDY.
>
The point is that you cannot modify the virtqueue to "not ready". The
only option is to destroy it and create a new one. This means that if I
get ready equals false after the virtqueue has been created I need to
teardown the driver and set it up again.
Given that, I think your original suggestion to defer this logic is
reasonable.
