* [PATCH 0/3] fix cleanup for fail to register_virtio_device
@ 2017-12-11 15:53 weiping zhang
2017-12-11 15:55 ` [PATCH 1/3] virtio_pci: use put_device instead of kfree weiping zhang
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: weiping zhang @ 2017-12-11 15:53 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
This series fix the cleanup for the caller of register_virtio_device,
the main work is use put_device instead of kfree.
weiping zhang (3):
virtio_pci: use put_device instead of kfree
virtio: use put_device instead of kfree
virtio: put reference count of virtio_device.dev
drivers/misc/mic/vop/vop_main.c | 16 +++++++++-------
drivers/remoteproc/remoteproc_virtio.c | 2 +-
drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
3 files changed, 19 insertions(+), 16 deletions(-)
--
2.9.4
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] virtio_pci: use put_device instead of kfree 2017-12-11 15:53 [PATCH 0/3] fix cleanup for fail to register_virtio_device weiping zhang @ 2017-12-11 15:55 ` weiping zhang 2017-12-12 10:00 ` Cornelia Huck 2017-12-11 15:55 ` [PATCH 2/3] virtio: " weiping zhang 2017-12-11 15:55 ` [PATCH 3/3] virtio: put reference count of virtio_device.dev weiping zhang 2 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-12-11 15:55 UTC (permalink / raw) To: cohuck, mst, jasowang; +Cc: virtualization don't free vp_dev until vp_dev->vdev.dev.release be called. Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> --- drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 1c4797e..91d20f7 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, pci_set_master(pci_dev); rc = register_virtio_device(&vp_dev->vdev); - if (rc) - goto err_register; + if (rc) { + if (vp_dev->ioaddr) + virtio_pci_legacy_remove(vp_dev); + else + virtio_pci_modern_remove(vp_dev); + pci_disable_device(pci_dev); + put_device(&vp_dev->vdev.dev); + } - return 0; + return rc; -err_register: - if (vp_dev->ioaddr) - virtio_pci_legacy_remove(vp_dev); - else - virtio_pci_modern_remove(vp_dev); err_probe: pci_disable_device(pci_dev); err_enable_device: -- 2.9.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] virtio_pci: use put_device instead of kfree 2017-12-11 15:55 ` [PATCH 1/3] virtio_pci: use put_device instead of kfree weiping zhang @ 2017-12-12 10:00 ` Cornelia Huck 2017-12-12 13:11 ` weiping zhang 0 siblings, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2017-12-12 10:00 UTC (permalink / raw) To: weiping zhang; +Cc: virtualization, mst On Mon, 11 Dec 2017 23:55:16 +0800 weiping zhang <zhangweiping@didichuxing.com> wrote: > don't free vp_dev until vp_dev->vdev.dev.release be called. Maybe add the same description as in your virtio_mmio patch so that it is clear why the kfree() is not ok? > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> > --- > drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 1c4797e..91d20f7 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > pci_set_master(pci_dev); > > rc = register_virtio_device(&vp_dev->vdev); > - if (rc) > - goto err_register; > + if (rc) { > + if (vp_dev->ioaddr) > + virtio_pci_legacy_remove(vp_dev); > + else > + virtio_pci_modern_remove(vp_dev); > + pci_disable_device(pci_dev); > + put_device(&vp_dev->vdev.dev); > + } > > - return 0; > + return rc; > > -err_register: > - if (vp_dev->ioaddr) > - virtio_pci_legacy_remove(vp_dev); > - else > - virtio_pci_modern_remove(vp_dev); > err_probe: > pci_disable_device(pci_dev); > err_enable_device: Otherwise, looks good. Reviewed-by: Cornelia Huck <cohuck@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] virtio_pci: use put_device instead of kfree 2017-12-12 10:00 ` Cornelia Huck @ 2017-12-12 13:11 ` weiping zhang 0 siblings, 0 replies; 9+ messages in thread From: weiping zhang @ 2017-12-12 13:11 UTC (permalink / raw) To: Cornelia Huck; +Cc: virtualization, mst On Tue, Dec 12, 2017 at 11:00:25AM +0100, Cornelia Huck wrote: > On Mon, 11 Dec 2017 23:55:16 +0800 > weiping zhang <zhangweiping@didichuxing.com> wrote: > > > don't free vp_dev until vp_dev->vdev.dev.release be called. > > Maybe add the same description as in your virtio_mmio patch so that it > is clear why the kfree() is not ok? OK, I'll add it at V2. > > > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> > > --- > > drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- > > 1 file changed, 9 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > > index 1c4797e..91d20f7 100644 > > --- a/drivers/virtio/virtio_pci_common.c > > +++ b/drivers/virtio/virtio_pci_common.c > > @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > > pci_set_master(pci_dev); > > > > rc = register_virtio_device(&vp_dev->vdev); > > - if (rc) > > - goto err_register; > > + if (rc) { > > + if (vp_dev->ioaddr) > > + virtio_pci_legacy_remove(vp_dev); > > + else > > + virtio_pci_modern_remove(vp_dev); > > + pci_disable_device(pci_dev); > > + put_device(&vp_dev->vdev.dev); > > + } > > > > - return 0; > > + return rc; > > > > -err_register: > > - if (vp_dev->ioaddr) > > - virtio_pci_legacy_remove(vp_dev); > > - else > > - virtio_pci_modern_remove(vp_dev); > > err_probe: > > pci_disable_device(pci_dev); > > err_enable_device: > > Otherwise, looks good. > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> Thanks a ton ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] virtio: use put_device instead of kfree 2017-12-11 15:53 [PATCH 0/3] fix cleanup for fail to register_virtio_device weiping zhang 2017-12-11 15:55 ` [PATCH 1/3] virtio_pci: use put_device instead of kfree weiping zhang @ 2017-12-11 15:55 ` weiping zhang 2017-12-12 10:17 ` Cornelia Huck 2017-12-11 15:55 ` [PATCH 3/3] virtio: put reference count of virtio_device.dev weiping zhang 2 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-12-11 15:55 UTC (permalink / raw) To: cohuck, mst, jasowang; +Cc: virtualization don't free vp_vdev until vp_vdev.dev.release be called. Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> --- drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index a341938..8c716a0 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data) static void vop_virtio_release_dev(struct device *_d) { - /* - * No need for a release method similar to virtio PCI. - * Provide an empty one to avoid getting a warning from core. - */ + struct virtio_device *vdev = + container_of(_d, struct virtio_device, dev); + struct _vop_vdev *vop_vdev = + container_of(vdev, struct _vop_vdev, vdev); + + kfree(vop_vdev); } /* @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, dev_err(_vop_dev(vdev), "Failed to register vop device %u type %u\n", offset, type); - goto free_irq; + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); + put_device(&vdev->vdev.dev); + return ret; } writeq((u64)vdev, &vdev->dc->vdev); dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n", @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, return 0; -free_irq: - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); kfree: kfree(vdev); return ret; -- 2.9.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] virtio: use put_device instead of kfree 2017-12-11 15:55 ` [PATCH 2/3] virtio: " weiping zhang @ 2017-12-12 10:17 ` Cornelia Huck 2017-12-12 13:13 ` weiping zhang 0 siblings, 1 reply; 9+ messages in thread From: Cornelia Huck @ 2017-12-12 10:17 UTC (permalink / raw) To: weiping zhang; +Cc: virtualization, mst On Mon, 11 Dec 2017 23:55:26 +0800 weiping zhang <zhangweiping@didichuxing.com> wrote: > don't free vp_vdev until vp_vdev.dev.release be called. Same comment as for the virtio_pci patch. > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> > --- > drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c > index a341938..8c716a0 100644 > --- a/drivers/misc/mic/vop/vop_main.c > +++ b/drivers/misc/mic/vop/vop_main.c > @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data) > > static void vop_virtio_release_dev(struct device *_d) > { > - /* > - * No need for a release method similar to virtio PCI. > - * Provide an empty one to avoid getting a warning from core. > - */ > + struct virtio_device *vdev = > + container_of(_d, struct virtio_device, dev); > + struct _vop_vdev *vop_vdev = > + container_of(vdev, struct _vop_vdev, vdev); > + > + kfree(vop_vdev); > } > > /* > @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > dev_err(_vop_dev(vdev), > "Failed to register vop device %u type %u\n", > offset, type); > - goto free_irq; > + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > + put_device(&vdev->vdev.dev); > + return ret; > } > writeq((u64)vdev, &vdev->dc->vdev); > dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n", > @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > > return 0; > > -free_irq: > - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > kfree: > kfree(vdev); > return ret; BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be a put_device() as well). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] virtio: use put_device instead of kfree 2017-12-12 10:17 ` Cornelia Huck @ 2017-12-12 13:13 ` weiping zhang 0 siblings, 0 replies; 9+ messages in thread From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw) To: Cornelia Huck; +Cc: virtualization, mst On Tue, Dec 12, 2017 at 11:17:42AM +0100, Cornelia Huck wrote: > On Mon, 11 Dec 2017 23:55:26 +0800 > weiping zhang <zhangweiping@didichuxing.com> wrote: > > > don't free vp_vdev until vp_vdev.dev.release be called. > > Same comment as for the virtio_pci patch. OK, I'll add it at V2. > > > > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> > > --- > > drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c > > index a341938..8c716a0 100644 > > --- a/drivers/misc/mic/vop/vop_main.c > > +++ b/drivers/misc/mic/vop/vop_main.c > > @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data) > > > > static void vop_virtio_release_dev(struct device *_d) > > { > > - /* > > - * No need for a release method similar to virtio PCI. > > - * Provide an empty one to avoid getting a warning from core. > > - */ > > + struct virtio_device *vdev = > > + container_of(_d, struct virtio_device, dev); > > + struct _vop_vdev *vop_vdev = > > + container_of(vdev, struct _vop_vdev, vdev); > > + > > + kfree(vop_vdev); > > } > > > > /* > > @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > > dev_err(_vop_dev(vdev), > > "Failed to register vop device %u type %u\n", > > offset, type); > > - goto free_irq; > > + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > > + put_device(&vdev->vdev.dev); > > + return ret; > > } > > writeq((u64)vdev, &vdev->dc->vdev); > > dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n", > > @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > > > > return 0; > > > > -free_irq: > > - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > > kfree: > > kfree(vdev); > > return ret; > > BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be > a put_device() as well). thanks for your comments, I'll fix it at v2. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] virtio: put reference count of virtio_device.dev 2017-12-11 15:53 [PATCH 0/3] fix cleanup for fail to register_virtio_device weiping zhang 2017-12-11 15:55 ` [PATCH 1/3] virtio_pci: use put_device instead of kfree weiping zhang 2017-12-11 15:55 ` [PATCH 2/3] virtio: " weiping zhang @ 2017-12-11 15:55 ` weiping zhang 2017-12-12 10:33 ` Cornelia Huck 2 siblings, 1 reply; 9+ messages in thread From: weiping zhang @ 2017-12-11 15:55 UTC (permalink / raw) To: cohuck, mst, jasowang; +Cc: virtualization rproc_virtio_dev_release will be called iff virtio_device.dev's refer count became to 0. Here we should put vdev.dev, and then rproc->dev's cleanup will be done in rproc_virtio_dev_release. Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> --- drivers/remoteproc/remoteproc_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c index 2946348..b0633fd 100644 --- a/drivers/remoteproc/remoteproc_virtio.c +++ b/drivers/remoteproc/remoteproc_virtio.c @@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) ret = register_virtio_device(vdev); if (ret) { - put_device(&rproc->dev); + put_device(&vdev->dev); dev_err(dev, "failed to register vdev: %d\n", ret); goto out; } -- 2.9.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] virtio: put reference count of virtio_device.dev 2017-12-11 15:55 ` [PATCH 3/3] virtio: put reference count of virtio_device.dev weiping zhang @ 2017-12-12 10:33 ` Cornelia Huck 0 siblings, 0 replies; 9+ messages in thread From: Cornelia Huck @ 2017-12-12 10:33 UTC (permalink / raw) To: weiping zhang; +Cc: virtualization, mst On Mon, 11 Dec 2017 23:55:38 +0800 weiping zhang <zhangweiping@didichuxing.com> wrote: > rproc_virtio_dev_release will be called iff virtio_device.dev's > refer count became to 0. Here we should put vdev.dev, and then > rproc->dev's cleanup will be done in rproc_virtio_dev_release. > > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> > --- > drivers/remoteproc/remoteproc_virtio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c > index 2946348..b0633fd 100644 > --- a/drivers/remoteproc/remoteproc_virtio.c > +++ b/drivers/remoteproc/remoteproc_virtio.c > @@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) > > ret = register_virtio_device(vdev); > if (ret) { > - put_device(&rproc->dev); > + put_device(&vdev->dev); > dev_err(dev, "failed to register vdev: %d\n", ret); > goto out; > } Uh, rproc_vdev is really weird. It embeds a struct virtio_device, and in addition a struct kref. At a glance, it seems to be correct, though. Your change is more obviously sane. Reviewed-by: Cornelia Huck <cohuck@redhat.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-12 13:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-12-11 15:53 [PATCH 0/3] fix cleanup for fail to register_virtio_device weiping zhang 2017-12-11 15:55 ` [PATCH 1/3] virtio_pci: use put_device instead of kfree weiping zhang 2017-12-12 10:00 ` Cornelia Huck 2017-12-12 13:11 ` weiping zhang 2017-12-11 15:55 ` [PATCH 2/3] virtio: " weiping zhang 2017-12-12 10:17 ` Cornelia Huck 2017-12-12 13:13 ` weiping zhang 2017-12-11 15:55 ` [PATCH 3/3] virtio: put reference count of virtio_device.dev weiping zhang 2017-12-12 10:33 ` Cornelia Huck
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).