On Mon, 20 Sep 2021 12:30:39 +0200 Cornelia Huck <cohuck@xxxxxxxxxx> wrote: > On Mon, Sep 20 2021, Halil Pasic <pasic@xxxxxxxxxxxxx> 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
