* [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
* [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
* [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 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 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 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
* 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
* 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
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).