lkml.org 
[lkml]   [2021]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] virtio/s390: fix vritio-ccw device teardown
On Mon, 20 Sep 2021 12:30:39 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Sep 20 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
[..]
>
> Basically, the vcdev is supposed to be around while the ccw device is
> online (with a tail end until references have been given up, of course.)
> It embeds a virtio device that has the ccw device as a parent, which
> will give us a reference on the ccw device as long as the virtio device
> is alive. Any interactions with the ccw device (except freeing the dma
> buffer) are limited to the time where we still have a reference to it
> via the virtio device.
>

I didn't remember that device_add() takes a reference to the parent, and
that device_del() before device_put(dev) and remove callback.


> >
> >>
> >> > So my intuition tells me that
> >> > drivers should manage explicitly. Yes virtio_ccw happens to have dma
> >> > memory whose lifetime is more or less the lifetime of struct virtio_ccw,
> >> > but that may not be always the case.
> >>
> >> I'm not sure what you're getting at here. Regardless of the lifetime of
> >> the dma memory, it depends on the presence of the ccw device to which it
> >> is tied. This means that the ccw device must not be released while the
> >> dma memory is alive. We can use the approach in your patch here due to
> >> the lifetime of the dma memory that virtio-ccw allocates when we start
> >> using the device and frees when we stop using the device, or we can use
> >> get/put with every allocate/release dma memory pair, which should be
> >> safe for everyone?
> >>
> >
> > What I mean is that ccw_device_dma_[zalloc,free]() take a pointer to the
> > ccw_device. If we get/put in those we can ensure that, provided the
> > alloc and the free calls are properly paired, the device will be still
> > alive (and the pointer valid) for the free, if it was valid for the
> > alloc. But it does not ensure that each and every call to alloc is with
> > a valid pointer, or that other uses of the pointer are OK. So I don't
> > think it is completely safe for everyone, because we could try to use
> > a pointer to a ccw device when not having any dma memory allocated from
> > its pool.
>
> But the problem is the dma memory, right? Also, it is the same issue for
> any potential caller of the ccw_device_dma_* interfaces.

I tend to agree, my argument was based on the assumption that we did not
use to take a reference to the ccw device in virtio_ccw_online(), but we
do via register_virtio_device(). This reference however gets dropped
right before virtio_ccw_release_dev() is called.
>
> >
> > This patch takes reference to cdev before the pointer is published via
> > vcdev->cdev and drops the reference after *vcdev is freed. The idea is
> > that the pointee basically outlives the pointer. (Without having a full
> > understanding of how things are synchronized).
>
> I don't think we have to care about accessing ->cdev (see above.) Plus,
> as we give up the dma memory at the very last point, we would also give
> up the reference via that memory at the very last point, so I'm not sure
> what additional problems could come up.

I understand now. Let me think about it some more. I'm wonderning about
leafs. Will come back at you shortly.

Regards,
Halil

\
 
 \ /
  Last update: 2021-09-20 15:28    [W:1.448 / U:0.388 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site