virtio_rpmsg: set DRIVER_OK before using device

PATCH Applied Michael S. Tsirkin 3 participants 7 messages Mar 7, 2015 – Mar 12, 2015

Subsystems (3)

KVM virtio core Remote processor messaging (rpmsg)

lore

Share & tools

lore

Michael S. Tsirkin

author maintainer

2015-03-07 19:06 UTC #0 lore raw

PATCH 1/1 virtio_rpmsg: set DRIVER_OK before using device

virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.

virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.

The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.

Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Note: compile-tested only.

 drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/rpmsg/virtio_rpmsg_bus.c +16 -1

index 92f6af6..73354ee 100644
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
+	bool notify;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)

@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		}
 	}
 
+	/*
+	 * Prepare to kick but don't notify yet - we can't do this before
+	 * device is ready.
+	 */
+	notify = virtqueue_kick_prepare(vrp->rvq);
+
+	/* From this point on, we can notify and get callbacks. */
+	virtio_device_ready(vdev);
+
 	/* tell the remote processor it can start sending messages */
-	virtqueue_kick(vrp->rvq);
+	/*
+	 * this might be concurrent with callbacks, but we are only
+	 * doing notify, not a full kick here, so that's ok.
+	 */
+	if (notify)
+		virtqueue_notify(vrp->rvq);
 
 	dev_info(&vdev->dev, "rpmsg host is online\n");

Signature · 1 line

MST

Rusty Russell

Applied

2015-03-09 07:09 UTC #1 lore raw

Quoted 8 lines · Michael S. Tsirkin

virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.

Quoted 8 lines

The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.
Signed-off-by Michael S. Tsirkin <[email protected]>

Applied. I'll wait for Ohad to ack before sending to Linus.

BTW I assume you have a version of qemu which warns on these kind of failures? That'd be nice to have!

Thanks,
Rusty.

Quoted 45 lines

---

Note: compile-tested only.

 drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/rpmsg/virtio_rpmsg_bus.c +16 -1

index 92f6af6..73354ee 100644
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
+	bool notify;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)

@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		}
 	}
 
+	/*
+	 * Prepare to kick but don't notify yet - we can't do this before
+	 * device is ready.
+	 */
+	notify = virtqueue_kick_prepare(vrp->rvq);
+
+	/* From this point on, we can notify and get callbacks. */
+	virtio_device_ready(vdev);
+
 	/* tell the remote processor it can start sending messages */
-	virtqueue_kick(vrp->rvq);
+	/*
+	 * this might be concurrent with callbacks, but we are only
+	 * doing notify, not a full kick here, so that's ok.
+	 */
+	if (notify)
+		virtqueue_notify(vrp->rvq);
 
 	dev_info(&vdev->dev, "rpmsg host is online\n");

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2015-03-09 08:27 UTC #2 lore raw

Quoted 18 lines · Rusty Russell

Quoted 8 lines · Michael S. Tsirkin
virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
Quoted 8 lines
The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.
Signed-off-by Michael S. Tsirkin <[email protected]>

Quoted 7 lines

Applied. I'll wait for Ohad to ack before sending to Linus.
BTW I assume you have a version of qemu which warns on these kind of failures? That'd be nice to have!
Thanks,
Rusty.

Yes but it's hacky ATM - it's just a one-liner that checks the status on kick and does fprintf(stderr).
I'm trying to rework the code so it'll actually
set the status automatically, but there are some difficulties there when ioeventfd is used,
in particular we need a way to re-inject the kick
after status update.

Quoted 45 lines

---

Note: compile-tested only.

 drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/rpmsg/virtio_rpmsg_bus.c +16 -1

index 92f6af6..73354ee 100644
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
+	bool notify;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)

@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		}
 	}
 
+	/*
+	 * Prepare to kick but don't notify yet - we can't do this before
+	 * device is ready.
+	 */
+	notify = virtqueue_kick_prepare(vrp->rvq);
+
+	/* From this point on, we can notify and get callbacks. */
+	virtio_device_ready(vdev);
+
 	/* tell the remote processor it can start sending messages */
-	virtqueue_kick(vrp->rvq);
+	/*
+	 * this might be concurrent with callbacks, but we are only
+	 * doing notify, not a full kick here, so that's ok.
+	 */
+	if (notify)
+		virtqueue_notify(vrp->rvq);
 
 	dev_info(&vdev->dev, "rpmsg host is online\n");

Signature · 1 line

MST

Rusty Russell

Applied

2015-03-09 07:09 UTC #3 lore raw

In reply to Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.

Quoted 8 lines

The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.
Signed-off-by Michael S. Tsirkin <[email protected]>

Applied. I'll wait for Ohad to ack before sending to Linus.

BTW I assume you have a version of qemu which warns on these kind of failures? That'd be nice to have!

Thanks,
Rusty.

Quoted 45 lines

---

Note: compile-tested only.

 drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/rpmsg/virtio_rpmsg_bus.c +16 -1

index 92f6af6..73354ee 100644
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
+	bool notify;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)

@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		}
 	}
 
+	/*
+	 * Prepare to kick but don't notify yet - we can't do this before
+	 * device is ready.
+	 */
+	notify = virtqueue_kick_prepare(vrp->rvq);
+
+	/* From this point on, we can notify and get callbacks. */
+	virtio_device_ready(vdev);
+
 	/* tell the remote processor it can start sending messages */
-	virtqueue_kick(vrp->rvq);
+	/*
+	 * this might be concurrent with callbacks, but we are only
+	 * doing notify, not a full kick here, so that's ok.
+	 */
+	if (notify)
+		virtqueue_notify(vrp->rvq);
 
 	dev_info(&vdev->dev, "rpmsg host is online\n");

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2015-03-09 08:41 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 11 lines · Michael S. Tsirkin

virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.

Quoted 6 lines

Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Ohad, can you review and ack pls?

Quoted 43 lines

Note: compile-tested only.

 drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

drivers/rpmsg/virtio_rpmsg_bus.c +16 -1

index 92f6af6..73354ee 100644
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
+	bool notify;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)

@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		}
 	}
 
+	/*
+	 * Prepare to kick but don't notify yet - we can't do this before
+	 * device is ready.
+	 */
+	notify = virtqueue_kick_prepare(vrp->rvq);
+
+	/* From this point on, we can notify and get callbacks. */
+	virtio_device_ready(vdev);
+
 	/* tell the remote processor it can start sending messages */
-	virtqueue_kick(vrp->rvq);
+	/*
+	 * this might be concurrent with callbacks, but we are only
+	 * doing notify, not a full kick here, so that's ok.
+	 */
+	if (notify)
+		virtqueue_notify(vrp->rvq);
 
 	dev_info(&vdev->dev, "rpmsg host is online\n");

Signature · 1 line

MST

Ohad Ben-Cohen

Acked-by

2015-03-11 12:45 UTC #5 lore raw

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Michael S. Tsirkin
virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.

Quoted 8 lines

Quoted 6 lines
Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Ohad, can you review and ack pls?

Sure,

Acked-by Ohad Ben-Cohen <[email protected]>

Rusty Russell

Applied

2015-03-12 01:05 UTC #6 lore raw

Quoted 17 lines · Ohad Ben-Cohen

Quoted 16 lines · Michael S. Tsirkin
Quoted 15 lines · Michael S. Tsirkin
virtio spec requires that all drivers set DRIVER_OK before using devices. While rpmsg isn't yet included in the virtio 1 spec, previous spec versions also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
The fix isn't trivial since simply calling virtio_device_ready earlier would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before virtio_device_ready - when we know we won't get interrupts. notify right afterwards.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Ohad, can you review and ack pls?
Sure,
Acked-by Ohad Ben-Cohen <[email protected]>

Applied.

Thanks,
Rusty.