weiping zhang
2017-Nov-29 01:23 UTC
[PATCH] virtio: release virtio index when fail to device_register
index can be reused by other virtio device. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/virtio/virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 48230a5..bf7ff39 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) /* device_register() causes the bus infrastructure to look for a * matching driver. */ err = device_register(&dev->dev); + if (err) + ida_simple_remove(&virtio_index_ida, dev->index); out: if (err) virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); -- 2.9.4
Cornelia Huck
2017-Nov-29 09:50 UTC
[PATCH] virtio: release virtio index when fail to device_register
On Wed, 29 Nov 2017 09:23:01 +0800 weiping zhang <zwp10758 at gmail.com> wrote:> index can be reused by other virtio device. > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>Reviewed-by: Cornelia Huck <cohuck at redhat.com>> --- > drivers/virtio/virtio.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 48230a5..bf7ff39 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) > /* device_register() causes the bus infrastructure to look for a > * matching driver. */ > err = device_register(&dev->dev); > + if (err) > + ida_simple_remove(&virtio_index_ida, dev->index); > out: > if (err) > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);I think your patch is correct (giving up the index if we failed to register), but this made me look at our error handling if a virtio device failed to register. We hold an extra reference to the struct device, even after a failed register, and it is the responsibility of the caller to give up that reference once no longer needed. As callers toregister_virtio_device() embed the struct virtio_device, it needs to be their responsibility. Looking at the existing callers, - ccw does a put_device - pci, mmio and remoteproc do nothing, causing a leak - vop does a free on the embedding structure, which is a big no-no Thoughts?
Michael S. Tsirkin
2017-Nov-29 13:55 UTC
[PATCH] virtio: release virtio index when fail to device_register
On Wed, Nov 29, 2017 at 09:23:01AM +0800, weiping zhang wrote:> index can be reused by other virtio device. > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>Thanks! I've queued this up, this is needed on stable as well.> --- > drivers/virtio/virtio.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 48230a5..bf7ff39 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) > /* device_register() causes the bus infrastructure to look for a > * matching driver. */ > err = device_register(&dev->dev); > + if (err) > + ida_simple_remove(&virtio_index_ida, dev->index); > out: > if (err) > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > -- > 2.9.4
weiping zhang
2017-Dec-01 16:55 UTC
[PATCH] virtio: release virtio index when fail to device_register
On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:> On Wed, 29 Nov 2017 09:23:01 +0800 > weiping zhang <zwp10758 at gmail.com> wrote: > > > index can be reused by other virtio device. > > > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > > Reviewed-by: Cornelia Huck <cohuck at redhat.com> > > > --- > > drivers/virtio/virtio.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index 48230a5..bf7ff39 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) > > /* device_register() causes the bus infrastructure to look for a > > * matching driver. */ > > err = device_register(&dev->dev); > > + if (err) > > + ida_simple_remove(&virtio_index_ida, dev->index); > > out: > > if (err) > > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > > I think your patch is correct (giving up the index if we failed to > register), but this made me look at our error handling if a virtio > device failed to register. > > We hold an extra reference to the struct device, even after a failed > register, and it is the responsibility of the caller to give up that > reference once no longer needed. As callers toregister_virtio_device() > embed the struct virtio_device, it needs to be their responsibility. > Looking at the existing callers, > > - ccw does a put_device > - pci, mmio and remoteproc do nothing, causing a leak > - vop does a free on the embedding structure, which is a big no-no > > Thoughts?Sorry to relay late and thanks for your review. Do you mean the "extra reference to the struct device" caused by the following code? err = device_register(&dev->dev); device_add(dev) get_device(dev) If I'm understand right, I think there is no extra reference if we fail virtio_register_device, because if device_register we don't get a reference. -- Thanks weiping
Maybe Matching Threads
- [PATCH] virtio: release virtio index when fail to device_register
- [PATCH] virtio: release virtio index when fail to device_register
- [PATCH] virtio: release virtio index when fail to device_register
- [PATCH v4 0/4] use put_device to cleanup resource
- [PATCH v4 0/4] use put_device to cleanup resource