kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Halil Pasic <pasic@linux.ibm.com>
To: Cornelia Huck <cohuck@redhat.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	Heiko Carstens <hca@linux.ibm.com>,
	Vasily Gorbik <gor@linux.ibm.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Pierre Morel <pmorel@linux.ibm.com>,
	Michael Mueller <mimu@linux.ibm.com>,
	linux-s390@vger.kernel.org,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: bfu@redhat.com, Vineeth Vijayan <vneethv@linux.ibm.com>
Subject: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
Date: Wed, 15 Sep 2021 23:57:42 +0200	[thread overview]
Message-ID: <20210915215742.1793314-1-pasic@linux.ibm.com> (raw)

Since commit 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers") we were supposed to make sure that
virtio_ccw_release_dev() completes before the ccw device, and the
attached dma pool are torn down, but unfortunately we did not.
Before that commit it used to be OK to delay cleaning up the memory
allocated by virtio-ccw indefinitely (which isn't really intuitive for
guys used to destruction happens in reverse construction order).

To accomplish this let us take a reference on the ccw device before we
allocate the dma_area and give it up after dma_area was freed.

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers")
Reported-by: bfu@redhat.com
---

I'm not certain this is the only hot-unplug and teardonw related problem
with virtio-ccw.

Some things that are not perfectly clear to me:
* What would happen if we observed an hot-unplug while we are doing
  wait_event() in ccw_io_helper()? Do we get stuck? I don't thin we
  are guaranteed to receive an irq for a subchannel that is gone.
* cdev->online seems to be manipulated under cdev->ccwlock, but
  in virtio_ccw_remove() we look at it to decide should we clean up
  or not. What is the idea there? I guess we want to avoid doing
  if nothing is there or twice. But I don't understand how stuff
  interlocks.
* Can virtio_ccw_remove() get called while !cdev->online and 
  virtio_ccw_online() is running on a different cpu? If yes, what would
  happen then?
 
The main addresse of these questions is Conny ;).

An alternative to this approach would be to inc and dec the refcount
in ccw_device_dma_zalloc() and ccw_device_dma_free() respectively.

---
 drivers/s390/virtio/virtio_ccw.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index d35e7a3f7067..99141df3259b 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1006,10 +1006,12 @@ static void virtio_ccw_release_dev(struct device *_d)
 {
 	struct virtio_device *dev = dev_to_virtio(_d);
 	struct virtio_ccw_device *vcdev = to_vc_device(dev);
+	struct ccw_device *cdev = READ_ONCE(vcdev->cdev);
 
 	ccw_device_dma_free(vcdev->cdev, vcdev->dma_area,
 			    sizeof(*vcdev->dma_area));
 	kfree(vcdev);
+	put_device(&cdev->dev);
 }
 
 static int irb_is_error(struct irb *irb)
@@ -1262,6 +1264,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 	struct virtio_ccw_device *vcdev;
 	unsigned long flags;
 
+	get_device(&cdev->dev);
 	vcdev = kzalloc(sizeof(*vcdev), GFP_KERNEL);
 	if (!vcdev) {
 		dev_warn(&cdev->dev, "Could not get memory for virtio\n");
@@ -1315,6 +1318,7 @@ static int virtio_ccw_online(struct ccw_device *cdev)
 				    sizeof(*vcdev->dma_area));
 	}
 	kfree(vcdev);
+	put_device(&cdev->dev);
 	return ret;
 }
 

base-commit: 3ca706c189db861b2ca2019a0901b94050ca49d8
-- 
2.25.1


             reply	other threads:[~2021-09-15 21:57 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 21:57 Halil Pasic [this message]
2021-09-15 22:00 ` [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown Halil Pasic
2021-09-16  8:59 ` Cornelia Huck
2021-09-16 13:18   ` Halil Pasic
2021-09-17  8:40     ` Cornelia Huck
2021-09-19 22:39       ` Halil Pasic
2021-09-20  7:41         ` Vineeth Vijayan
2021-09-20 10:07           ` Cornelia Huck
2021-09-21  3:25             ` Halil Pasic
2021-09-21 12:09               ` Cornelia Huck
2021-09-21 13:31               ` Vineeth Vijayan
2021-09-21 16:52                 ` Halil Pasic
2021-09-21 18:25                   ` Vineeth Vijayan
2021-09-20 10:30         ` Cornelia Huck
2021-09-20 13:27           ` Halil Pasic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210915215742.1793314-1-pasic@linux.ibm.com \
    --to=pasic@linux.ibm.com \
    --cc=bfu@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=cohuck@redhat.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=mimu@linux.ibm.com \
    --cc=pmorel@linux.ibm.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=vneethv@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).