On Fri, Jul 17, 2020 at 04:57:29PM +0800, Jason Wang wrote:

quoted

Looks like checking intialized is enough. Will fix this.

quoted

quoted

+
+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.