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 at linux.ibm.com>
Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and
classic notifiers")
Reported-by: bfu at 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
s/vritio/virtio/ (subject) [..] On Wed, 15 Sep 2021 23:57:42 +0200 Halil Pasic <pasic at linux.ibm.com> wrote:> 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). >[..]
Cornelia Huck
2021-Sep-16 08:59 UTC
[PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
On Wed, Sep 15 2021, Halil Pasic <pasic at linux.ibm.com> wrote:> 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 at linux.ibm.com> > Fixes: 48720ba56891 ("virtio/s390: use DMA memory for ccw I/O and > classic notifiers") > Reported-by: bfu at 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.Hm. I think we may need to do a wake_up during remove handling.> * 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.We only created the virtio device when we onlined the ccw device. Do you have a better idea how to check for that? (And yes, I'm not sure the locking is correct.)> * 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?All of the remove/online/... etc. callbacks are invoked via the ccw bus code. We have to trust that it gets it correct :) (Or have the common I/O layer maintainers double-check it.)> > 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.Yeah, I also thought about that. This would give us more get/put operations, but might be the safer option.> > --- > 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